Re: [dpdk-dev] [PATCH v5] power: add fifo per core for JSON interface

["Hunt, David" <david.hunt@intel.com>]

Hi Anatoly,

On 09/07/2019 16:12, Burakov, Anatoly wrote:
> On 09-Jul-19 4:07 PM, David Hunt wrote:
>> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>>
>> This patch implements a separate FIFO for each cpu core to improve the
>> previous functionality where anyone with access to the FIFO could affect
>> any core on the system. By using appropriate permissions, fifo 
>> interfaces
>> can be configured to only affect the particular cores.
>>
>> Because each FIFO is per core, the following fields have been removed
>> from the command JSON format: 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>
>> Tested-by: David Hunt <david.hunt@intel.com>
>>
>> ---
>> v2:
>> * updated handling vm_name (use proper buff size)
>> * rebase to master changes
>>
>> v3:
>> * improvement to coding style
>>
>> v4:
>> * rebase to tip of master
>>
>> v5:
>> * merged docs into same patch as the code, as per mailing list policy
>> * made changes out of review by Anatoly.
>> ---
>
> <snip>
>
>> -    if (setup_host_channel_info(&chan_info, 0) < 0) {
>> -        rte_free(chan_info);
>> -        return 0;
>> +        ret = fifo_path(socket_path, sizeof(socket_path), i);
>> +        if (ret < 0)
>> +            goto error;
>> +
>> +        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));
>> +            goto error;
>> +        }
>> +        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);
>> +            goto error;
>> +        }
>> +        chan_infos[i] = chan_info;
>> +        rte_strlcpy(chan_info->channel_path, socket_path,
>> +                sizeof(chan_info->channel_path));
>> +
>> +        if (setup_host_channel_info(&chan_info, i) < 0) {
>> +            rte_free(chan_info);
>> +            chan_infos[i] = NULL;
>> +            goto error;
>> +        }
>> +        num_channels_enabled++;
>>       }
>> -    num_channels_enabled++;
>>         return num_channels_enabled;
>> +error:
>> +    /* Clean up the channels opened before we hit an error. */
>> +    for (i = 0; i < RTE_MAX_LCORE; i++) {
>
> You're going up to RTE_MAX_LCORE here, but up to ci->core_count in the 
> original loop. Is that intentional?
>
> Other than that,
>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>

Just to be totally clean, I've fixed the loop limit, and respun as v6. :)

Thanks for the review.

Rgds,

Dave.