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 9C21BA0C52; Wed, 24 Nov 2021 19:05:06 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 27CD740E78; Wed, 24 Nov 2021 19:05:06 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id 0BC244003C; Wed, 24 Nov 2021 19:05:03 +0100 (CET) X-IronPort-AV: E=McAfee;i="6200,9189,10178"; a="216046475" X-IronPort-AV: E=Sophos;i="5.87,261,1631602800"; d="scan'208";a="216046475" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Nov 2021 10:05:02 -0800 X-IronPort-AV: E=Sophos;i="5.87,261,1631602800"; d="scan'208";a="509972483" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.21.34]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 24 Nov 2021 10:05:00 -0800 Date: Wed, 24 Nov 2021 18:04:56 +0000 From: Bruce Richardson To: Tyler Retzlaff Cc: Thomas Monjalon , 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: References: <20211118192802.23955-1-eagostini@nvidia.com> <20211118201931.GA6492@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <4933677.M48bl325bv@thomas> <20211124172442.GA9129@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211124172442.GA9129@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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 Wed, Nov 24, 2021 at 09:24:42AM -0800, Tyler Retzlaff wrote: > 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. I like using this standard, because it also allows consistent behaviour for non-integer returning functions, e.g. object creation functions returning pointers. if (ret < 0 && rte_errno == EAGAIN) becomes for a pointer: if (ret == NULL && rte_errno == EAGAIN) Regards, /Bruce