Re: [libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]
On Thu, Jan 10, 2019 at 09:23:06AM -0600, Eric Blake wrote:
> I do wonder if we can enable __attribute__(nonnull) under GCC
and
> then set '-fdelete-null-pointer-checks' to stop it deleting our
> runtime NULL checks, so we get both compile time & runtime protection.
I think that leads to the more confusing situation where we are
documenting something that is not true. If we're going to handle NULL,
then there's no point in telling users not to rely on that.
For any pointer parameter there's then two possibilities. Either NULL
is a valid value and results in some useful behaviour, or NULL is not
valid and libvirt will catch that and report an error.
The latter are essentially all programmer bugs, but we wanted to be robust
and catch them at runtime & report graceful error instead of referencing
NULL or assert()ing. We added ATTRIBUTE_NONNULL widely to try to catch the
latter at compile time too. This also served as documentation in the header
files to indicate that you shouldn't be passing NULL for that parameter.
We later discovered our usage of ATTRIBUTE_NONNULL was based on a
mis-understanding of the functional impact of that annotation, and so
#defined it to a no-op, so it merely served as documentation that you
should not pass NULL to a method.
It is still desirable for us to find a way to do all three tasks
1. document that NULL should not be passed for a param
2. detect mistakes passing NULL via static analysis
3. retain runtime graceful NULL param checking.
AFAICT we have a choice either
a. Replace ATTRIBUTE_NONNULL to just a plain comment /* nonnull */
against each parameter. This covers items 1 & 2, mostly as
we do today, while stopping coverity getting upset
b. Re-enable ATTRIBUTE_NONNULL to expand to attribute(nonnull)
but only under modern GCC, and add -fdelete-null-pointer-checks
This covers items 1, 2 & 3, under GCC, while stopping
coverity getting upset
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 :|