From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id 057843572 for ; Tue, 19 Jun 2018 13:48:42 +0200 (CEST) Received: by mail-wr0-f193.google.com with SMTP id k16-v6so20237768wro.0 for ; Tue, 19 Jun 2018 04:48:42 -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=u2F1sM9ztf8u0mGwhNUR+YuOxoMnvDc/8J7vc+fNMLY=; b=oDXHxJWH03Sa/fdfzX1YBXg+VBItCLOkU9DV68dabgrsbnu79JVYahyk85A9GdkUpi kTvUNFKlGYYMLvtosBlhSL9n4J9+YA9yoE6n6T4UfdOQcEULt6RnfPjI7dLyBVLoriB5 hTG1+eZRjJ8WxQZvU8qUPkzZ1vAtteoUGIaUF1vKin2JGO8G3dnAtfc6W3ZOCpYEG6R8 CTb5fyahZyPvGoYQPVpujBhVJMjbJ/ZvxjqInuZRhOcIIfJM+6WLtWieJVys8FbAYphO 1FUM+WTcvFKpIQ6FP/z+gyzdSl6asv483724gjL0Ddzp+fpquBMKUw6lvSQo1doMK0zM RL2Q== 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=u2F1sM9ztf8u0mGwhNUR+YuOxoMnvDc/8J7vc+fNMLY=; b=tEk3LtL5vUJ+GELiMQD96YhisecrcYn4q0ddYbB5eOyuHq+0S2KYn5CCj3pOgECf2L cB+XldPOsItUYrhP0TTzvm0P67UvHZeDIS3H+ruKhvJlrllHop9z2ghNgAeUrR9b2GL3 1L38bRVO03SVOTNNt9QlrTxYVCRJUUFZf1+vPRGme+KuTEVy4zNl/KxfJH9aX11vifyz f3JwaqXH4otCPJtHZGcz1PIQVvdJWeOsROKDXWiiBcSFYh2BMBlkI4H6YW+G95OfMjp4 +nRaumtIyyjUleLmw+UtCpLqN1AW2sndX+N4kstp0NbrwwfWs10Ng9kQ35j7gi7/enhj 4LnQ== X-Gm-Message-State: APt69E2Qtb1acsDnn0qpYoGoPneuEXGajs3ENJe3nfItAj5TqMGKgGH6 Fn1YBLwuQQgyZdYOf38BAbmE X-Google-Smtp-Source: ADUXVKKPPtWURKrzi1pA+XM6Cw9mNlUz043yg8KJ7DdkiO7jTGiPlsvCjlwGWPjwJQvUoG4z2ZJ0VQ== X-Received: by 2002:adf:e311:: with SMTP id b17-v6mr14670735wrj.158.1529408922693; Tue, 19 Jun 2018 04:48:42 -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 d3-v6sm15880847wrr.90.2018.06.19.04.48.41 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 19 Jun 2018 04:48:41 -0700 (PDT) Date: Tue, 19 Jun 2018 13:48:52 +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: <20180619114852.lzxwjj7ud5owcyuu@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> <20180606065501.hhrfrti47nr5xigo@laranjeiro-vm.dev.6wind.com> <20180606183926.GA1446@yongseok-MBP.local> <20180607073944.zewdysx6ddrdygoz@laranjeiro-vm.dev.6wind.com> <42BB3FF2-80B8-4250-928C-32D509E32DAA@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <42BB3FF2-80B8-4250-928C-32D509E32DAA@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: Tue, 19 Jun 2018 11:48:43 -0000 On Mon, Jun 18, 2018 at 05:06:41PM +0000, Yongseok Koh wrote: > > > On Jun 7, 2018, at 12:39 AM, Nélio Laranjeiro wrote: > > > > On Wed, Jun 06, 2018 at 11:39:27AM -0700, Yongseok Koh wrote: > >> On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote: > >>> 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. > >> > >> That's logically a wrong interpretation. According to the description, if > >> returning error, rte_errno is set but the opposite isn't always true. Even if > >> rte_errno is set, it doesn't mean there's an error. So the description coincides > >> with that of errno. If you want to enforce preserving rte_errno in case of > >> success, you should amend the documentation. > >> > >>> 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(); > >> > >> This example is weird. You can simply set rte_errno after foo_undo() in this > >> case. > >> > >>> 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. > >> > >> I knew it. That's why rte_errno is saved before calling another API which may > >> change the rte_errno inside. But, we are talking about a case where an API > >> returns success. If caller is supposed to save rte_errno (when it's needed), why > >> does callee have to put some effort to preserve it even in case of success? If > >> rte_errno must be preserved even in case of success, we have to make a big > >> change to preserve rte_errno for cases where a void function is called (or cases > >> where we don't check its return value of non-void function). > >> > >>> This is also the main reason almost all system function only update > >>> errno when no error is encountered. > >> > >> 'Almost' doesn't mean 'all", does it? It is true that such functions must update > >> errno when it returns error but it doesn't care about the value when it returns > >> success. Like the man page I attached above, the errno is significant only when > >> it returns an error. And "a function that succeeds is allowed to change errno." > > > > It is "almost" because a system function touching the errno when the > > function succeed it not common. But as the man page says it is not > > impossible. > > > >> So, the decision point is whether we want to preserve rte_errno in case of > >> success? My opinion is no. > > > > I did not understood it was only a concern about the success of the > > function, even it is better to avoid as most as possible a useless > > store, in this specific case, as errno (rte_errno) has a garbage value, > > I fully agree with you. > > Nelio, > > Do you still want me to make any change for this patch? > Let me know if any. With your modification the function documentation is no more accurate as rte_errno is always set. -- Nélio Laranjeiro 6WIND