Libguestfs - Jan 2019 - Re: [PATCH nbdkit v2 1/2] Annotate internal function parameters with attribute((nonnull)).

v1 was here:
https://www.redhat.com/archives/libguestfs/2019-January/msg00008.html

In v2 I have provided two patches:

The first patch extends attribute((nonnull)) to most internal
functions, but not to the external API.

The second patch uses a macro so that attribute((format)) is only used
in the public API on GCC or Clang.  At least in theory these headers
could be used by a C compiler which only supports the C99 standard.

Note that I don't propose to use attribute((nonnull)) in the public
API until we have a bit more knowledge about: (1) Which version of GCC
fixed this.  (2) Whether it really works well with new GCC or if we
find there are unexpected problems.

Rich.

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
---
 common/bitmap/bitmap.h              | 18 ++++---
 common/include/iszero.h             |  2 +-
 common/include/random.h             |  4 +-
 common/regions/regions.h            | 19 ++++---
 common/sparse/sparse.h              | 12 +++--
 filters/cache/blk.h                 | 17 ++++--
 filters/cow/blk.h                   |  7 ++-
 filters/xz/blkcache.h               | 12 +++--
 plugins/floppy/virtual-floppy.h     | 21 +++++---
 plugins/partitioning/efi-crc32.h    |  3 +-
 plugins/partitioning/virtual-disk.h | 11 ++--
 plugins/sh/call.h                   | 10 ++--
 server/internal.h                   | 81 +++++++++++++++++++++--------
 tests/test.h                        |  3 +-
 tests/web-server.h                  |  3 +-
 common/sparse/sparse.c              | 10 ++--
 16 files changed, 160 insertions(+), 73 deletions(-)

diff --git a/common/bitmap/bitmap.h b/common/bitmap/bitmap.h
index 0da650a..553b9d2 100644
--- a/common/bitmap/bitmap.h
+++ b/common/bitmap/bitmap.h
@@ -65,7 +65,7 @@ struct bitmap {
   size_t size;                  /* Size of bitmap in bytes. */
 };
 
-static inline void
+static inline void __attribute__((__nonnull__ (1)))
 bitmap_init (struct bitmap *bm, unsigned blksize, unsigned bpb)
 {
   assert (is_power_of_2 (blksize));
@@ -99,10 +99,11 @@ bitmap_free (struct bitmap *bm)
 /* Resize the bitmap to the virtual disk size in bytes.
  * Returns -1 on error, setting nbdkit_error.
  */
-extern int bitmap_resize (struct bitmap *bm, uint64_t new_size);
+extern int bitmap_resize (struct bitmap *bm, uint64_t new_size)
+  __attribute__((__nonnull__ (1)));
 
 /* Clear the bitmap (set everything to zero). */
-static inline void
+static inline void __attribute__((__nonnull__ (1)))
 bitmap_clear (struct bitmap *bm)
 {
   memset (bm->bitmap, 0, bm->size);
@@ -125,7 +126,7 @@ bitmap_clear (struct bitmap *bm)
 /* Return the bit(s) associated with the given block.
  * If the request is out of range, returns the default value.
  */
-static inline unsigned
+static inline unsigned __attribute__((__nonnull__ (1)))
 bitmap_get_blk (const struct bitmap *bm, uint64_t blk, unsigned default_)
 {
   BITMAP_OFFSET_BIT_MASK (bm, blk);
@@ -139,7 +140,7 @@ bitmap_get_blk (const struct bitmap *bm, uint64_t blk,
unsigned default_)
 }
 
 /* As above but works with virtual disk offset in bytes. */
-static inline unsigned
+static inline unsigned __attribute__((__nonnull__ (1)))
 bitmap_get (const struct bitmap *bm, uint64_t offset, unsigned default_)
 {
   return bitmap_get_blk (bm, offset / bm->blksize, default_);
@@ -148,7 +149,7 @@ bitmap_get (const struct bitmap *bm, uint64_t offset,
unsigned default_)
 /* Set the bit(s) associated with the given block.
  * If out of range, it is ignored.
  */
-static inline void
+static inline void __attribute__((__nonnull__ (1)))
 bitmap_set_blk (const struct bitmap *bm, uint64_t blk, unsigned v)
 {
   BITMAP_OFFSET_BIT_MASK (bm, blk);
@@ -163,7 +164,7 @@ bitmap_set_blk (const struct bitmap *bm, uint64_t blk,
unsigned v)
 }
 
 /* As above bit works with virtual disk offset in bytes. */
-static inline void
+static inline void __attribute__((__nonnull__ (1)))
 bitmap_set (const struct bitmap *bm, uint64_t offset, unsigned v)
 {
   return bitmap_set_blk (bm, offset / bm->blksize, v);
@@ -177,6 +178,7 @@ bitmap_set (const struct bitmap *bm, uint64_t offset,
unsigned v)
  * Returns -1 if the bitmap is all zeroes from blk to the end of the
  * bitmap.
  */
-extern int64_t bitmap_next (const struct bitmap *bm, uint64_t blk);
+extern int64_t bitmap_next (const struct bitmap *bm, uint64_t blk)
+  __attribute__((__nonnull__ (1)));
 
 #endif /* NBDKIT_BITMAP_H */
diff --git a/common/include/iszero.h b/common/include/iszero.h
index 7a54f0a..efe0762 100644
--- a/common/include/iszero.h
+++ b/common/include/iszero.h
@@ -46,7 +46,7 @@
  * See also:
  * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69908
  */
-static inline bool
+static inline bool __attribute__((__nonnull__ (1)))
 is_zero (const char *buffer, size_t size)
 {
   size_t i;
diff --git a/common/include/random.h b/common/include/random.h
index eb08295..098c6d6 100644
--- a/common/include/random.h
+++ b/common/include/random.h
@@ -67,7 +67,7 @@ snext (uint64_t *seed)
 }
 
 /* Seed the random state from a 64 bit seed. */
-static inline void
+static inline void  __attribute__((__nonnull__ (2)))
 xsrandom (uint64_t seed, struct random_state *state)
 {
   state->s[0] = snext (&seed);
@@ -83,7 +83,7 @@ rotl (const uint64_t x, int k)
 }
 
 /* Returns 64 random bits.  Updates the state. */
-static inline uint64_t
+static inline uint64_t __attribute__((__nonnull__ (1)))
 xrandom (struct random_state *state)
 {
   const uint64_t result_starstar = rotl (state->s[1] * 5, 7) * 9;
diff --git a/common/regions/regions.h b/common/regions/regions.h
index ca9b3d5..4fcaf09 100644
--- a/common/regions/regions.h
+++ b/common/regions/regions.h
@@ -76,25 +76,30 @@ struct regions {
   size_t nr_regions;
 };
 
-extern void init_regions (struct regions *regions);
-extern void free_regions (struct regions *regions);
-extern const struct region *find_region (const struct regions *regions,
uint64_t offset);
-extern int append_region (struct regions *regions, struct region region);
+extern void init_regions (struct regions *regions)
+  __attribute__((__nonnull__ (1)));
+extern void free_regions (struct regions *regions)
+  __attribute__((__nonnull__ (1)));
+extern const struct region *find_region (const struct regions *regions,
+                                         uint64_t offset)
+  __attribute__((__nonnull__ (1)));
+extern int append_region (struct regions *regions, struct region region)
+  __attribute__((__nonnull__ (1)));
 
-static inline const struct region *
+static inline const struct region * __attribute__((__nonnull__ (1)))
 get_region (const struct regions *regions, size_t i)
 {
   assert (i < regions->nr_regions);
   return &regions->regions[i];
 }
 
-static inline size_t
+static inline size_t __attribute__((__nonnull__ (1)))
 nr_regions (struct regions *regions)
 {
   return regions->nr_regions;
 }
 
-static inline int64_t
+static inline int64_t __attribute__((__nonnull__ (1)))
 virtual_size (struct regions *regions)
 {
   if (regions->nr_regions == 0)
diff --git a/common/sparse/sparse.h b/common/sparse/sparse.h
index 3acb0af..818d804 100644
--- a/common/sparse/sparse.h
+++ b/common/sparse/sparse.h
@@ -64,12 +64,16 @@ extern void free_sparse_array (struct sparse_array *sa);
 /* Read bytes from the sparse array.
  * Note this can never return an error and never allocates.
  */
-extern void sparse_array_read (struct sparse_array *sa, void *buf, uint32_t
count, uint64_t offset);
+extern void sparse_array_read (struct sparse_array *sa, void *buf,
+                               uint32_t count, uint64_t offset)
+  __attribute__((__nonnull__ (1, 2)));
 
 /* Write bytes to the sparse array.
  * This can allocate and can return an error.
  */
-extern int sparse_array_write (struct sparse_array *sa, const void *buf,
uint32_t count, uint64_t offset);
+extern int sparse_array_write (struct sparse_array *sa, const void *buf,
+                               uint32_t count, uint64_t offset)
+  __attribute__((__nonnull__ (1, 2)));
 
 /* Zero byte range in the sparse array.
  *
@@ -79,6 +83,8 @@ extern int sparse_array_write (struct sparse_array *sa, const
void *buf, uint32_
  *
  * This may free memory, but never returns an error.
  */
-extern void sparse_array_zero (struct sparse_array *sa, uint32_t count,
uint64_t offset);
+extern void sparse_array_zero (struct sparse_array *sa,
+                               uint32_t count, uint64_t offset)
+  __attribute__((__nonnull__ (1)));
 
 #endif /* NBDKIT_SPARSE_H */
diff --git a/filters/cache/blk.h b/filters/cache/blk.h
index ab9134e..2a65bdb 100644
--- a/filters/cache/blk.h
+++ b/filters/cache/blk.h
@@ -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)));
 
 /* 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)));
 
 /* Write a whole block.
  *
@@ -64,10 +69,14 @@ extern int blk_writethrough (struct nbdkit_next_ops
*next_ops, void *nxdata, uin
  *
  * Otherwise it will only write to the cache.
  */
-extern int blk_write (struct nbdkit_next_ops *next_ops, void *nxdata, uint64_t
blknum, const uint8_t *block, uint32_t flags, int *err);
+extern int blk_write (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)));
 
 /* Iterates over each dirty block in the cache. */
 typedef int (*block_callback) (uint64_t blknum, void *vp);
-extern int for_each_dirty_block (block_callback f, void *vp);
+extern int for_each_dirty_block (block_callback f, void *vp)
+  __attribute__((__nonnull__ (1)));
 
 #endif /* NBDKIT_BLK_H */
diff --git a/filters/cow/blk.h b/filters/cow/blk.h
index 2829a89..2279133 100644
--- a/filters/cow/blk.h
+++ b/filters/cow/blk.h
@@ -56,10 +56,13 @@ extern void blk_free (void);
 extern int blk_set_size (uint64_t new_size);
 
 /* Read a single block from the overlay 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)));
 
 /* Write a single block. */
-extern int blk_write (uint64_t blknum, const uint8_t *block, int *err);
+extern int blk_write (uint64_t blknum, const uint8_t *block, int *err)
+  __attribute__((__nonnull__ (2, 3)));
 
 /* Flush the overlay to disk. */
 extern int blk_flush (void);
diff --git a/filters/xz/blkcache.h b/filters/xz/blkcache.h
index e68f5d1..c86c8e8 100644
--- a/filters/xz/blkcache.h
+++ b/filters/xz/blkcache.h
@@ -42,9 +42,13 @@ typedef struct blkcache_stats {
 } blkcache_stats;
 
 extern blkcache *new_blkcache (size_t maxdepth);
-extern void free_blkcache (blkcache *);
-extern char *get_block (blkcache *, uint64_t offset, uint64_t *start, uint64_t
*size);
-extern int put_block (blkcache *, uint64_t start, uint64_t size, char *data);
-extern void blkcache_get_stats (blkcache *, blkcache_stats *ret);
+extern void free_blkcache (blkcache *) __attribute__((__nonnull__ (1)));
+extern char *get_block (blkcache *, uint64_t offset,
+                        uint64_t *start, uint64_t *size)
+  __attribute__((__nonnull__ (1, 3, 4)));
+extern int put_block (blkcache *, uint64_t start, uint64_t size, char *data)
+  __attribute__((__nonnull__ (1, 4)));
+extern void blkcache_get_stats (blkcache *, blkcache_stats *ret)
+  __attribute__((__nonnull__ (1, 2)));
 
 #endif /* NBDKIT_XZFILE_H */
diff --git a/plugins/floppy/virtual-floppy.h b/plugins/floppy/virtual-floppy.h
index 5a50c62..d9111b0 100644
--- a/plugins/floppy/virtual-floppy.h
+++ b/plugins/floppy/virtual-floppy.h
@@ -223,11 +223,20 @@ struct virtual_floppy {
 #define SECTORS_PER_CLUSTER 32
 #define CLUSTER_SIZE (SECTOR_SIZE * SECTORS_PER_CLUSTER)
 
-extern void init_virtual_floppy (struct virtual_floppy *floppy);
-extern int create_virtual_floppy (const char *dir, const char *label, struct
virtual_floppy *floppy);
-extern void free_virtual_floppy (struct virtual_floppy *floppy);
-extern int create_directory (size_t di, const char *label, struct
virtual_floppy *floppy);
-extern int update_directory_first_cluster (size_t di, struct virtual_floppy
*floppy);
-extern void pad_string (const char *label, size_t n, uint8_t *out);
+extern void init_virtual_floppy (struct virtual_floppy *floppy)
+  __attribute__((__nonnull__ (1)));
+extern int create_virtual_floppy (const char *dir, const char *label,
+                                  struct virtual_floppy *floppy)
+  __attribute__((__nonnull__ (1, 2, 3)));
+extern void free_virtual_floppy (struct virtual_floppy *floppy)
+  __attribute__((__nonnull__ (1)));
+extern int create_directory (size_t di, const char *label,
+                             struct virtual_floppy *floppy)
+  __attribute__((__nonnull__ (2, 3)));
+extern int update_directory_first_cluster (size_t di,
+                                           struct virtual_floppy *floppy)
+  __attribute__((__nonnull__ (2)));
+extern void pad_string (const char *label, size_t n, uint8_t *out)
+  __attribute__((__nonnull__ (1, 3)));
 
 #endif /* NBDKIT_VIRTUAL_FLOPPY_H */
diff --git a/plugins/partitioning/efi-crc32.h b/plugins/partitioning/efi-crc32.h
index 76f5a6a..21e28d7 100644
--- a/plugins/partitioning/efi-crc32.h
+++ b/plugins/partitioning/efi-crc32.h
@@ -36,6 +36,7 @@
 
 #include <stdint.h>
 
-extern uint32_t efi_crc32 (const void *buf, size_t len);
+extern uint32_t efi_crc32 (const void *buf, size_t len)
+  __attribute__((__nonnull__ (1)));
 
 #endif /* NBDKIT_EFI_CRC32_H */
diff --git a/plugins/partitioning/virtual-disk.h
b/plugins/partitioning/virtual-disk.h
index da846f5..3860f46 100644
--- a/plugins/partitioning/virtual-disk.h
+++ b/plugins/partitioning/virtual-disk.h
@@ -111,14 +111,19 @@ extern int create_virtual_disk_layout (void);
 /* Parse a GPT GUID.  Note that GPT GUIDs have peculiar
  * characteristics which make them unlike general GUIDs.
  */
-extern int parse_guid (const char *str, char *out);
+extern int parse_guid (const char *str, char *out)
+  __attribute__((__nonnull__ (1, 2)));
 
 /* Internal functions for creating MBR and GPT layouts.  These are
  * published here because the GPT code calls into the MBR code, but
  * are not meant to be called from the main plugin.
  */
-extern void create_mbr_partition_table (unsigned char *out);
-extern void create_mbr_partition_table_entry (const struct region *, int
bootable, int partition_id, unsigned char *);
+extern void create_mbr_partition_table (unsigned char *out)
+  __attribute__((__nonnull__ (1)));
+extern void create_mbr_partition_table_entry (const struct region *,
+                                              int bootable, int partition_id,
+                                              unsigned char *)
+  __attribute__((__nonnull__ (1, 4)));
 extern void create_gpt_layout (void);
 
 #endif /* NBDKIT_VIRTUAL_DISK_H */
diff --git a/plugins/sh/call.h b/plugins/sh/call.h
index 204a404..b5916b0 100644
--- a/plugins/sh/call.h
+++ b/plugins/sh/call.h
@@ -42,9 +42,13 @@ typedef enum exit_code {
   RET_FALSE = 3        /* script exited with code 3 meaning false */
 } exit_code;
 
-extern exit_code call (const char **argv);
-extern exit_code call_read (char **rbuf, size_t *rbuflen, const char **argv);
-extern exit_code call_write (const char *wbuf, size_t wbuflen, const char
**argv);
+extern exit_code call (const char **argv)
+  __attribute__((__nonnull__ (1)));
+extern exit_code call_read (char **rbuf, size_t *rbuflen, const char **argv)
+  __attribute__((__nonnull__ (1, 2, 3)));
+extern exit_code call_write (const char *wbuf, size_t wbuflen,
+                             const char **argv)
+  __attribute__((__nonnull__ (1, 3)));
 
 extern char tmpdir[];
 extern char *script;
diff --git a/server/internal.h b/server/internal.h
index 68c9636..a89b41c 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -125,24 +125,44 @@ extern void cleanup_unlock (pthread_mutex_t **ptr);
 
 /* connections.c */
 struct connection;
-typedef int (*connection_recv_function) (struct connection *, void *buf, size_t
len);
-typedef int (*connection_send_function) (struct connection *, const void *buf,
size_t len);
-typedef void (*connection_close_function) (struct connection *);
+typedef int (*connection_recv_function) (struct connection *,
+                                         void *buf, size_t len)
+  __attribute__((__nonnull__ (1, 2)));
+typedef int (*connection_send_function) (struct connection *,
+                                         const void *buf, size_t len)
+  __attribute__((__nonnull__ (1, 2)));
+typedef void (*connection_close_function) (struct connection *)
+  __attribute__((__nonnull__ (1)));
 extern int handle_single_connection (int sockin, int sockout);
-extern int connection_set_handle (struct connection *conn, size_t i, void
*handle);
-extern void *connection_get_handle (struct connection *conn, size_t i);
-extern pthread_mutex_t *connection_get_request_lock (struct connection *conn);
-extern void connection_set_crypto_session (struct connection *conn, void
*session);
-extern void *connection_get_crypto_session (struct connection *conn);
-extern void connection_set_recv (struct connection *,
connection_recv_function);
-extern void connection_set_send (struct connection *,
connection_send_function);
-extern void connection_set_close (struct connection *,
connection_close_function);
+extern int connection_set_handle (struct connection *conn,
+                                  size_t i, void *handle)
+  __attribute__((__nonnull__ (1 /* not 3 */)));
+extern void *connection_get_handle (struct connection *conn, size_t i)
+  __attribute__((__nonnull__ (1)));
+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 */)));
+extern void connection_set_recv (struct connection *,
+                                 connection_recv_function)
+  __attribute__((__nonnull__ (1, 2)));
+extern void connection_set_send (struct connection *,
+                                 connection_send_function)
+  __attribute__((__nonnull__ (1, 2)));
+extern void connection_set_close (struct connection *,
+                                  connection_close_function)
+  __attribute__((__nonnull__ (1, 2)));
 
 /* crypto.c */
 #define root_tls_certificates_dir sysconfdir "/pki/" PACKAGE_NAME
 extern void crypto_init (bool tls_set_on_cli);
 extern void crypto_free (void);
-extern int crypto_negotiate_tls (struct connection *conn, int sockin, int
sockout);
+extern int crypto_negotiate_tls (struct connection *conn,
+                                 int sockin, int sockout)
+  __attribute__((__nonnull__ (1)));
 
 /* debug.c */
 #define debug nbdkit_debug
@@ -206,33 +226,48 @@ struct backend {
 };
 
 /* plugins.c */
-extern struct backend *plugin_register (size_t index, const char *filename,
void *dl, struct nbdkit_plugin *(*plugin_init) (void));
-extern void set_debug_flags (void *dl, const char *name);
+extern struct backend *plugin_register (size_t index, const char *filename,
+                                        void *dl, struct nbdkit_plugin
*(*plugin_init) (void))
+  __attribute__((__nonnull__ (2, 3, 4)));
+extern void set_debug_flags (void *dl, const char *name)
+  __attribute__((__nonnull__ (1, 2)));
 
 /* filters.c */
-extern struct backend *filter_register (struct backend *next, size_t index,
const char *filename, void *dl, struct nbdkit_filter *(*filter_init) (void));
+extern struct backend *filter_register (struct backend *next, size_t index,
+                                        const char *filename, void *dl,
+                                        struct nbdkit_filter *(*filter_init)
(void))
+  __attribute__((__nonnull__ (1, 3, 4, 5)));
 
 /* locks.c */
 extern void lock_init_thread_model (void);
 extern void lock_connection (void);
 extern void unlock_connection (void);
-extern void lock_request (struct connection *conn);
-extern void unlock_request (struct connection *conn);
+extern void lock_request (struct connection *conn)
+  __attribute__((__nonnull__ (1)));
+extern void unlock_request (struct connection *conn)
+  __attribute__((__nonnull__ (1)));
 extern void lock_unload (void);
 extern void unlock_unload (void);
 
 /* sockets.c */
-extern int *bind_unix_socket (size_t *);
-extern int *bind_tcpip_socket (size_t *);
-extern void accept_incoming_connections (int *socks, size_t nr_socks);
-extern void free_listening_sockets (int *socks, size_t nr_socks);
+extern int *bind_unix_socket (size_t *)
+  __attribute__((__nonnull__ (1)));
+extern int *bind_tcpip_socket (size_t *)
+  __attribute__((__nonnull__ (1)));
+extern void accept_incoming_connections (int *socks, size_t nr_socks)
+  __attribute__((__nonnull__ (1)));
+extern void free_listening_sockets (int *socks, size_t nr_socks)
+  __attribute__((__nonnull__ (1)));
 
 /* threadlocal.c */
 extern void threadlocal_init (void);
 extern void threadlocal_new_server_thread (void);
-extern void threadlocal_set_name (const char *name);
+extern void threadlocal_set_name (const char *name)
+  __attribute__((__nonnull__ (1)));
 extern void threadlocal_set_instance_num (size_t instance_num);
-extern void threadlocal_set_sockaddr (const struct sockaddr *addr, socklen_t
addrlen);
+extern void threadlocal_set_sockaddr (const struct sockaddr *addr,
+                                      socklen_t addrlen)
+  __attribute__((__nonnull__ (1)));
 extern const char *threadlocal_get_name (void);
 extern size_t threadlocal_get_instance_num (void);
 extern void threadlocal_set_error (int err);
diff --git a/tests/test.h b/tests/test.h
index dae3afc..cf0b1eb 100644
--- a/tests/test.h
+++ 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)));
 
 /* Declare program_name. */
 #if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME == 1
diff --git a/tests/web-server.h b/tests/web-server.h
index ac022b4..77faf6f 100644
--- a/tests/web-server.h
+++ b/tests/web-server.h
@@ -55,6 +55,7 @@
  * cleaned up automatically on exit.  Note that the returned string
  * must NOT be freed by the main program.
  */
-extern const char *web_server (const char *filename);
+extern const char *web_server (const char *filename)
+  __attribute__((__nonnull__ (1)));
 
 #endif /* NBDKIT_WEB_SERVER_H */
diff --git a/common/sparse/sparse.c b/common/sparse/sparse.c
index be4eb2c..a5ace48 100644
--- a/common/sparse/sparse.c
+++ b/common/sparse/sparse.c
@@ -123,10 +123,12 @@ free_sparse_array (struct sparse_array *sa)
 {
   size_t i;
 
-  for (i = 0; i < sa->l1_size; ++i)
-    free_l2_dir (sa->l1_dir[i].l2_dir);
-  free (sa->l1_dir);
-  free (sa);
+  if (sa) {
+    for (i = 0; i < sa->l1_size; ++i)
+      free_l2_dir (sa->l1_dir[i].l2_dir);
+    free (sa->l1_dir);
+    free (sa);
+  }
 }
 
 struct sparse_array *
-- 
2.19.2

Allow [in theory at least - it's not tested] the public API header
files to be consumed by a compiler which doesn't support
attribute((format)), a non-standard C extension.

Note that we still require GCC or Clang for compiling nbdkit.
---
 include/nbdkit-common.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 27f6ed1..36fce20 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -45,6 +45,13 @@
 extern "C" {
 #endif
 
+#if defined(__GNUC__) || defined(__clang__)
+#define ATTRIBUTE_FORMAT_PRINTF(fmtpos, argpos) \
+  __attribute__((__format__ (__printf__, fmtpos, argpos)))
+#else
+#define ATTRIBUTE_FORMAT_PRINTF(fmtpos, argpos)
+#endif
+
 #define NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS     0
 #define NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS    1
 #define NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS        2
@@ -57,11 +64,9 @@ extern "C" {
 #define NBDKIT_FUA_EMULATE    1
 #define NBDKIT_FUA_NATIVE     2
 
-extern void nbdkit_error (const char *msg, ...)
-  __attribute__((__format__ (__printf__, 1, 2)));
+extern void nbdkit_error (const char *msg, ...) ATTRIBUTE_FORMAT_PRINTF (1, 2);
 extern void nbdkit_verror (const char *msg, va_list args);
-extern void nbdkit_debug (const char *msg, ...)
-  __attribute__((__format__ (__printf__, 1, 2)));
+extern void nbdkit_debug (const char *msg, ...) ATTRIBUTE_FORMAT_PRINTF (1, 2);
 extern void nbdkit_vdebug (const char *msg, va_list args);
 
 extern char *nbdkit_absolute_path (const char *path);
-- 
2.19.2

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

On 1/2/19 2:14 PM, Richard W.M. Jones wrote:
> Allow [in theory at least - it's not tested] the public API header
> files to be consumed by a compiler which doesn't support
> attribute((format)), a non-standard C extension.
> 
> Note that we still require GCC or Clang for compiling nbdkit.
> ---
>  include/nbdkit-common.h | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
Looks good.

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