drivers/raw/ifpga_rawdev: fix coverity issue 323508
Checks
Commit Message
This patch fixes rte_eal_hotplug_add without checking return value issue
Signed-off-by: Rosen Xu <rosen.xu@intel.com>
Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
Cc: rosen.xu@intel.com
---
drivers/raw/ifpga_rawdev/ifpga_rawdev.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Comments
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.xu@intel.com>
> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
> Cc: rosen.xu@intel.com
> ---
Fixes comes *before* signed-off.
<Patch header>
<Message>
..
<Fixes>
<Signed-off...>
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.xu@intel.com>
> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
> Cc: rosen.xu@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);
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.jain@nxp.com>
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.xu@intel.com>
>> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
>> Cc: rosen.xu@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.jain@nxp.com>
>
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.xu@intel.com>
>>> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
>>> Cc: rosen.xu@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?
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.jain@nxp.com>
>>
>
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.xu@intel.com>
>>>> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
>>>> Cc: rosen.xu@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.jain@nxp.com>
>>>
>>
>
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.xu@intel.com>
>> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
>> Cc: rosen.xu@intel.com
<...>
> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
Coverity issue: 323508
Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
Cc: stable@dpdk.org
Applied to dpdk-next-net/master, thanks.
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, October 25, 2018 18:26
> To: Shreyansh Jain <shreyansh.jain@nxp.com>; Xu, Rosen
> <rosen.xu@intel.com>; dev@dpdk.org
> Cc: Zhang, Tianfei <tianfei.zhang@intel.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH] drivers/raw/ifpga_rawdev: fix coverity issue
> 323508
>
> 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.xu@intel.com>
> >> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
> >> Cc: rosen.xu@intel.com
>
> <...>
> > Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>
> Coverity issue: 323508
> Fixes: ef1e8ede3da5 ("raw/ifpga: add Intel FPGA bus rawdev driver")
> Cc: stable@dpdk.org
>
> Applied to dpdk-next-net/master, thanks.
Thanks all.
@@ -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);
end:
if (kvlist)
@@ -591,7 +592,7 @@
if (name)
free(name);
- return 0;
+ return ret;
}
static int