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.
