From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 65027A00C2; Wed, 8 Dec 2021 18:27:52 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EC9514111B; Wed, 8 Dec 2021 18:27:51 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 6F552410F3; Wed, 8 Dec 2021 18:27:49 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 83D8020B7179; Wed, 8 Dec 2021 09:27:48 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 83D8020B7179 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1638984468; bh=VegFr69o5ZrD+aTm0srjj/X2lYGrPXD/PJwCwE1Wt7I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=G/zK8/gDAze1kkfBKcNSfs5wWHv9LaQP9/SnMu+pjV9rslUVGcvryiDyep+iQwOhw ZQNhsogZLW9FMiK2ahDyrbR6OwizPbuF6fSU9CAc6X/qqZNbqamvHJqqpJuaiE2zap 38KkfM6aw/XNw1vHOGQjfDb101MSFE696d93Y1Bg= Date: Wed, 8 Dec 2021 09:27:48 -0800 From: Tyler Retzlaff To: "Ananyev, Konstantin" Cc: Morten =?iso-8859-1?Q?Br=F8rup?= , Thomas Monjalon , "Richardson, Bruce" , "techboard@dpdk.org" , "dev@dpdk.org" , Andrew Rybchenko , David Marchand , "Yigit, Ferruh" Subject: Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister Message-ID: <20211208172748.GA17852@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20211118192802.23955-1-eagostini@nvidia.com> <20211201213749.GA5097@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <3625450.hdfAi7Kttb@thomas> <98CBD80474FA8B44BF855DF32C47DC35D86D33@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, Dec 02, 2021 at 01:01:26PM +0000, Ananyev, Konstantin wrote: > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > Sent: Thursday, 2 December 2021 08.19 > > > > > > 01/12/2021 22:37, Tyler Retzlaff: > > > > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote: > > > > > if (ret < 0 && rte_errno == EAGAIN) > > > > > > > > i only urge that this be explicit as opposed to a range i.e. ret == - > > > 1 > > > > preferred over ret < 0 > > > > > > I don't understand why you think it is important to limit return value > > > to -1. > > > Why "if (ret == -1)" is better than "if (ret < 0)" ? > > > > Speaking for myself: > > > > For clarity. It leaves no doubt that "it failed" is represented by the return value -1, and that the function does not return errno values such as > > -EINVAL. > > > > But why '< 0' gives you less clarity? > Negative value means failure - seems perfectly clear to me. > it's ambiguous and the contract allows it to be. being explicit prevents it. don't mix your signaling with your info. you simply can't have the following ever happen if you are explicit. int somefunc(void) { rte_errno = EPERM; return -EINVAL; } sure this example you can see is obviously wrong but when you start dealing with callstacks that are propagating errors N levels down it gets a lot harder to catch the fact that rte_errno wasn't set to -ret. also there are many apis right now that do return -1 do you propose it is suddenly valid to start return -rte_errno? when you do expect this application code to break. int somefunc3(void) { rte_errno = EPERM; return -1; } int somefunc2(void *param) { // some work return somefunc3(); } int rv = somefunc2(param) if (rv == -1) // handle rte_errno else // no error then we get the foolishness that was recently integrated en masse. --- before.c 2021-12-08 09:22:10.491248400 -0800 +++ after.c 2021-12-08 09:22:45.859431300 -0800 @@ -1,5 +1,8 @@ int somefunc2(void *param) { + if (param == NULL) + return -EINVAL; + // some work return somefunc3(); } compatibility breaks happen when some application writes code in a way you wouldn't expect. everytime this sort of stuff is done you create an opportunity for compatibility break. now you can spend your life writing unit tests that somehow exercise every error path to make sure someone didn't introduce an inconsistent / unmatching rte_errno to -ret or you can just stop inter-mixing signalling with info and get rid of the ambiguity.