From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Subject: Re: [PATCH] net/mlx5: fix error number handling Date: Wed, 6 Jun 2018 08:55:01 +0200 Message-ID: <20180606065501.hhrfrti47nr5xigo@laranjeiro-vm.dev.6wind.com> References: <20180604173731.29125-1-yskoh@mellanox.com> <20180605065246.mw7xnk24cfwxy4an@laranjeiro-vm.dev.6wind.com> <84C956B6-28EE-4F82-97AE-5E9C371DD115@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Adrien Mazarguil , "dev@dpdk.org" , Shahaf Shuler , dpdk stable , "Xueming(Steven) Li" To: Yongseok Koh Return-path: Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by dpdk.org (Postfix) with ESMTP id 687911B7D3 for ; Wed, 6 Jun 2018 08:54:34 +0200 (CEST) Received: by mail-wr0-f195.google.com with SMTP id y15-v6so4933958wrg.11 for ; Tue, 05 Jun 2018 23:54:34 -0700 (PDT) Content-Disposition: inline In-Reply-To: <84C956B6-28EE-4F82-97AE-5E9C371DD115@mellanox.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jun 05, 2018 at 09:36:32PM +0000, Yongseok Koh wrote: > > On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro 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 > >> --- > >> 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 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. Functions in this PMD are defined as is: * @return * 0 on success, a negative errno value otherwise and rte_errno is set. Which means rte_errno is only modified in case of error. This fix does not respect the documentation of the function or any other function of the PMD which can return errors. rte_errno is only set if an error is encountered and contains only the error code of the first error sub-sequent ones are considered consequences of the first one and thus not preserved. Not preserving the rte_errno in roll backs is equivalent to not setting it at all as a function called by the rollback may also set it, example: { void * a; foo_do(); a = malloc(10); if (!a) { rte_errno = ENOMEM; foo_undo(); return -rte_errno; } } If foo_undo() also encounter an error it will modify the rte_errno which may have a value different from ENOMEM, for the callee won't be informed the error is due to a memory issue and thus cannot make counter parts. In such situation the rte_errno must be preserved to keep the ENOMEM error code. This is also the main reason almost all system function only update errno when no error is encountered. Regards, -- Nélio Laranjeiro 6WIND