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 9B8E4A0A02; Wed, 5 May 2021 17:58:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 606A740143; Wed, 5 May 2021 17:58:25 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 451F040040 for ; Wed, 5 May 2021 17:58:23 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 5933320B7178; Wed, 5 May 2021 08:58:22 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 5933320B7178 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1620230302; bh=+Lxr67GHrgmSou1BaEh9ddmM735XH4L1djV0R1TUOrs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Oe1V9/ZsA1Mn8dlBeQemFLV29Y360nh2P8EMKvhltO7lOy1d4lAM3rrdpXR9GmWv1 TLlpZ29t07AjzRfUztQcMN7VWxGgDaxTdcBp9UgdjL2FxB5BBdROxk0Xp88x0Wi0Gu cao7QIoDsUT/YSgN2RD7RzNjqlieSbCeTYsFDOnA= Date: Wed, 5 May 2021 08:58:22 -0700 From: Tyler Retzlaff To: "Ananyev, Konstantin" Cc: Dmitry Kozlyuk , "Yigit, Ferruh" , "hemant.agrawal@nxp.com" , Ajit Khaparde , Jerin Jacob , 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: <20210505155822.GA13051@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: 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 Tue, May 04, 2021 at 09:36:24AM +0000, Ananyev, Konstantin 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. > > > > 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. > > I agree with Dmitry summary above. > Asserting inside the library calls is bad programming practice, > please keep it away from the project. i'm not advocating for asserts i'm advocating for users to have a choice instead of being opted in to this change unconditionally. asserts are an option that may be policy controlled as previously mentioned either in this thread or another. so if you don't like them you can disable them as a function of the policy. for a basic assert that means building release instead of debug but a more sophisticated policy mechanism could be employed if desired. what you can't turn off is introduction of superfluous errors being returned due to programming mistakes in the application which should be handled yet have no sensible way to be handled. it just clutters the calling code with unnecessary error handling, makes the errors returned ambiguious and often indistinguishable from real errors. by this logic we should modify rte_free to be int rte_free(void * p) { if (p == NULL) return EINVAL; mem_free(p, true); } which is about as useful as one can imagine. this proposal has been pushed through too quickly without proper debate, and the patch that introduces the superfluous errors breaks abi. tech board should get involved before it goes further. i'm not asking for asserts, i'm asking not to be opted in to an equally harmful error handling pattern that makes application logic more error prone and more complex negatively impacting quality. thanks