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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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.