From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=9qHo5pwZculagygsCWEzLLWcFeMROyCpMBhQh4VQ4XI=; b=zdTWgwm8Gz7QaSNdhWzviH9OD0WThDxkiovRUVXGVx1OpuvDVKJzlr9cOn/0rsUbrd Dkhle6E7rpEeTh1TzjjxZuiETDySDGtAvBElpRBtfG2KKcOqlOSi+OOG8G3eQqOBve6o VmyviORlruFLs4vxBSyOTGQ4kiwhnb/6B2vPTFWdrc6YLSx6HjrQCjh5nVIfiONZA4sy DusYtWXw+1iEpk4wyq7pgOplxjR2rdn1oc9teKqbGJZsSZtHSZSDNoJymmNFtqwxmgOB HWqg43sM0GMFc2A03B/5mAhYq9Uu+bZ3ix0OsMQggSE0h2k1+azhyDamO8nSwsATUZxK iBeA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=9qHo5pwZculagygsCWEzLLWcFeMROyCpMBhQh4VQ4XI=; b=L3WILTDPnOiXsSIewTkki2XMZV74IpjymuiHGwIJ2onTYMcsx9rXJ0jeaYHXju4c7d JYMq4uiGnz16M6lEuG/shj6e+MZ3A0nsksM3XB1796KtBkShXzHlBlkEOJ/rR2I24vIy Bbeq79lNckzGWcR6LgZxa++WU3VGC5B81aQ8FUf11xlLkmoqX4YfxD9XiCO0M1Nr4XS3 4rqwbVax6VUiBFA7KMxEvZskBbKKMspjzp80UvDH9LOl25/UJJ9mEi8GCXSIbBf94LB0 Um/0OgEEev5r8jbG2vd7iFsqGBZMia3UaOa5bbyuZFI8S0Z9nZxo2je12vDC+uzv95JO bCrQ== X-Gm-Message-State: APt69E0qjZDX7BimjnNll2K05kJdcM0soBbs74hPUpVZPG/W1qtq0hAN qGF7IDosfQRbFamxBjBJsYab X-Google-Smtp-Source: ADUXVKIbvIFDpGL7neYjj56pTRfWpwkkb6adzjyefzosYgmo3c3h7++jRpImKcP32+uROiazy3NW8Q== X-Received: by 2002:adf:edc6:: with SMTP id v6-v6mr1170210wro.264.1528268074037; Tue, 05 Jun 2018 23:54:34 -0700 (PDT) Received: from laranjeiro-vm.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 132-v6sm4657671wmr.33.2018.06.05.23.54.32 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 05 Jun 2018 23:54:33 -0700 (PDT) Date: Wed, 6 Jun 2018 08:55:01 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Yongseok Koh Cc: Adrien Mazarguil , "dev@dpdk.org" , Shahaf Shuler , dpdk stable , "Xueming(Steven) Li" 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-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <84C956B6-28EE-4F82-97AE-5E9C371DD115@mellanox.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix error number handling X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jun 2018 06:54:34 -0000 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