Re: Potential static analysis ideas

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Hannes,

On Mon, Jul 26, 2021 at 11:08 AM Hannes Reinecke <hare@suse.de> wrote:
> On 7/26/21 10:55 AM, Julia Lawall wrote:
> > On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> >> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >>>> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> >>>>> On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> >>>>>>> To make it work well, you need to know if frob() and/or the current
> >>>>>>> function return an error code or not.  While you can use some heuristics
> >>>>>>> (e.g. is there any return -Exxx), perhaps we can add an annotation to
> >>>>>>> indicate if a function returns an error code, or an error pointer?
> >>>>>>
> >>>>>> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> >>>>>>
> >>>>>> I think it would be useful, if not for the tools, at least for
> >>>>>> developers.
> >>>>>
> >>>>> Agreed.  I added some code to smatch so that I could annotate pointers to
> >>>>> say if they are allowed to be NULL.  The implementation isn't perfect,
> >>>>> but I love having that extra documentation about when I do or don't have
> >>>>> to check for NULL.
> >>>>
> >>>> I can think of four different annotations that limit what a pointer return from
> >>>> a function can be:
> >>>>
> >>>> a) either a valid pointer or NULL, but never an error pointer,
> >>>> b) either a valid pointer or an error pointer, but not NULL,
> >>>> c) always a valid pointer, never NULL or an error,
> >>>> d) always NULL, but callers are expected to check for error pointers.
> >>>
> >>> e) either a valid pointer, NULL, or an error pointer
> >>>
> >>> The last pattern is seen with the various *get*_optional() functions.
> >>
> >> I would always consider those the exact bug that I meant with "because
> >> everyone gets those wrong". I think the idea of the "optional" functions is
> >> that you have two implementations b) and d) and pick one of them
> >> at compile time. To the caller this means either an error pointer or
> >> success, but checking for NULL is a bug in the caller, while conditionally
> >> returning NULL or ERR_PTR() would be a bug in the interface.
> >
> > I'm not sure to understand the "bug in the caller" part.  Couldn't there
> > be two possible definitions of the called function, according to different
> > configuration options, and a single caller that calls both?
> >
> > 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.

For optional resources, the ability to return NULL (and for the caller
not having to care if the check should be for -ENOENT, -ENODEV,
or something else), and accept NULL for further operations greatly
simplifies the handling of optional resources like clocks, enable
gpios, resets, regulators, ...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds