Re: [dpdk-dev] [PATCH v4 1/2] power: add fifo per core for JSON interface

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 13-Jun-19 10:21 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@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@intel.com>
> Signed-off-by: Lukasz Gosiewski <lukaszx.gosiewski@intel.com>
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@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