From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-eopbgr20059.outbound.protection.outlook.com [40.107.2.59]) by dpdk.org (Postfix) with ESMTP id 987C91B72E; Wed, 6 Jun 2018 20:39:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Cs/hnH4QB7/M6qn7K2AEvGdKnum8EDM1zRk3LA77zqI=; b=XmSLrlVqL7DwrBXyyM7HC4ASihSI/UrL6wKYbqqZ3U6FoF21pcIq6RW2TSKOcvN95xP1r0ATwEToLOgl3xUYHd9XIcYd9ZmxcZoZiUmPMbg/WGJ0vrZYxv7qWZlP1O13Lj2IaZV4lYF93Ors5yrsi2xdpnr1LOdumlL3gsnJ0Jk= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=yskoh@mellanox.com; Received: from yongseok-MBP.local (209.116.155.178) by HE1PR0501MB2041.eurprd05.prod.outlook.com (2603:10a6:3:35::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.820.15; Wed, 6 Jun 2018 18:39:44 +0000 Date: Wed, 6 Jun 2018 11:39:27 -0700 From: Yongseok Koh To: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Cc: Adrien Mazarguil , "dev@dpdk.org" , Shahaf Shuler , dpdk stable , "Xueming(Steven) Li" Message-ID: <20180606183926.GA1446@yongseok-MBP.local> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180606065501.hhrfrti47nr5xigo@laranjeiro-vm.dev.6wind.com> User-Agent: Mutt/1.9.3 (2018-01-21) X-Originating-IP: [209.116.155.178] X-ClientProxiedBy: BYAPR03CA0024.namprd03.prod.outlook.com (2603:10b6:a02:a8::37) To HE1PR0501MB2041.eurprd05.prod.outlook.com (2603:10a6:3:35::19) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(48565401081)(5600026)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603328)(7153060)(7193020); SRVR:HE1PR0501MB2041; X-Microsoft-Exchange-Diagnostics: 1; HE1PR0501MB2041; 3:QnDJu6Lov/sZ4RxTIA+ImPKpPzeAPsizAHJy1XaAq1IQMhfTk4pJtpo3C1I1Fe1oXV2xtr6z502+WD3DqzH1g2SIpIhcT43UfxODV+l96mBGBm7nVF9XG7KueJaOBBhjIVRgrCAeay6Ic0U35yYD6J24ACPONRt1dk1FWujcDjeExty/y0LNwpIbRpaiQAsXwhruVE17aJYtQ2FtYWrvd9mvx0UrsOwiiXIo4SlRhRm7PAhtEiNttwMEKG4UBmt0; 25:xGY9zsBCfOmwmD9mjXrSz0Vn4bZxaJVpIvdVgpcgSkenjn1yLMhSsAzvngiw1EuhZzWDD3WMtiWHKz2ACg6Ad/u0t3qZunixykHp4ai8+Z6q3F6mSrDDtouyO7iBg/zZBJih3VCVqigorlfQ/HxzXc+FbrXLQji9Dvrr7k8fthmkrNDWXFRpLphgV1ZHxUBVO3b4fe2+D1hngvE+RzKCF47IFzZ3vBTlox8X2BjCwFfVoo8M0Y5/r77V7w5PuuTcR1RvpV/wdxOBt8rMnYxxAOL/phnnR9SOv//hiY3Ux4rMAQ9Un6ddqftEwaeP4WTXPajYtW8h1YopUPRcCrZ8Jg==; 31:rA67kcCkHkGbFsGEUKMoKtxY/UIIdSvjV4cYAOyySF32Y/r81D20tH32YhxHtnJ2i0bSuUQDJUrntPqFe1WXlhBafIJHLb+Qy+xVdv4Kaw1bl5Sog6nn2HSNyfCkjgko38ZG4EcOUbBjQdOUc6in4/LXzs4ENnFZaW6vHN+qGr6kmWDjWq+ovJMoBNU9Igw6sn4RDIuPGYZcitZXZTmpdgGuz7qgO+9/yIirBDyXmEI= X-MS-TrafficTypeDiagnostic: HE1PR0501MB2041: X-LD-Processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr X-Microsoft-Exchange-Diagnostics: 1; HE1PR0501MB2041; 20:rSi/fga/rlnfn7tDvZn+VWEHM4SnDF85+ifqkF3/5isbUzxzgzNHfdzKZ9eE7WWesD/q708vsGjbO3PZRsKXYbWXISZ50wnJblqDojohToJq5W6XdFVXlVIesra05GxR2hvRs9PI4jZlc/6TDgxIosmLKPF5XkUAe3cB+bkUz2X+I6MxGNRhEdpY+NLlbVzAoCxcgrq/hxeMkcubWl9PFTFd/WHL1LibvvNr+scIdRrjeftIBKy4pbC5NxJXoceIBsDqoxNqoF1MARarpgQw1aF10nkjw+QLVX6SZ9TqNWIJUhZIktXrdFxbbhbz400hZHGTBZpMCBwqyxynEWnquGvVcXPNLrTxQRWHg+ADryNcFG07F3AvxyLjHVvosjXb172RvlrKMGrK7xO6ZOSH3v9rODpCWoWTLIYI74Hq7xNJWXnDF5/yqeEZHkg2JNxJkymwOHCI9jzcsx0KTS3q+es33rHu75j+UiRwjZxtb8PWG5ooJmUaY4SZtmciFP6W; 4:gsiTGZxp6R/hYLV0f+PWfL9BqbgSYL1cqCxBEQGudvwUwk6uHTghIycjsS4VLINIAyLiBH7LvDzdVpvHkVRHdMNVPx6tphssYv2CQxUku+BbjGtYkdkB1i80vXVg1xVUREEEfsqnmVXkSzPYilJCxp6sHvMhCKNP0o2LvoRwaxoWnAgjw+sWwTyFibl3pulBIm0TZGQirJ1JyXS4DLW+0kFOod3Rr0SGTyS9UVVMOMJtLNB5d6R3WIlvSDEouO6s17LcXA608Zk5LgvP/cmtnma5XHo2nRnGIVhrtOOj06t9z1qr+MQSu8Ht4LNpnXmK X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(60795455431006); X-MS-Exchange-SenderADCheck: 1 X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(93006095)(93001095)(10201501046)(3002001)(3231254)(944501410)(52105095)(6055026)(149027)(150027)(6041310)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123562045)(20161123558120)(20161123564045)(6072148)(201708071742011)(7699016); SRVR:HE1PR0501MB2041; BCL:0; PCL:0; RULEID:; SRVR:HE1PR0501MB2041; X-Forefront-PRVS: 06952FC175 X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(396003)(366004)(346002)(39380400002)(376002)(39860400002)(189003)(199004)(1076002)(23756003)(478600001)(5890100001)(305945005)(7736002)(86362001)(229853002)(4326008)(6506007)(52116002)(2906002)(386003)(53546011)(58126008)(55016002)(68736007)(33896004)(6666003)(6916009)(54906003)(76176011)(105586002)(16526019)(6246003)(7696005)(26005)(2870700001)(316002)(486006)(93886005)(6116002)(476003)(53936002)(446003)(107886003)(98436002)(97736004)(33656002)(25786009)(5660300001)(8936002)(3846002)(81156014)(8676002)(81166006)(59450400001)(9686003)(11346002)(106356001)(47776003)(66066001)(186003)(956004)(50466002)(18370500001); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR0501MB2041; H:yongseok-MBP.local; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; Received-SPF: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1; HE1PR0501MB2041; 23:hydOQ10igytzscGNGmlj0BD2ol7/0nGrnKMXt?= =?iso-8859-1?Q?NAKWoeV6o6oSOCyPHhkTq5FyhQyRbbI+eTIkN9V/IrI3pO2zOuKbQL+dkz?= =?iso-8859-1?Q?MYhqsCSue8E51q61FiCZKiZLxVl1BuhPK9es3ww3jf+PBHuuin+A1WegIh?= =?iso-8859-1?Q?93Q8sElrMiqRwH0ZRKoc6PZLuOTAXa53ztLsvBCGiuy9RPruvGJbHdydNj?= =?iso-8859-1?Q?Lf5syu1rJydAg9UYza6SeRVkvw4cASUa/tUOiz3MiQHlOVBYCTwuAcs9N/?= =?iso-8859-1?Q?g2qu5JZa10aPl1yBHh4IQpqgmsI5wqxpvBmAF7Zla9bqhz+Yk3o+XtusFG?= =?iso-8859-1?Q?OIhXQeg2bjhXnl4t74h3J1XzmoNYxi7cNid0POBGiVd8Zk52I5j30TbXE2?= =?iso-8859-1?Q?WrNOrzHLErtnpNRIjlAvbKsJblZVrZmmgMpnavXfzYLyLveOhcGaeR5zJu?= =?iso-8859-1?Q?rqy0t6aii34PmCRfXvFbMrYRcgqa35MO2iAy+0cWlKhxdGS3gfoEFK52nN?= =?iso-8859-1?Q?rlNnjt5qAq2+pYbkRsTfqckNFs5j53GblzppVuWWNraWcnIPbTm/knJgNl?= =?iso-8859-1?Q?pg5fW2uKUfi2hU2ZX6Hc9YAFZNoGa7vN15QQqmzQFMHmwb+r5yrHySkYdc?= =?iso-8859-1?Q?qXlF68ESmsHFQ0bjxnKsZCfJ5OBKw6CRGeSYAdnbWrIou4r22tJ2IHu7+q?= =?iso-8859-1?Q?WQfPnG5PR2sjo/b+OiLT9zuFDPn2nEthLoCxIIvMfu/I1qx+BWS4j7hHMk?= =?iso-8859-1?Q?4ERmRx/MyC8MWtZ5MN5fsRpMoRGmdbpIAJ4ylpof16IRWzATuT10OCsoZU?= =?iso-8859-1?Q?mz58kAjS3WN3D2CGN3wtRPrSQ1uBlM44GvrZ03XVZ3ntXA+OAEcNyu0D5j?= =?iso-8859-1?Q?CpGkWvV9RBnjUAV99iFvL0DnzkxiHTIq/YWSj/m7c82hx1E8ZoCSbmn3aY?= =?iso-8859-1?Q?aSc9adSqww01EINnsZ9pSmuT725SwArrodCIncDuXtRfzCqAZrzzZQWYy8?= =?iso-8859-1?Q?s1QIw9h4cR2ztK35bt/URl/7gTCFrC+gTIHK9WXnmh7pTdIa66KCBViyCG?= =?iso-8859-1?Q?H7Lk/Yer98STeNNOzb5l7k8kze97JQ1cq+rqWSF3B+399Z8f2uTW0SIe1C?= =?iso-8859-1?Q?/VKkQcaMiZP0DaC3YPiM9amYAyLTwFe1rYWu1f8S5y89fTNLMZuk/Pgz9C?= =?iso-8859-1?Q?jkNFsQ7+ZJsqAaA+KBQ2KNVro1u26qSZ4UekD7IGVeN4AK37l0IrbC5omk?= =?iso-8859-1?Q?sKAeOW5TcePaMGv1KLysdkHguJIjYFzuC3Q3COfOrVP79Kr+Y76rU+f945?= =?iso-8859-1?Q?cg44BW6WDUliMM3DVorhrQQryFOq/MOJi6NAYSGx4AsJDj82VfgTglEdic?= =?iso-8859-1?Q?ogigarJX4U/T20M8v2pyj2Pd2S+YT7c19CJme/VQVEQRko3dy1xQELz5Dj?= =?iso-8859-1?Q?83ED1H0r40j0Ua3gEQhPeCo5pap2PXeeV1yd97lCygiaeQH+kMG9VO2jTD?= =?iso-8859-1?Q?i3RegJhIo/vLA5YmRsrthg=3D?= X-Microsoft-Antispam-Message-Info: LjoWcjjwZticW0WMlzasVmlos2ypznZFIU/8xJwGjlIvNy0Nl6EM8KQnCEiP+eZzLl9ftd9MVOMMflrlZbFA3jWfClz72nnu+kvEyUeSYTHRrOSYaoOh2kJrWW9IQOtD0e41h31Tnjh9JiibTNj5OGQ5OTtIVbMnc6uXPH4OsckWiTIxeVGlqLbCzUxmKgDp X-Microsoft-Exchange-Diagnostics: 1; HE1PR0501MB2041; 6:e+2zjFQGs5iH4yBvCKGibesXx7H2BU7ZUE27gbdyPfcR7ZEHbMZxjVGngBGHLVfh2R1juSHk2u5eeSudLN1/OW/8moO3WySVkpJME3GGNgRowAoSjSxBj1rkx3PrMI53ivM4V3NMGXkla2DiHUo7nqAFTNJblVYh3KK6rrEIiFH69Ypmm2Xl40ncgpayyyTvQN83PrO4bRyn0OExyp22f3EWuxRHkuzEDONcPRdtsFissFooWh1HpavwJ1d++MniLnna6IDYjlECKtGLSImXX/HIm/iDXM3+K6VxE34/pYaiIJ0x7w/92maRO7GRphwb/FKNrh1eUNaB8InBOxySwGQRGC8Z/y/F6u6PB5haZptaojeV+a87X/6fWawxAQRTWryg7qzzUMuMEyPTrxyuqZzX00u9HzwDDkyec40+DSIz68chsppnEByZ1fjMvgS0IjSm6wt8kCUwD9jf7npLQw==; 5:OZcdcg2VQhw/kq/J0x+EAblCpgebuWfKHLHF9zhPWGVTFwuMMcwz8STc593HS+D8hMj40hQslCRHFGEt2qNl4X0bqHdLQ8TQoXs/ZgsJqNzHVPFqideevk54kP5Ygfxb0LTq7VunRZbJ1tqTo4dyoBe0QIRE+11I9tNaQ/2aeu8=; 24:it6UgKaiaEhjoy64h/ucw3cE2jJ06yj7vkS9YCQwGhIJ5jiqxs0xDRU6/xoaxlVgz6JnVZSZgkvbHB0YHYYKNNCnCA5YvVDfPnqOeBujmqQ= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; HE1PR0501MB2041; 7:BymR1aWh9EphDMA4g1pHODfqnM2m46BQsMHWJwBiJNski/wfJi+gROSf4EZMHJ/LdPYU7asJ2haC2D6TDsR8WxZQ4EAqgwzS/GauWnI4KyMCMKa7fGcy0XKalpT5o7RGZM/OfKXLMrE1lvK6bESE6ydIAC8R9DSgxccy70RThZntSevrw1VWwlgLPINJGnZC+c7GXX3X1J6Hu5ikr8JAB8dZbQVjjef8bwsM+7lL769j9dPiO7yIkq+n/PHA68cs X-MS-Office365-Filtering-Correlation-Id: 11549c7f-01b9-4dbc-6264-08d5cbdce02b X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2018 18:39:44.2444 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 11549c7f-01b9-4dbc-6264-08d5cbdce02b X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0501MB2041 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 18:39:48 -0000 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." So, the decision point is whether we want to preserve rte_errno in case of success? My opinion is no. Thanks, Yongseok