[Libguestfs] [PATCH nbdkit v2 1/2] Annotate internal function parameters with attribute((nonnull)).

[Eric Blake]

On 1/2/19 2:14 PM, Richard W.M. Jones wrote:
> Annotate some function parameters with attribute((nonnull)).  Only do
> this for internal headers where we are sure that we will be using
> sufficiently recent GCC or Clang.  For the public header files
> (ie. include/nbdkit-*.h) it may be that people building out of tree
> plugins are using old GCC which had problems, or even other compilers
> that don't support this extension at all.
> 
> Libvirt has an interesting history with this attribute.  In 2012 use
> of the attribute was disabled when compiling under GCC (but kept for
> Coverity) because of poor behaviour of GCC.  This is still the case
> for libvirt upstream.  However the bug in GCC does appear to have been
> fixed at the end of 2016.
> 
> https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=eefb881d4683d50882b43e5b28b0e94657cd0c9c
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308
> ---

> +++ b/filters/cache/blk.h

This file is not on the current nbdkit.git master; I'm assuming the
patch depends on some of your other patches landing first.

> @@ -51,10 +51,15 @@ extern void blk_free (void);
>  extern int blk_set_size (uint64_t new_size);
>  
>  /* Read a single block from the cache or plugin. */
> -extern int blk_read (struct nbdkit_next_ops *next_ops, void *nxdata, uint64_t blknum, uint8_t *block, int *err);
> +extern int blk_read (struct nbdkit_next_ops *next_ops, void *nxdata,
> +                     uint64_t blknum, uint8_t *block, int *err)
> +  __attribute__((__nonnull__ (1, 2, 4, 5)));

nxdata is opaque - which means blk_read() shouldn't be dereferencing it.
It may happen to be true that struct backend is set up such that nxdata
is currently always non-NULL, but that's an implementation detail, and
we are violating layering if we enforce backend behavior by insisting on
the opaque parameter having or not having any particular value, rather
than just blindly passing it on through and letting the backend do the
enforcing.  I would use just __nonnull__ (1, 4, 5).

>  
>  /* Write to the cache and the plugin. */
> -extern int blk_writethrough (struct nbdkit_next_ops *next_ops, void *nxdata, uint64_t blknum, const uint8_t *block, uint32_t flags, int *err);
> +extern int blk_writethrough (struct nbdkit_next_ops *next_ops, void *nxdata,
> +                             uint64_t blknum, const uint8_t *block,
> +                             uint32_t flags, int *err)
> +  __attribute__((__nonnull__ (1, 2, 4, 6)));

Same comment; probably repeats elsewhere in the patch.

> +++ b/server/internal.h

> +extern pthread_mutex_t *connection_get_request_lock (struct connection *conn)
> +  __attribute__((__nonnull__ (1)));
> +extern void connection_set_crypto_session (struct connection *conn,
> +                                           void *session)
> +  __attribute__((__nonnull__ (1 /* not 2 */)));
> +extern void *connection_get_crypto_session (struct connection *conn)
> +  __attribute__((__nonnull__ (1 /* not 2 */)));

Spurious comment here (but correct use of not decorating parameters in
earlier functions that are opaque).

> +++ b/tests/test.h
> @@ -44,7 +44,8 @@ extern const char *sock;        /* socket of most recent nbdkit process */
>  extern const char *server[2];   /* server parameter for add_drive */
>  
>  /* Can be called more than once (useful for nbd plugin) */
> -extern int test_start_nbdkit (const char *arg, ...);
> +extern int test_start_nbdkit (const char *arg, ...)
> +  __attribute__((__nonnull__ (1)));

Independent fix: this should probably also have
__attribute__((__sentinel__))

All the other changes look sane.

-- 
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/libguestfs/attachments/20190102/94d3496e/attachment.sig>