[dpdk-dev] [PATCH v4 1/2] power: add fifo per core for JSON interface
Burakov, Anatoly
anatoly.burakov at intel.com
Mon Jul 8 15:44:32 CEST 2019
On 13-Jun-19 10:21 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski at intel.com>
>
> 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 <lukaszx.krakowiak at intel.com>
> Signed-off-by: Lukasz Gosiewski <lukaszx.gosiewski at intel.com>
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski at intel.com>
> ---
<snip>
> - 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
More information about the dev
mailing list