[libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]

[Eric Blake]

On 1/10/19 9:39 AM, Daniel P. Berrangé wrote:

> 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.

But within the latter are two cases: libvirt catches the error
gracefully, or libvirt crashes.

> 
> 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

Or go with two separate macros:
One for use on internal files, where when we mark something non-NULL, it
is a programming bug in libvirt for the caller for passing NULL, or a
programming bug in the implementation for using the attribute and then
checking for NULL after all. Let gcc/coverity always flag these
violations as bugs, and don't bother worrying about safety nets because
we trust our callers of internal interfaces for knowing the semantics.

The other for use in public headers, where the public gets good
documentation that passing NULL is nonsense, and even gets the attribute
enabled if they compile with gcc in order to help them avoid bad code.
But when compiling internally, we disable the macro (similarly to how
our headers use VIR_ENUM_SENTINELS to control whether the VIR_*_LAST
enum members are present for internal use but absent for the public);
that way, our implementations don't see the attribute and can code in
the safety-net runtime check to handle bad users that disobeyed the
warning, without crashing and without our NULL checks being optimized out.

I'm still not sold that we want -fdelete-null-pointer-checks, but on the
other hand, how much overhead does it add?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190110/5300ec32/attachment-0001.sig>