On 10/23/2018 11:43 AM, Shreyansh Jain wrote: > On Tuesday 23 October 2018 03:21 PM, Ferruh Yigit wrote: >> 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 >> > > Oh!. Thanks - something I had missed reading. > > I don't want to hijack the conversation, but for my clarity, I think > > >>> snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s", > >>> port, name); > > won't be correct. Right?
You are right, it doesn't look correct. > I am not suggesting that it should be changed now that it is already > part of code. > >>> >>> 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> >>> >> >