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:

quoted

On Fri, Nov 19, 2021 at 10:56:36AM +0100, Thomas Monjalon wrote:

quoted

19/11/2021 10:34, Ferruh Yigit:

quoted

quoted

quoted

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

quoted

quoted

quoted

   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.

quoted

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.