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

  From: Elena Agostini <eagostini@nvidia.com>

Signed-off-by: Elena Agostini <eagostini@nvidia.com>
---
 lib/gpudev/gpudev.c     | 10 ++++++++++
 lib/gpudev/rte_gpudev.h |  2 ++
 2 files changed, 12 insertions(+)
  

On Thu, 18 Nov 2021 19:28:02 +0000
<eagostini@nvidia.com> wrote:

> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> index 2b174d8bd5..97575ed979 100644
> --- a/lib/gpudev/gpudev.c
> +++ b/lib/gpudev/gpudev.c
> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
>  		return -rte_errno;
>  	}
>  
> +	if (ptr == NULL) {
> +		rte_errno = EINVAL;
> +		return -rte_errno;
> +	}
> +

The convention for free(), and rte_free() is that calling free
with a NULL pointer is a nop. Why not follow those?

This would keep programmers from having to view GPU as a
special case.
  

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thursday, 18 November 2021 at 17:21
> To: Elena Agostini <eagostini@nvidia.com>
> Cc: dev@dpdk.org <dev@dpdk.org>
> Subject: Re: [PATCH v1] gpudev: return EINVAL if invalid input pointer for free and unregister
> External email: Use caution opening links or attachments>
>
> On Thu, 18 Nov 2021 19:28:02 +0000
> <eagostini@nvidia.com> wrote:>
> > diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> > index 2b174d8bd5..97575ed979 100644
> > --- a/lib/gpudev/gpudev.c
> > +++ b/lib/gpudev/gpudev.c
> > @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> >               return -rte_errno;
> >       }
> >
> > +     if (ptr == NULL) {
> > +             rte_errno = EINVAL;
> > +             return -rte_errno;
> > +     }
> > +>
> The convention for free(), and rte_free() is that calling free
> with a NULL pointer is a nop. Why not follow those?>
> This would keep programmers from having to view GPU as a
> special case.

Please look at v2 here https://patches.dpdk.org/project/dpdk/patch/20211118203354.25355-1-eagostini@nvidia.com/
  

On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
> From: Elena Agostini <eagostini@nvidia.com>
> 
> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> ---
>  lib/gpudev/gpudev.c     | 10 ++++++++++
>  lib/gpudev/rte_gpudev.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> index 2b174d8bd5..97575ed979 100644
> --- a/lib/gpudev/gpudev.c
> +++ b/lib/gpudev/gpudev.c
> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
>  		return -rte_errno;
>  	}
>  
> +	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
  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 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.

thanks!
  

On 11/18/2021 8:19 PM, Tyler Retzlaff wrote:
> On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
>> From: Elena Agostini <eagostini@nvidia.com>
>>
>> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
>> ---
>>   lib/gpudev/gpudev.c     | 10 ++++++++++
>>   lib/gpudev/rte_gpudev.h |  2 ++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
>> index 2b174d8bd5..97575ed979 100644
>> --- a/lib/gpudev/gpudev.c
>> +++ b/lib/gpudev/gpudev.c
>> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
>>   		return -rte_errno;
>>   	}
>>   
>> +	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
>    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.
> 
> thanks!
>
  

19/11/2021 10:34, Ferruh Yigit:
> On 11/18/2021 8:19 PM, Tyler Retzlaff wrote:
> > On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
> >> From: Elena Agostini <eagostini@nvidia.com>
> >>
> >> Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> >> ---
> >>   lib/gpudev/gpudev.c     | 10 ++++++++++
> >>   lib/gpudev/rte_gpudev.h |  2 ++
> >>   2 files changed, 12 insertions(+)
> >>
> >> diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> >> index 2b174d8bd5..97575ed979 100644
> >> --- a/lib/gpudev/gpudev.c
> >> +++ b/lib/gpudev/gpudev.c
> >> @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> >>   		return -rte_errno;
> >>   	}
> >>   
> >> +	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?

> >    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
  

On Fri, Nov 19, 2021 at 09:34:08AM +0000, Ferruh Yigit wrote:
> On 11/18/2021 8:19 PM, Tyler Retzlaff wrote:
> > On Thu, Nov 18, 2021 at 07:28:02PM +0000, eagostini@nvidia.com wrote:
> > > From: Elena Agostini <eagostini@nvidia.com>
> > > 
> > > Signed-off-by: Elena Agostini <eagostini@nvidia.com>
> > > ---
> > >   lib/gpudev/gpudev.c     | 10 ++++++++++
> > >   lib/gpudev/rte_gpudev.h |  2 ++
> > >   2 files changed, 12 insertions(+)
> > > 
> > > diff --git a/lib/gpudev/gpudev.c b/lib/gpudev/gpudev.c
> > > index 2b174d8bd5..97575ed979 100644
> > > --- a/lib/gpudev/gpudev.c
> > > +++ b/lib/gpudev/gpudev.c
> > > @@ -576,6 +576,11 @@ rte_gpu_mem_free(int16_t dev_id, void *ptr)
> > >   		return -rte_errno;
> > >   	}
> > > +	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
> >    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.
>

Yes, it's inconsistent. However, this only applies to functions which
return integer values. Other functions which return pointer values are
hopefully standardized to return NULL on error. [Which means any error code
must be in rte_errno.]

/Bruce
  

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.
  

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
  

On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> 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)

i only urge that this be explicit as opposed to a range i.e. ret == -1
preferred over ret < 0

> 
> becomes for a pointer:
> 
>   if (ret == NULL && rte_errno == EAGAIN)
> 
> Regards,
> /Bruce

but otherwise i agree, ret indicates an error happened and rte_errno
provides the detail.
  

01/12/2021 22:37, Tyler Retzlaff:
> On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> >   if (ret < 0 && rte_errno == EAGAIN)
> 
> i only urge that this be explicit as opposed to a range i.e. ret == -1
> preferred over ret < 0

I don't understand why you think it is important to limit return value to -1.
Why "if (ret == -1)" is better than "if (ret < 0)" ?
  

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 2 December 2021 08.19
> 
> 01/12/2021 22:37, Tyler Retzlaff:
> > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> > >   if (ret < 0 && rte_errno == EAGAIN)
> >
> > i only urge that this be explicit as opposed to a range i.e. ret == -
> 1
> > preferred over ret < 0
> 
> I don't understand why you think it is important to limit return value
> to -1.
> Why "if (ret == -1)" is better than "if (ret < 0)" ?

Speaking for myself:

For clarity. It leaves no doubt that "it failed" is represented by the return value -1, and that the function does not return errno values such as -EINVAL.

-Morten
  

> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, 2 December 2021 08.19
> >
> > 01/12/2021 22:37, Tyler Retzlaff:
> > > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> > > >   if (ret < 0 && rte_errno == EAGAIN)
> > >
> > > i only urge that this be explicit as opposed to a range i.e. ret == -
> > 1
> > > preferred over ret < 0
> >
> > I don't understand why you think it is important to limit return value
> > to -1.
> > Why "if (ret == -1)" is better than "if (ret < 0)" ?
> 
> Speaking for myself:
> 
> For clarity. It leaves no doubt that "it failed" is represented by the return value -1, and that the function does not return errno values such as
> -EINVAL.
> 

But why '< 0' gives you less clarity?
Negative value means failure - seems perfectly clear to me.
  

> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Thursday, 2 December 2021 14.01
> 
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Thursday, 2 December 2021 08.19
> > >
> > > 01/12/2021 22:37, Tyler Retzlaff:
> > > > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> > > > >   if (ret < 0 && rte_errno == EAGAIN)
> > > >
> > > > i only urge that this be explicit as opposed to a range i.e. ret
> == -
> > > 1
> > > > preferred over ret < 0
> > >
> > > I don't understand why you think it is important to limit return
> value
> > > to -1.
> > > Why "if (ret == -1)" is better than "if (ret < 0)" ?
> >
> > Speaking for myself:
> >
> > For clarity. It leaves no doubt that "it failed" is represented by
> the return value -1, and that the function does not return errno values
> such as
> > -EINVAL.
> >
> 
> But why '< 0' gives you less clarity?
> Negative value means failure - seems perfectly clear to me.

I disagree: Negative value does not mean failure. Only -1 means failure.

There is no -2 return value. There is no -EINVAL return value.

Testing for (ret < 0) might confuse someone to think that other values than -1 could be returned as indication of failure, which is not the case when following the convention where the functions set errno and return -1 in case of failure.

It would be different if following a convention where the functions return -errno in case of failure. In this case, testing (ret < 0) would be appropriate.

So explicitly testing (ret == -1) clarifies which of the two conventions are relevant.
  

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Thursday, 2 December 2021 14.56
> 
> > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > Sent: Thursday, 2 December 2021 14.01
> >
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Sent: Thursday, 2 December 2021 08.19
> > > >
> > > > 01/12/2021 22:37, Tyler Retzlaff:
> > > > > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson
> wrote:
> > > > > >   if (ret < 0 && rte_errno == EAGAIN)
> > > > >
> > > > > i only urge that this be explicit as opposed to a range i.e.
> ret
> > == -
> > > > 1
> > > > > preferred over ret < 0
> > > >
> > > > I don't understand why you think it is important to limit return
> > value
> > > > to -1.
> > > > Why "if (ret == -1)" is better than "if (ret < 0)" ?
> > >
> > > Speaking for myself:
> > >
> > > For clarity. It leaves no doubt that "it failed" is represented by
> > the return value -1, and that the function does not return errno
> values
> > such as
> > > -EINVAL.
> > >
> >
> > But why '< 0' gives you less clarity?
> > Negative value means failure - seems perfectly clear to me.
> 
> I disagree: Negative value does not mean failure. Only -1 means
> failure.
> 
> There is no -2 return value. There is no -EINVAL return value.
> 
> Testing for (ret < 0) might confuse someone to think that other values
> than -1 could be returned as indication of failure, which is not the
> case when following the convention where the functions set errno and
> return -1 in case of failure.
> 
> It would be different if following a convention where the functions
> return -errno in case of failure. In this case, testing (ret < 0) would
> be appropriate.
> 
> So explicitly testing (ret == -1) clarifies which of the two
> conventions are relevant.
> 

I tested it on Godbolt, and (ret < 0) produces slightly smaller code than (ret == -1) on x86-64:

https://godbolt.org/z/3xME3jxq8

A binary test (Error or Data) uses 1 byte less, and a tristate test (Error, Zero or Data) uses 3 byte less.

Although there is no measurable performance difference for a single instance of this kind of test, we should consider that this kind of test appears many times in the code, so the saved bytes might add up to something slightly significant in the instruction cache.

My opinion is not so strong anymore... perhaps we should prefer performance over code readability, also in this case?
  

On Thu, Dec 02, 2021 at 01:01:26PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Thursday, 2 December 2021 08.19
> > >
> > > 01/12/2021 22:37, Tyler Retzlaff:
> > > > On Wed, Nov 24, 2021 at 06:04:56PM +0000, Bruce Richardson wrote:
> > > > >   if (ret < 0 && rte_errno == EAGAIN)
> > > >
> > > > i only urge that this be explicit as opposed to a range i.e. ret == -
> > > 1
> > > > preferred over ret < 0
> > >
> > > I don't understand why you think it is important to limit return value
> > > to -1.
> > > Why "if (ret == -1)" is better than "if (ret < 0)" ?
> > 
> > Speaking for myself:
> > 
> > For clarity. It leaves no doubt that "it failed" is represented by the return value -1, and that the function does not return errno values such as
> > -EINVAL.
> > 
> 
> But why '< 0' gives you less clarity?
> Negative value means failure - seems perfectly clear to me.
> 

it's ambiguous and the contract allows it to be. being explicit prevents
it. don't mix your signaling with your info. you simply can't have the
following ever happen if you are explicit.

int somefunc(void)
{
    rte_errno = EPERM;
    return -EINVAL;
}

sure this example you can see is obviously wrong but when you start
dealing with callstacks that are propagating errors N levels down it
gets a lot harder to catch the fact that rte_errno wasn't set to -ret.

also there are many apis right now that do return -1 do you propose it
is suddenly valid to start return -rte_errno? when you do expect this
application code to break.

int somefunc3(void)
{
    rte_errno = EPERM;
    return -1;
}

int somefunc2(void *param)
{
    // some work
    return somefunc3();
}

int rv = somefunc2(param)
if (rv == -1)
    // handle rte_errno
else
    // no error

then we get the foolishness that was recently integrated en masse.

--- before.c    2021-12-08 09:22:10.491248400 -0800
+++ after.c     2021-12-08 09:22:45.859431300 -0800
@@ -1,5 +1,8 @@
 int somefunc2(void *param)
 {
+    if (param == NULL)
+        return -EINVAL;
+
     // some work
     return somefunc3();
 }

compatibility breaks happen when some application writes code in a way
you wouldn't expect. everytime this sort of stuff is done you create an
opportunity for compatibility break.

now you can spend your life writing unit tests that somehow exercise
every error path to make sure someone didn't introduce an inconsistent /
unmatching rte_errno to -ret or you can just stop inter-mixing
signalling with info and get rid of the ambiguity.
  

On Fri, Dec 03, 2021 at 11:37:10AM +0100, Morten Brørup wrote:
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Thursday, 2 December 2021 14.56
> > 
> > 
> > I disagree: Negative value does not mean failure. Only -1 means
> > failure.
> > 
> > There is no -2 return value. There is no -EINVAL return value.
> > 
> > Testing for (ret < 0) might confuse someone to think that other values
> > than -1 could be returned as indication of failure, which is not the
> > case when following the convention where the functions set errno and
> > return -1 in case of failure.
> > 
> > It would be different if following a convention where the functions
> > return -errno in case of failure. In this case, testing (ret < 0) would
> > be appropriate.
> > 
> > So explicitly testing (ret == -1) clarifies which of the two
> > conventions are relevant.
> > 
> 
> I tested it on Godbolt, and (ret < 0) produces slightly smaller code than (ret == -1) on x86-64:
> 
> https://godbolt.org/z/3xME3jxq8
> 
> A binary test (Error or Data) uses 1 byte less, and a tristate test (Error, Zero or Data) uses 3 byte less.
> 
> Although there is no measurable performance difference for a single instance of this kind of test, we should consider that this kind of test appears many times in the code, so the saved bytes might add up to something slightly significant in the instruction cache.
> 
> My opinion is not so strong anymore... perhaps we should prefer performance over code readability, also in this case?
> 

i would not expect many calls that return rte_errno to be made on the
hot path. most of the use of errno / rte_errno is control but it's good
to have considered it. if i start seeing a lot of error handling in hot
paths i ordinarily find a way to get rid of it through various
techniques.
  

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 8 December 2021 18.35
> 
> On Fri, Dec 03, 2021 at 11:37:10AM +0100, Morten Brørup wrote:
> > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > Sent: Thursday, 2 December 2021 14.56
> > >
> > >
> > > I disagree: Negative value does not mean failure. Only -1 means
> > > failure.
> > >
> > > There is no -2 return value. There is no -EINVAL return value.
> > >
> > > Testing for (ret < 0) might confuse someone to think that other
> values
> > > than -1 could be returned as indication of failure, which is not
> the
> > > case when following the convention where the functions set errno
> and
> > > return -1 in case of failure.
> > >
> > > It would be different if following a convention where the functions
> > > return -errno in case of failure. In this case, testing (ret < 0)
> would
> > > be appropriate.
> > >
> > > So explicitly testing (ret == -1) clarifies which of the two
> > > conventions are relevant.
> > >
> >
> > I tested it on Godbolt, and (ret < 0) produces slightly smaller code
> than (ret == -1) on x86-64:
> >
> > https://godbolt.org/z/3xME3jxq8
> >
> > A binary test (Error or Data) uses 1 byte less, and a tristate test
> (Error, Zero or Data) uses 3 byte less.
> >
> > Although there is no measurable performance difference for a single
> instance of this kind of test, we should consider that this kind of
> test appears many times in the code, so the saved bytes might add up to
> something slightly significant in the instruction cache.
> >
> > My opinion is not so strong anymore... perhaps we should prefer
> performance over code readability, also in this case?
> >
> 
> i would not expect many calls that return rte_errno to be made on the
> hot path. most of the use of errno / rte_errno is control but it's good
> to have considered it. if i start seeing a lot of error handling in hot
> paths i ordinarily find a way to get rid of it through various
> techniques.

Tyler, I think you and I agree perfectly on this topic.

-1 should be returned as error, and rte_errno should provide details.

I'm only saying that comparing the return value with < 0 provides marginally less instruction bytes than comparing it with == -1, so even though -1 is the canonical indication of error, the comparison could be < 0 instead of == -1 (if weighing performance higher than code clarity).
  

On Wed, Dec 08, 2021 at 07:40:10PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Wednesday, 8 December 2021 18.35
> > 
> > On Fri, Dec 03, 2021 at 11:37:10AM +0100, Morten Brørup wrote:
> > > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > > Sent: Thursday, 2 December 2021 14.56
> > > >
> > > >
> > > > I disagree: Negative value does not mean failure. Only -1 means
> > > > failure.
> > > >
> > > > There is no -2 return value. There is no -EINVAL return value.
> > > >
> > > > Testing for (ret < 0) might confuse someone to think that other
> > values
> > > > than -1 could be returned as indication of failure, which is not
> > the
> > > > case when following the convention where the functions set errno
> > and
> > > > return -1 in case of failure.
> > > >
> > > > It would be different if following a convention where the functions
> > > > return -errno in case of failure. In this case, testing (ret < 0)
> > would
> > > > be appropriate.
> > > >
> > > > So explicitly testing (ret == -1) clarifies which of the two
> > > > conventions are relevant.
> > > >
> > >
> > > I tested it on Godbolt, and (ret < 0) produces slightly smaller code
> > than (ret == -1) on x86-64:
> > >
> > > https://godbolt.org/z/3xME3jxq8
> > >
> > > A binary test (Error or Data) uses 1 byte less, and a tristate test
> > (Error, Zero or Data) uses 3 byte less.
> > >
> > > Although there is no measurable performance difference for a single
> > instance of this kind of test, we should consider that this kind of
> > test appears many times in the code, so the saved bytes might add up to
> > something slightly significant in the instruction cache.
> > >
> > > My opinion is not so strong anymore... perhaps we should prefer
> > performance over code readability, also in this case?
> > >
> > 
> > i would not expect many calls that return rte_errno to be made on the
> > hot path. most of the use of errno / rte_errno is control but it's good
> > to have considered it. if i start seeing a lot of error handling in hot
> > paths i ordinarily find a way to get rid of it through various
> > techniques.
> 
> Tyler, I think you and I agree perfectly on this topic.
> 
> -1 should be returned as error, and rte_errno should provide details.
> 
> I'm only saying that comparing the return value with < 0 provides marginally less instruction bytes than comparing it with == -1, so even though -1 is the canonical indication of error, the comparison could be < 0 instead of == -1 (if weighing performance higher than code clarity).

sounds about right to me, i appreciate the extra analysis. certainly with
explicit -1 it doesn't stop an application gaining the slightly better
code generation with < 0.

thanks