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 689A6A0546; Fri, 30 Apr 2021 02:15:33 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 316F2410D7; Fri, 30 Apr 2021 02:15:33 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id D62FD40395 for ; Fri, 30 Apr 2021 02:15:31 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 1501420B8006; Thu, 29 Apr 2021 17:15:31 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1501420B8006 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1619741731; bh=Xt8zA2nAUEpHRiF5YiAash4ypqAnA+fFUinC1fGwq94=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=f/fC8MD5b74+a39v23GPhfoyPFvfUiruMh0iGNrPCuSUkoicDfHamCRC2KzHfTYf2 cFJXTfNSSCysqzzVLH4NWHDSeU3AEz34YgqCchfCCGsmMWF/S4D3maWRzNusKywOAk Z+tXxMFxZf50XzUT4yy6qdqKH5S1SOpJt3GMrBOk= Date: Thu, 29 Apr 2021 17:15:31 -0700 From: Tyler Retzlaff To: Dmitry Kozlyuk Cc: Ferruh Yigit , hemant.agrawal@nxp.com, Ajit Khaparde , Jerin Jacob , "Ananyev, Konstantin" , Thomas Monjalon , Andrew Rybchenko , "Min Hu (Connor)" , "dev@dpdk.org" , "olivier.matz@6wind.com" , "david.marchand@redhat.com" , "jerinj@marvell.com" , "Richardson, Bruce" Message-ID: <20210430001531.GA2751@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <6114bde2-423a-da82-ac4d-608141235e39@huawei.com> <1672555.D3d3fyF7jD@thomas> <39bb5d09-9e95-db2d-929f-b0b3e922d921@oss.nxp.com> <68bb19fb-2d1a-677d-05f2-e2029d5095a5@intel.com> <20210429161645.GB21799@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20210429214924.308a636b@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210429214924.308a636b@sovereign> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [dpdk-dev] Questions about API with no parameter check 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 Sender: "dev" On Thu, Apr 29, 2021 at 09:49:24PM +0300, Dmitry Kozlyuk wrote: > 2021-04-29 09:16 (UTC-0700), Tyler Retzlaff: > > On Wed, Apr 07, 2021 at 05:10:00PM +0100, Ferruh Yigit wrote: > > > On 4/7/2021 4:25 PM, Hemant Agrawal wrote: > > > >>+1 > > > >>But are we going to check all parameters? > > > > > > > >+1 > > > > > > > >It may be better to limit the number of checks. > > > > > > > > > > +1 to verify input for APIs. > > > > > > Why not do all, what is the downside of checking all input for control path APIs? > > > > why not assert them then, what is the purpose of returning an error to a > > caller for a api contract violation like a `parameter shall not be NULL` > > > > * assert.h/cassert can be compiled away for those pundits who don't want > > to see extra branches in their code > > > > * when not compiled away it gives you an immediate stack trace or dump to operate > > on immediately identifying the problem instead of having to troll > > through hoaky inconsistently formatted logging. > > > > * it catches callers who don't bother to check for error from return of > > the function (debug builds) instead of some arbitrary failure at some > > unrelated part of the code where the corrupted program state is relied > > upon. > > > > we aren't running in kernel, we can crash. > > As library developers we can't assume stability requirements at call site. > There may be temporary files to clean up, for example, > or other threads in the middle of their work. if a callers state is so incoherent that it is passing NULL to functions that contractually expect non-NULL it is already way past the point of no return. continuing to run only accomplishes destroying the state that might be used to diagnose the originating flaw in program logic. if you return an error instead of fail fast at best you'll crash soon but more often then not you'll keep running and produce incorrect results or worst keep running security compromised. about the only argument that can be made for having this silly error pattern that is valid is when many-party code is running inside the same process and you don't want someone elses bad code taking your process down. a problem that i am accutely aware of in allowing 3rd party code run in kernel space. (but this is mostly? mitigated by multi-process mode). > As an application developer I'd hate to get a crash inside a library and > having to debug it. Usually installed are release versions with assertions > compiled away. > so it wouldn't crash at all at least not at the point of failure. the only difference is i guess you wouldn't get a log message with what is being done now. could we turn this around and have it tunable by policy instead of opting everyone in to this behavior maybe? i'm just making some ideas up on the fly but couldn't we just have something that is compile time policy? #ifdef EAL_FAILURE_POLICY_RETURN #define EAL_FAILURE(condition, error) \ if ((condition)) { \ return (error); \ } #else #define EAL_FAILURE(condition, error) \ assert(! (condition), (error)); #endif also, i'll point out that lately there have been a lot of patches accepted that call functions and don't evaluate their return value and the reason is those functions really should never have been "failable". so we'll just see more of that as we stack on often compile time or immediate runtime failure returns. of course the compatibility of the code calling these functions is only as good as the implicit dependency on the implementation... until it changes and the application misbehaves. i'll also throw another gripe in here that there are a lot of "deallocation" functions in dpdk that according to their api can fail again because of this kind of "oh i'll fail because i got a bad parameter design". deallocation should never fail ever and i shouldn't need to write logic around a deallocation to handle failures. imagine if free failed? p = malloc(...); if (p == NULL) return -1; ... do work with p ... rv = free(p); if (rv != 0) ... what the hell? yet this pattern exists in a bunch of places. it's insane. (i'll quietly ignore the design error that free does accept NULL and is a noop standardized *facepalm*). anyway, i guess i've ranted enough. there are some users who would prefer not to have this but i admit there are an overwhelming number of people who seem to want it.