Libguestfs - Jan 2019 - Re: [PATCH nbdkit] include: Annotate function parameters with attribute((nonnull)).

Should we use attribute((nonnull)) at all?  There's a very interesting
history of this in libvirt -- try looking at commit eefb881 plus the
commits referencing eefb881 -- but it does seem to work for me using
recent GCC and Clang.

I only did a few functions because annotating them gets old quickly...

Rich.

To test this change I experimentally modified the file plugin to pass
NULL to nbdkit_realpath, and at least in this simple explicit case
both GCC and Clang give warnings (which turn into errors when using
?./configure --enable-gcc-warnings?).

GCC warns:

file.c: In function ?file_config?:
file.c:86:16: error: null argument where non-null required (argument 1)
[-Werror=nonnull]
     filename = nbdkit_realpath (NULL);
                ^~~~~~~~~~~~~~~

Clang warns:

file.c:86:37: error: null passed to a callee that requires a non-null argument
      [-Werror,-Wnonnull]
    filename = nbdkit_realpath (NULL);
                                ~~~~^

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/nextnonzero.h |  2 +-
 common/include/random.h      |  4 ++--
 common/regions/regions.h     | 19 ++++++++++++-------
 common/sparse/sparse.h       | 12 +++++++++---
 include/nbdkit-common.h      | 15 ++++++++++-----
 common/sparse/sparse.c       | 10 ++++++----
 server/utils.c               |  8 ++++----
 9 files changed, 55 insertions(+), 35 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/nextnonzero.h b/common/include/nextnonzero.h
index 3f96e85..f25000b 100644
--- a/common/include/nextnonzero.h
+++ b/common/include/nextnonzero.h
@@ -45,7 +45,7 @@
  * https://sourceware.org/bugzilla/show_bug.cgi?id=19920
  * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69908
  */
-static inline const char *
+static inline const char * __attribute__((__nonnull__ (1)))
 next_non_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/include/nbdkit-common.h b/include/nbdkit-common.h
index 27f6ed1..e58ca8a 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -64,11 +64,16 @@ 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);
-extern int64_t nbdkit_parse_size (const char *str);
-extern int nbdkit_parse_bool (const char *str);
-extern int nbdkit_read_password (const char *value, char **password);
-extern char *nbdkit_realpath (const char *path);
+extern char *nbdkit_absolute_path (const char *path)
+  __attribute__((__nonnull__ (1)));
+extern int64_t nbdkit_parse_size (const char *str)
+  __attribute__((__nonnull__ (1)));
+extern int nbdkit_parse_bool (const char *str)
+  __attribute__((__nonnull__ (1)));
+extern int nbdkit_read_password (const char *value, char **password)
+  __attribute__((__nonnull__ (1, 2)));
+extern char *nbdkit_realpath (const char *path)
+  __attribute__((__nonnull__ (1)));
 
 /* A static non-NULL pointer which can be used when you don't need a
  * per-connection handle.
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 *
diff --git a/server/utils.c b/server/utils.c
index 18011fd..ceacb39 100644
--- a/server/utils.c
+++ b/server/utils.c
@@ -54,8 +54,8 @@ nbdkit_absolute_path (const char *path)
   CLEANUP_FREE char *pwd = NULL;
   char *ret;
 
-  if (path == NULL || *path == '\0') {
-    nbdkit_error ("cannot convert null or empty path to an absolute
path");
+  if (*path == '\0') {
+    nbdkit_error ("cannot convert empty path to an absolute path");
     return NULL;
   }
 
@@ -261,8 +261,8 @@ nbdkit_realpath (const char *path)
 {
   char *ret;
 
-  if (path == NULL || *path == '\0') {
-    nbdkit_error ("cannot resolve a null or empty path");
+  if (*path == '\0') {
+    nbdkit_error ("realpath: cannot resolve an empty path");
     return NULL;
   }
 
-- 
2.19.2

On 1/1/19 12:20 PM, Richard W.M. Jones wrote:
> Should we use attribute((nonnull)) at all?  There's a very interesting
> history of this in libvirt -- try looking at commit eefb881 plus the
> commits referencing eefb881 -- but it does seem to work for me using
> recent GCC and Clang.
> 
> I only did a few functions because annotating them gets old quickly...
For internal usage, I think the annotations help a working compiler do a
better job. I don't know if modern gcc does better than the old one that
you mention being the reason that libvirt doesn't use it.  I also
haven't researched if libvirt could change their policy now (that is,
are we stuck with avoiding something because of having been burned in
the past, even though we would no longer be burned by it?).

For installed headers, where the annotations are used by clients
compiling a plugin, we need to be a bit more careful - while nbdkit
itself requires compilation with gcc or clang (for things like our
automatic variable cleanups) and therefore has unconditional support for
attributes, I think we should still aim for a generic plugin being
compiled with a standards-compliant compiler rather than requiring
clients to use just gcc or clang.  It's not impossible to make the use
of attributes in a public header be properly guarded by the
preprocessor, as long as we remember to do it right.

-- 
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/bc8f4714/attachment.sig>

On Wed, Jan 02, 2019 at 08:48:08AM -0600, Eric Blake
wrote:
> On 1/1/19 12:20 PM, Richard W.M. Jones wrote:
> > Should we use attribute((nonnull)) at all?  There's a very
interesting
> > history of this in libvirt -- try looking at commit eefb881 plus the
> > commits referencing eefb881 -- but it does seem to work for me using
> > recent GCC and Clang.
> > 
> > I only did a few functions because annotating them gets old quickly...
> 
> For internal usage, I think the annotations help a working compiler do a
> better job. I don't know if modern gcc does better than the old one
that
> you mention being the reason that libvirt doesn't use it.  I also
> haven't researched if libvirt could change their policy now (that is,
> are we stuck with avoiding something because of having been burned in
> the past, even though we would no longer be burned by it?).
> 
> For installed headers, where the annotations are used by clients
> compiling a plugin, we need to be a bit more careful - while nbdkit
> itself requires compilation with gcc or clang (for things like our
> automatic variable cleanups) and therefore has unconditional support for
> attributes, I think we should still aim for a generic plugin being
> compiled with a standards-compliant compiler rather than requiring
> clients to use just gcc or clang.  It's not impossible to make the use
> of attributes in a public header be properly guarded by the
> preprocessor, as long as we remember to do it right.
OK good points, let me come back with a v2 of this patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v