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 AF7B7A0C52; Wed, 24 Nov 2021 18:24:44 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6CA8440040; Wed, 24 Nov 2021 18:24:44 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id EF2294003C; Wed, 24 Nov 2021 18:24:42 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 40DB220CEAB5; Wed, 24 Nov 2021 09:24:42 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 40DB220CEAB5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1637774682; bh=PmY0TqoRHLdBZV+lybcTkKE+VUq8RIf0ecyNPh90ii4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=I3yYjy7b5zKuhSbeaU3dJXJGVOmpXXfvfMSxt11+TTFplD5T+RwidoElzt6JELwf1 wjmMtJNsjxRbRRjC+Verj/hoJOUtF1z2UylpoKMITTifxK6NbZ6KX0bqdzchJEzpQp keXVjgIe9lT3tlc7hr1ldkCfAHzq/v0bxujtI+xk= Date: Wed, 24 Nov 2021 09:24:42 -0800 From: Tyler Retzlaff To: Thomas Monjalon Cc: eagostini@nvidia.com, techboard@dpdk.org, dev@dpdk.org, Andrew Rybchenko , David Marchand , Ferruh Yigit Subject: Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister Message-ID: <20211124172442.GA9129@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20211118192802.23955-1-eagostini@nvidia.com> <20211118201931.GA6492@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <4933677.M48bl325bv@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4933677.M48bl325bv@thomas> 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 Fri, Nov 19, 2021 at 10:56:36AM +0100, Thomas Monjalon wrote: > 19/11/2021 10:34, Ferruh Yigit: > > >> + if (ptr == NULL) { > > >> + rte_errno = EINVAL; > > >> + return -rte_errno; > > >> + } > > > > > > in general dpdk has real problems with how it indicates that an error > > > occurred and what error occurred consistently. > > > > > > some api's return 0 on success > > > and maybe return -errno if ! 0 > > > and maybe return errno if ! 0 > > Which function returns a positive errno? i may have mispoke about this variant, it may be something i recall seeing in a posted patch that was resolved before integration. > > > > and maybe set rte_errno if ! 0 > > > > > > some api's return -1 on failure > > > and set rte_errno if -1 > > > > > > some api's return < 0 on failure > > > and maybe set rte_errno > > > and maybe return -errno > > > and maybe set rte_errno and return -rte_errno > > > > This is a generic comment, cc'ed a few more folks to make the comment more > > visible. > > > > > this isn't isiolated to only this change but since additions and context > > > in this patch highlight it maybe it's a good time to bring it up. > > > > > > it's frustrating to have to carefully read the implementation every time > > > you want to make a function call to make sure you're handling the flavor > > > of error reporting for a particular function. > > > > > > if this is new code could we please clearly identify the current best > > > practice and follow it as a standard going forward for all new public > > > apis. > > I think this patch is following the best practice. > 1/ Return negative value in case of error > 2/ Set rte_errno > 3/ Set same absolute value in rte_errno and return code with the approach proposed as best practice above it results in at least the applicaiton code variations as follows. int rv = rte_func_call(); 1. if (rv < 0 && rte_errno == EAGAIN) 2. if (rv == -1 && rte_errno == EAGAIN) 3. if (rv < 0 && -rv == EAGAIN) 4. if (rv < 0 && rv == -EAGAIN) (and incorrectly) 5. // ignore rv if (rte_errno == EAGAIN) it might be better practice if indication that an error occurs is signaled distinctly from the error that occurred. otherwise why use rte_errno at all instead returning -rte_errno always? this philosophy would align better with modern posix / unix platform apis. often documented in the RETURN VALUE section of the manpage as: ``Upon successful completion, somefunction() shall return 0; otherwise, -1 shall be returned and errno set to indicate the error.'' therefore returning a value outside of the set {0, -1} is an abi break. separately i have misgivings about how many patches have been integrated and in some instances backported to dpdk stable that have resulted in new return values and / or set new values to rte_errno outside of the set of values initially possible when the dpdk release was made.