Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister

From: Bruce Richardson <bruce.richardson@intel.com>
To: Tyler Retzlaff <roretzla@linux.microsoft.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
	eagostini@nvidia.com, techboard@dpdk.org, dev@dpdk.org,
	Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
	David Marchand <david.marchand@redhat.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
Date: Wed, 24 Nov 2021 18:04:56 +0000	[thread overview]
Message-ID: <YZ5+yAwjgf6B57Np@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20211124172442.GA9129@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>

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