On 10/23/2018 8:09 AM, Shreyansh Jain wrote:
> Besides the comment I sent before about 'Fixes' before sign-off, a
> single trivial comment inline ...
>
> On Tuesday 23 October 2018 07:20 AM, Rosen Xu wrote:
>> This patch fixes rte_eal_hotplug_add without checking return value issue
>>
>> Signed-off-by: Rosen Xu <rosen...@intel.com>
>> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
>> Cc: rosen...@intel.com
>> ---
>> drivers/raw/ifpga_rawdev/ifpga_rawdev.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
>> b/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
>> index 3fed057..32e318f 100644
>> --- a/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
>> +++ b/drivers/raw/ifpga_rawdev/ifpga_rawdev.c
>> @@ -542,6 +542,7 @@
>> int port;
>> char *name = NULL;
>> char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
>> + int ret = -1;
>>
>> devargs = dev->device.devargs;
>>
>> @@ -583,7 +584,7 @@
>> snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
>> port, name);
>>
>> - rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
>> + ret = rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
>> dev_name, devargs->args);
>
> Ideally, the function argument spreading on next line should start
> underneath the previous arguments - something like:
>
> ret = rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> dev_name, devargs->args);
Hi Shreyansh,
According dpdk coding convention [1], indentation done by hard tab, code seems
inline with coding convention, only perhaps can be done single tab instead of
double.
And to remind again, I am not for syntax discussions but just defining one and
consistently follow it .
[1]
https://doc.dpdk.org/guides/contributing/coding_style.html#c-indentation
https://doc.dpdk.org/guides/contributing/coding_style.html#prototypes
>
> But, in this file this is not being done at multiple places (for
> example, the snprintf in this code snippet). So, either you can ignore
> this comment, or fix it for just this change.
>
>> end:
>> if (kvlist)
>> @@ -591,7 +592,7 @@
>> if (name)
>> free(name);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int
>>
>
> Otherwise, the patch is simple enough.
>
> Acked-by: Shreyansh Jain <shreyansh.j...@nxp.com>
>
