diff mbox series

drivers/raw/ifpga_rawdev: fix coverity issue 323508

Message ID 1540259449-135300-1-git-send-email-rosen.xu@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series drivers/raw/ifpga_rawdev: fix coverity issue 323508 |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Xu, Rosen Oct. 23, 2018, 1:50 a.m. UTC 
  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

Shreyansh Jain Oct. 23, 2018, 6:59 a.m. UTC | #1 
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...>
Shreyansh Jain Oct. 23, 2018, 7:09 a.m. UTC | #2 
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>
Ferruh Yigit Oct. 23, 2018, 9:51 a.m. UTC | #3 
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>
>
Shreyansh Jain Oct. 23, 2018, 10:43 a.m. UTC | #4 
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>
>>
>
Ferruh Yigit Oct. 23, 2018, 12:14 p.m. UTC | #5 
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>
>>>
>>
>
Ferruh Yigit Oct. 25, 2018, 10:25 a.m. UTC | #6 
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.
Xu, Rosen Oct. 25, 2018, 12:49 p.m. UTC | #7 
> -----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.
diff mbox series

Patch

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);
 end:
 	if (kvlist)
@@ -591,7 +592,7 @@ 
 	if (name)
 		free(name);
 
-	return 0;
+	return ret;
 }
 
 static int