Re: [dpdk-dev] [PATCH] app/testpmd: fix passing negative parameter to strerror

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 6/5/2020 3:57 AM, Wei Hu (Xavier) wrote:
> 
> 
> On 2020/6/5 0:30, Ferruh Yigit wrote:
>> On 6/4/2020 7:22 AM, Wei Hu (Xavier) wrote:
>>> Currently, there are coverity defect warnings as blow:
>>> Coverity issue:
>>>    In nic_stats_clear function:
>>>      CID 290021 (#1 of 1): Argument cannot be negative (NEGATIVE_RETURNS)
>>>      5. negative_returns: ret is passed to a parameter that cannot be
>>>         negative.
>>>
>>>      CID 289974 (#1 of 1): Argument cannot be negative (NEGATIVE_RETURNS)
>>>      6. negative_returns: ret is passed to a parameter that cannot be
>>>         negative.
>>>
>>>    In nic_xstats_clear function:
>>>      CID 289985 (#1 of 1): Argument cannot be negative (NEGATIVE_RETURNS)
>>>      5. negative_returns: ret is passed to a parameter that cannot be
>>>         negative.
>>>
>>>      CID 289850 (#1 of 1): Argument cannot be negative (NEGATIVE_RETURNS)
>>>      6. negative_returns: ret is passed to a parameter that cannot be
>>>         negative.
>> I guess these coverity IDs are from the internal coverity, because I can't find
>> them in the public coverity.
>> If it is internal, not sure about the benefit of documenting them in the commit
>> log, since no one except huawei can access them. What do you think to remove all
>> above reference?
> Yes, these are internal covertiy defects information. Maybe we can remove
> internal coverity CID xxxx and reserve the description of the defect in the
> commit log.
> 
> By the way,  when we access the "View defects" on the page of the public 
> coverity
> https://scan.coverity.com/projects/dpdk-data-plane-development-kit?tab=overview 
> 
> , The browser prompts HTTP ERROR 403.

Can you please try a few times, I am also getting same error time to time but it
works after some tr.

And when it is successful, I have following URL, not sure if it works but you
can try this too:
https://scan4.coverity.com/reports.htm#v22369/p10075


>>> This patch fixes them by passing '-ret' to the function strerror() when ret
>>> is negative.
>>>
>>> Fixes: af75078fece3 ("first public release")
>>> Fixes: 9eb974221f44 ("app/testpmd: fix statistics after reset")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> ---
>>>   app/test-pmd/config.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 5381207..356d0d2 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -244,14 +244,14 @@ nic_stats_clear(portid_t port_id)
>>>   	ret = rte_eth_stats_reset(port_id);
>>>   	if (ret != 0) {
>>>   		printf("%s: Error: failed to reset stats (port %u): %s",
>>> -		       __func__, port_id, strerror(ret));
>>> +		       __func__, port_id, strerror(-ret));
>>>   		return;
>>>   	}
>>>   
>>>   	ret = rte_eth_stats_get(port_id, &ports[port_id].stats);
>>>   	if (ret != 0) {
>>>   		printf("%s: Error: failed to get stats (port %u): %s",
>>> -		       __func__, port_id, strerror(ret));
>>> +		       __func__, port_id, strerror(-ret));
>> Although in practice this may be the case, the 'rte_eth_stats_get()' function
>> documentation doesn't guarantee that return value will be negative, it says:
>>
>> "
>>   * @return
>>   *   Zero if successful. Non-zero otherwise
>> "
>>
>> To be accurate, what do you think to adding a negative check for 'ret' before
>> doing '-ret'?
> OK, we will add a negative check in V2.
> 
> Thanks, Xavier
>