From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id BFBA8A00E6 for ; Mon, 8 Jul 2019 15:44:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6B76C397D; Mon, 8 Jul 2019 15:44:39 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id F03B0325F for ; Mon, 8 Jul 2019 15:44:37 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Jul 2019 06:44:36 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,466,1557212400"; d="scan'208";a="316724443" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.82]) ([10.237.220.82]) by orsmga004.jf.intel.com with ESMTP; 08 Jul 2019 06:44:33 -0700 To: Hajkowski , david.hunt@intel.com Cc: dev@dpdk.org, Lukasz Krakowiak , Lukasz Gosiewski References: <20190424082152.5396-1-lukaszx.gosiewski@intel.com> <20190613092117.7252-1-marcinx.hajkowski@intel.com> <20190613092117.7252-2-marcinx.hajkowski@intel.com> From: "Burakov, Anatoly" Message-ID: <400b5cf3-7b47-3335-18bd-df250e32e6ad@intel.com> Date: Mon, 8 Jul 2019 14:44:32 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20190613092117.7252-2-marcinx.hajkowski@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 1/2] power: add fifo per core for JSON interface X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 13-Jun-19 10:21 AM, Hajkowski wrote: > From: Marcin Hajkowski > > This patch implement a separate FIFO for each cpu core. > For proper handling JSON interface, removed fields from cmds: > core_list, resource_id, name. > > Signed-off-by: Lukasz Krakowiak > Signed-off-by: Lukasz Gosiewski > Signed-off-by: Marcin Hajkowski > --- > - RTE_LOG(ERR, CHANNEL_MANAGER, "Error allocating memory for " > - "channel '%s'\n", socket_path); > - return 0; > - } > - rte_strlcpy(chan_info->channel_path, socket_path, UNIX_PATH_MAX); > + do { > + if (ci->cd[num_channels_enabled].global_enabled_cpus == 0) > + continue; > > - if (setup_host_channel_info(&chan_info, 0) < 0) { > - rte_free(chan_info); > - return 0; > - } > - num_channels_enabled++; > + ret = fifo_path(socket_path, sizeof(socket_path), > + num_channels_enabled); > + if (ret < 0) > + return 0; So if we encounter *any* failure, *all* channels become invalid? Should we at least roll back the changes we've made by this point? This is consistent with previous behavior so maybe not in this patch, but still... > + > + ret = mkfifo(socket_path, 0660); > + RTE_LOG(DEBUG, CHANNEL_MANAGER, "TRY CREATE fifo '%s'\n", > + socket_path); > + if ((errno != EEXIST) && (ret < 0)) { > + RTE_LOG(ERR, CHANNEL_MANAGER, "Cannot create fifo '%s' error: " > + "%s\n", socket_path, strerror(errno)); > + return 0; > + } > + > + if (access(socket_path, F_OK) < 0) { > + RTE_LOG(ERR, CHANNEL_MANAGER, "Channel path '%s' error: " > + "%s\n", socket_path, strerror(errno)); > + return 0; > + } I believe this is not needed. Trying to do this here is a TOCTOU issue, and if the access fails on open later, you handle that and free the channel info anyway, so this check is essentially useless. > + chan_info = rte_malloc(NULL, sizeof(*chan_info), 0); > + if (chan_info == NULL) { > + RTE_LOG(ERR, CHANNEL_MANAGER, "Error allocating memory for " > + "channel '%s'\n", socket_path); > + return 0; > + } > + strlcpy(chan_info->channel_path, socket_path, > + sizeof(chan_info->channel_path)); should this be rte_strlcpy? > + > + if (setup_host_channel_info(&chan_info, > + num_channels_enabled) < 0) { > + rte_free(chan_info); > + return 0; > + } > + } while (++num_channels_enabled <= ci->core_count); This looks like a for-loop, why is `while` used here? I mean, i don't care either way, it's just a for-loop would have been a more obvious choice... -- Thanks, Anatoly