Re: [Qemu-devel] [RFC 00/15] Error API: Flag errors in *errp even if errors are being ignored

[Daniel P. Berrange]

On Wed, Jun 28, 2017 at 02:41:58PM -0300, Eduardo Habkost wrote:
> On Wed, Jun 28, 2017 at 11:05:26AM +0200, Markus Armbruster wrote:

> > > Ensuring errp is never NULL
> > > ---------------------------
> > >
> > > The last patch on this series changes the (Error **errp)
> > > parameters in functions to (Error *errp[static 1]), just to help
> > > validate the existing code, as clang warns about NULL arguments
> > > on that case.  I don't think we should apply that patch, though,
> > > because the "[static 1]" syntax confuses Coccinelle.
> > >
> > > I have a branch where I experimented with the idea of replacing
> > > (Error **errp) parameters with an opaque type (void*, or a struct
> > > type).  I gave up when I noticed it would require touching all
> > > callers to replace &err with a wrapper macro to convert to the
> > > right type.  Suggestions to make NULL errp easier to detect at
> > > build time are welcome.
> > >
> > > (Probably the easiest solution for that is to add assert(errp)
> > > lines to the ERR_IS_*() macros.)
> > 
> > We'll obviously struggle with null arguments until all the developers
> > adjusted to the new interface.  Possibly with occasional mistakes
> > forever.  Compile-time checking would really, really help.
> 
> True.  I'm investigating the possibility of using
> __attribute__((nonull(...))) with Coccinelle's help.

Beware that '__attribute__((nonnull))' has two distinct effects,
one of which is a potentially nasty trap which leads to crashes....

The useful part is that it allows compilers & analysis tools
like coverity to warn if you accidentally pass NULL into
a method. These warnings, particularly from gcc, only catch
a fraction of scenarios where you pass NULL in though.

The less useful part is that if GCC sees a nonnull annotation
on a parameter, then in the body of the method, it will silently
remove any code which does  "if (!paramname)". So if you added
a check for the parameter being NULL to avoid a crash, gcc will
remove that protection, so you'll once again get a crash at
runtime if passing NULL.

So if you use the nonnull annotation, they you probably want
to make sure to pass  -fno-delete-null-pointer-checks to
GCC to stop it removing your protection code, or you need to
be very confident that nothing will mistakenly pass NULL into
the methods annotated nonnull.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|