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

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

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?

> 
> 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'?