Re: Potential static analysis ideas

[Julia Lawall <julia.lawall@inria.fr>]

[-- Attachment #1: Type: text/plain, Size: 2641 bytes --]



On Mon, 26 Jul 2021, Hannes Reinecke wrote:

> On 7/26/21 11:28 AM, Julia Lawall wrote:
> > > > > Also, over 230 files contain functions that return both NULL and
> > > > > ERR_PTR.
> > > > > A random example, chosen for conciseness, is the following from
> > > > > fs/overlayfs/inode.c:
> > > > >
> > > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry
> > > > > *real,
> > > > >                                  bool is_upper)
> > > > > {
> > > > >        struct inode *inode, *key = d_inode(real);
> > > > >
> > > > >           inode = ilookup5(sb, (unsigned long) key, ovl_inode_test,
> > > > > key);
> > > > >        if (!inode)
> > > > >                   return NULL;
> > > > >
> > > > >           if (!ovl_verify_inode(inode, is_upper ? NULL : real,
> > > > >                                 is_upper ? real : NULL, false)) {
> > > > >                   iput(inode);
> > > > >                   return ERR_PTR(-ESTALE);
> > > > >           }
> > > > >
> > > > >           return inode;
> > > > > }
> > > > >
> > > > And that I would consider a coding error.
> > > > If a function is able to return an error pointer it should _always_
> > > > return an error pointer; here it would be trivial to return -ENXIO
> > > > instead of NULL in the first condition.
> > > >
> > > > Not doing so is just sloppy programming IMO.
> > >
> > > In this case I agree.
> >
> > I looked at the calling context, and it is not so obvious.  There are
> > different behaviors in the two cases at both callsites.  It is like what
> > Dan describes.  If the thing is not available, we just move on.  If it is
> > available some action is needed.  If there is an actual error, some error
> > handling is needed.
> >
> But isn't 'not available' an error, too?

No, it doesn't seem to be.

> And isn't that exactly why we have individual errnos?
>
> Why do we have to introduce different classes of errors (errno, NULL pointer),
> when we could have used a simple errno lik -ENXIO?
> And, of course, documentation for that function outlining what exactly the
> meaning of the individual error numbers is.
> But then we would need that anyway to clarify how the caller should interpret
> a 'NULL' return value.
>
> So: not convinced :-)

OK, I basically agree.  The point is just that there are going to be two
tests at both call sites either way.

julia

> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                Kernel Storage Architect
> hare@suse.de                              +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
>