Re: [dpdk-dev] [PATCH] net/mlx5: fix error number handling

[Yongseok Koh <yskoh@mellanox.com>]

> On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> 
> On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
>> rte_errno should be saved only if error has occurred because rte_errno
>> could have garbage value.
>> 
>> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
>> Cc: stable@dpdk.org
>> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> ---
>> drivers/net/mlx5/mlx5_flow.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>> index 994be05be..eaffe7495 100644
>> --- a/drivers/net/mlx5/mlx5_flow.c
>> +++ b/drivers/net/mlx5/mlx5_flow.c
>> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
>> 		/* The flow does not match. */
>> 		continue;
>> 	}
>> -	ret = rte_errno; /* Save rte_errno before cleanup. */
>> +	if (ret)
>> +		ret = rte_errno; /* Save rte_errno before cleanup. */
>> 	if (flow)
>> 		mlx5_flow_list_destroy(dev, &priv->flows, flow);
>> exit:
>> -- 
>> 2.11.0
> 
> This patch is not enough, the returned value being -rte_errno if no
> error is detected by the function it cannot set rte_errno nor return it.

We may need to refactor this kind of code (saving and restoring rte_errno). I
still don't understand why we should preserve rte_errno like this.

Even if this function returns success, there's no obligation to preserve
rte_errno in the function. Once it is called, the ownership of rte_errno belongs
to this function. I can't find how we define this per-lcore variable but, from
the man page of errno,

       The  <errno.h>  header  file  defines  the integer variable errno, which
       is set by system calls and some library functions in the event of an
       error to indicate what went wrong.  Its value is significant only when
       the return value of the call indicated an error (i.e., -1 from most
       system calls; -1 or NULL from most library  functions);
       a function that succeeds is allowed to change errno.

So, I still think an API can change rte_errno even if it succeeds, no need to
preserve it. If needed, the caller has to save it.


Thanks,
Yongseok