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