Re: [libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]
Adding Rich, since he recently raised the topic on the libguestfs list
and pointed to libvirt as precedence:
On 12/18/18 9:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
> Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair
> to perform NULL argument checking; however, no corresponding change
> to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the
> Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
We have historically still added nonnull annotations even when
having checks for NULL in the impl. We had to explicitly disabble
the nonnull annotation when building under GCC to prevent it from
optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS
so that coverity would still report if any callers passed a NULL into
the methods.
Looking at the gcc bug and following some of the links mentioned therein,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308
https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01103.html
It looks like modern gcc has added -Wnonnull-compare that noisily
informs us any time we have an ATTRIBUTE_NONNULL mismatch with the body
of a function comparing that parameter against NULL after all. And
gnulib's manywarnings module turns that on automatically (at least for
gcc 8.2.1 on Fedora 29).
That is sufficient to fix the bug that led us to historically disable
ATTRIBUTE_NONNULL under gcc (commit eefb881), so maybe it's time to just
blindly enable ATTRIBUTE_NONNULL everywhere on the grounds that we have
enough developers and CI coverage to now immediately catch inconsistent
attributes rather than risking silently mis-compiled code that omits the
branch after a NULL comparison as dead code.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 488 bytes)