Re: [Libguestfs] [PATCH libnbd v3 0/6] Additional non-NULL warnings, checks and docs
On Wed, Sep 28, 2022 at 06:25:33PM +0100, Richard W.M. Jones wrote:
v2:
https://listman.redhat.com/archives/libguestfs/2022-September/030014.html
I didn't think this would need a v3, but here we are.
The first patch (also a new patch) appears to fix a bug in Eric's
earlier series to do with meta queries. It's not possible to call the
new APIs with queries == NULL, and this becomes obvious when you use
attribute((nonnull)) and enable GCC warnings. I tried to fix this,
but two tests still fail for reasons I'm not clear about:
FAIL: opt-list-meta-queries
FAIL: opt-set-meta-queries
The third patch is also new, and extends attribute((nonnull))
annotations to many internal functions. No actual errors found by
this, but it seems worth it to avoid future problems, assuming that
GCC won't start adding undefined behaviour. I wonder aloud if we
should only enable attribute((nonnull)) for developer builds, ie. tie
it to ./configure --enable-gcc-warnings somehow. (This series does
not do this.)
At this point, I think we're safe. We've disabled the attribute for
the one generated .c file where we really want to prevent the compiler
from being too smart at eliding our intentional NULL checks, as well
as in the testsuite files where we prove that we can trigger the
EFAULT behavior when the attribute is suppressed. Everywhere else,
clang's handling of nonnull is better than gcc such that CI tests
should catch if we start passing NULL where we shouldn't (callers), or
checking for NULL after documenting that we didn't need to
(implementations).
I added comments as suggested by Laszlo to patch 2, and
picked up his R-b's and Acks.
I have some suggestions, but the series is now close enough that you
don't need to post a v4 on my behalf.
Acked-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org