Libguestfs - Sep 2022 - [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters

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.)

I added comments as suggested by Laszlo to patch 2, and
picked up his R-b's and Acks.

Rich.

StringList parameters (char ** in C) will be marked with
__attribute__((nonnull)).  To pass an empty list you have to use a
list containing a single NULL element, not a NULL pointer.

nbd_internal_set_querylist has also been adjusted.

Fixes: commit e33762a86cd5f064e5ef841520baa5fe7361d2c2
---
 generator/states-newstyle-opt-meta-context.c |  4 +++-
 lib/opt.c                                    | 16 ++++++++++++----
 lib/utils.c                                  |  4 ++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/generator/states-newstyle-opt-meta-context.c
b/generator/states-newstyle-opt-meta-context.c
index 46fee15be1..a60aa66f3b 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -65,12 +65,14 @@  NEWSTYLE.OPT_META_CONTEXT.START:
     }
   }
   if (opt != h->opt_current) {
+    char *empty[] = { NULL };
+
     if (!h->request_meta || !h->structured_replies ||
         h->request_meta_contexts.len == 0) {
       SET_NEXT_STATE (%^OPT_GO.START);
       return 0;
     }
-    if (nbd_internal_set_querylist (h, NULL) == -1) {
+    if (nbd_internal_set_querylist (h, empty) == -1) {
       SET_NEXT_STATE (%.DEAD);
       return 0;
     }
diff --git a/lib/opt.c b/lib/opt.c
index 1b18920fdb..e323d7b1b0 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -224,7 +224,9 @@ int
 nbd_unlocked_opt_list_meta_context (struct nbd_handle *h,
                                     nbd_context_callback *context)
 {
-  return nbd_unlocked_opt_list_meta_context_queries (h, NULL, context);
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_opt_list_meta_context_queries (h, empty, context);
 }
 
 /* Issue NBD_OPT_LIST_META_CONTEXT and wait for the reply. */
@@ -255,7 +257,9 @@ int
 nbd_unlocked_opt_set_meta_context (struct nbd_handle *h,
                                    nbd_context_callback *context)
 {
-  return nbd_unlocked_opt_set_meta_context_queries (h, NULL, context);
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_opt_set_meta_context_queries (h, empty, context);
 }
 
 /* Issue NBD_OPT_SET_META_CONTEXT and wait for the reply. */
@@ -371,7 +375,9 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle
*h,
                                         nbd_context_callback *context,
                                         nbd_completion_callback *complete)
 {
-  return nbd_unlocked_aio_opt_list_meta_context_queries (h, NULL, context,
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_aio_opt_list_meta_context_queries (h, empty, context,
                                                          complete);
 }
 
@@ -407,7 +413,9 @@ nbd_unlocked_aio_opt_set_meta_context (struct nbd_handle *h,
                                        nbd_context_callback *context,
                                        nbd_completion_callback *complete)
 {
-  return nbd_unlocked_aio_opt_set_meta_context_queries (h, NULL, context,
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_aio_opt_set_meta_context_queries (h, empty, context,
                                                         complete);
 }
 
diff --git a/lib/utils.c b/lib/utils.c
index c1d633fea1..b4043aa3bc 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -100,7 +100,7 @@ nbd_internal_set_querylist (struct nbd_handle *h, char
**queries)
 {
   size_t i;
 
-  if (queries) {
+  if (queries[0] != NULL /* non-empty list */) {
     if (nbd_internal_copy_string_list (&h->querylist, queries) == -1) {
       set_error (errno, "realloc");
       return -1;
@@ -109,7 +109,7 @@ nbd_internal_set_querylist (struct nbd_handle *h, char
**queries)
     assert (h->querylist.len > 0);
     string_vector_remove (&h->querylist, h->querylist.len - 1);
   }
-  else {
+  else /* empty list */ {
     string_vector_reset (&h->querylist);
     for (i = 0; i < h->request_meta_contexts.len; ++i) {
       char *copy = strdup (h->request_meta_contexts.ptr[i]);
-- 
2.37.0.rc2

For API parameters that are pointers and must not be NULL, add the
appropriate GCC annotations.

Reviewed-by: Laszlo Ersek <lersek at redhat.com>
---
 generator/C.ml              | 59 +++++++++++++++++++++++++++++++++++--
 tests/errors-connect-null.c |  4 +++
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index 66498a7d88..6e4538fd34 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -107,6 +107,28 @@ let
   | UInt64 n -> [n]
   | UIntPtr n -> [n]
 
+(* Map function arguments to __attribute__((nonnull)) annotations.
+ * Because a single arg may map to multiple C parameters this
+ * returns a list.  Note: true => add attribute nonnull.
+ *)
+let arg_attr_nonnull +  function
+  (* BytesIn/Out are passed using a non-null pointer, and size_t *)
+  | BytesIn _ | BytesOut _
+  | BytesPersistIn _ | BytesPersistOut _ -> [ true; false ]
+  (* sockaddr is also non-null pointer, and length *)
+  | SockAddrAndLen (n, len) -> [ true; false ]
+  (* strings should be marked as non-null *)
+  | Path _ | String _ -> [ true ]
+  (* list of strings should be marked as non-null *)
+  | StringList n -> [ true ]
+  (* numeric and other non-pointer types are not able to be null *)
+  | Bool _ | Closure _ | Enum _ | Fd _ | Flags _
+  | Int _ | Int64 _ | SizeT _
+  | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ]
+
+let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ]
+
 let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
           ?closure_style args optargs    if parens then pr "(";
@@ -216,7 +238,21 @@ let
 let print_fndecl ?wrap ?closure_style name args optargs ret    pr "extern
";
   print_call ?wrap ?closure_style name args optargs ret;
-  pr ";\n"
+
+  (* Output __attribute__((nonnull)) for the function parameters:
+   * eg. struct nbd_handle *, int, char *
+   *     => [ true, false, true ]
+   *     => LIBNBD_ATTRIBUTE_NONNULL((1,3))
+   *     => __attribute__((nonnull,(1,3)))
+   *)
+  let nns : bool list +    [ true ] (* struct nbd_handle * *)
+    @ List.flatten (List.map arg_attr_nonnull args)
+    @ List.flatten (List.map optarg_attr_nonnull optargs) in
+  let nns = List.mapi (fun i b -> (i+1, b)) nns in
+  let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in
+  let nns : string list = List.map string_of_int nns in
+  pr "\n    LIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat
"," nns)
 
 let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
           cbargs @@ -349,6 +385,19 @@ let
   pr "extern \"C\" {\n";
   pr "#endif\n";
   pr "\n";
+  pr "#if defined(__GNUC__)\n";
+  pr "#define LIBNBD_GCC_VERSION \\\n";
+  pr "    (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 +
__GNUC_PATCHLEVEL__)\n";
+  pr "#endif\n";
+  pr "\n";
+  pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
+  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /*
gcc >= 12.0 */\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__
s))\n";
+  pr "#else\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
+  pr "#endif\n";
+  pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
+  pr "\n";
   pr "struct nbd_handle;\n";
   pr "\n";
   List.iter (
@@ -382,7 +431,7 @@ let
   pr "extern struct nbd_handle *nbd_create (void);\n";
   pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
   pr "\n";
-  pr "extern void nbd_close (struct nbd_handle *h);\n";
+  pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL
*/\n";
   pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n";
   pr "\n";
   pr "extern const char *nbd_get_error (void);\n";
@@ -770,6 +819,12 @@ let
   pr "\n";
   pr "#include <pthread.h>\n";
   pr "\n";
+  pr "/* GCC will remove NULL checks from this file for any
parameter\n";
+  pr " * annotated with attribute((nonnull)).  See RHBZ#1041336. 
To\n";
+  pr " * avoid this, disable the attribute when including
libnbd.h.\n";
+  pr " */\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
+  pr "\n";
   pr "#include \"libnbd.h\"\n";
   pr "#include \"internal.h\"\n";
   pr "\n";
diff --git a/tests/errors-connect-null.c b/tests/errors-connect-null.c
index 27b21f0f8f..b975181afb 100644
--- a/tests/errors-connect-null.c
+++ b/tests/errors-connect-null.c
@@ -27,6 +27,10 @@
 #include <string.h>
 #include <errno.h>
 
+/* GCC will warn that we are passing NULL (or worse), so to do this
+ * test we must remove the header file attribute.
+ */
+#define LIBNBD_ATTRIBUTE_NONNULL(s)
 #include <libnbd.h>
 
 static char *progname;
-- 
2.37.0.rc2

---
 lib/internal.h | 75 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 25 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index 7896b1d5ca..fb15ada1c3 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -384,20 +384,27 @@ struct command {
   } while (0)
 
 /* aio.c */
-extern void nbd_internal_retire_and_free_command (struct command *);
+extern void nbd_internal_retire_and_free_command (struct command *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* connect.c */
-extern int nbd_internal_wait_until_connected (struct nbd_handle *h);
+extern int nbd_internal_wait_until_connected (struct nbd_handle *h)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* crypto.c */
-extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *,
struct socket *oldsock);
-extern bool nbd_internal_crypto_is_reading (struct nbd_handle *);
-extern int nbd_internal_crypto_handshake (struct nbd_handle *);
-extern void nbd_internal_crypto_debug_tls_enabled (struct nbd_handle *);
+extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *,
struct socket *oldsock)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
+extern bool nbd_internal_crypto_is_reading (struct nbd_handle *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
+extern int nbd_internal_crypto_handshake (struct nbd_handle *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
+extern void nbd_internal_crypto_debug_tls_enabled (struct nbd_handle *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* debug.c */
 extern void nbd_internal_debug (struct nbd_handle *h, const char *context,
-                                const char *fs, ...);
+                                const char *fs, ...)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 3));
 #define debug(h, fs, ...)                                   \
   do {                                                      \
     if_debug ((h))                                          \
@@ -410,9 +417,11 @@ extern void nbd_internal_debug (struct nbd_handle *h, const
char *context,
   } while (0)
 
 /* errors.c */
-extern void nbd_internal_set_error_context (const char *context);
+extern void nbd_internal_set_error_context (const char *context)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern const char *nbd_internal_get_error_context (void);
-extern void nbd_internal_set_last_error (int errnum, char *error);
+extern void nbd_internal_set_last_error (int errnum, char *error)
+  LIBNBD_ATTRIBUTE_NONNULL((2));
 #define set_error(errnum, fs, ...)                                      \
   do {                                                                  \
     int _e = (errnum);                                                  \
@@ -430,12 +439,15 @@ extern void nbd_internal_set_last_error (int errnum, char
*error);
   } while (0)
 
 /* flags.c */
-extern void nbd_internal_reset_size_and_flags (struct nbd_handle *h);
+extern void nbd_internal_reset_size_and_flags (struct nbd_handle *h)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern int nbd_internal_set_size_and_flags (struct nbd_handle *h,
                                             uint64_t exportsize,
-                                            uint16_t eflags);
+                                            uint16_t eflags)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern int nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min,
-                                        uint32_t pref, uint32_t max);
+                                        uint32_t pref, uint32_t max)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* is-state.c */
 extern bool nbd_internal_is_state_created (enum state state);
@@ -447,7 +459,8 @@ extern bool nbd_internal_is_state_dead (enum state state);
 extern bool nbd_internal_is_state_closed (enum state state);
 
 /* opt.c */
-extern void nbd_internal_free_option (struct nbd_handle *h);
+extern void nbd_internal_free_option (struct nbd_handle *h)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* protocol.c */
 extern int nbd_internal_errno_of_nbd_error (uint32_t error);
@@ -458,15 +471,18 @@ extern int64_t nbd_internal_command_common (struct
nbd_handle *h,
                                             uint16_t flags, uint16_t type,
                                             uint64_t offset, uint64_t count,
                                             int count_err, void *data,
-                                            struct command_cb *cb);
+                                            struct command_cb *cb)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* socket.c */
 struct socket *nbd_internal_socket_create (int fd);
 
 /* states.c */
 extern void nbd_internal_abort_commands (struct nbd_handle *h,
-                                         struct command **list);
-extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev);
+                                         struct command **list)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
+extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern const char *nbd_internal_state_short_string (enum state state);
 extern enum state_group nbd_internal_state_group (enum state state);
 extern enum state_group nbd_internal_state_group_parent (enum state_group
group);
@@ -477,14 +493,22 @@ extern int nbd_internal_aio_get_direction (enum state
state);
 #define get_public_state(h) ((h)->public_state)
 
 /* utils.c */
-extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp);
-extern int nbd_internal_copy_string_list (string_vector *v, char **in);
-extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv);
-extern int nbd_internal_set_querylist (struct nbd_handle *h, char **queries);
-extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len);
-extern void nbd_internal_fork_safe_perror (const char *s);
-extern char *nbd_internal_printable_buffer (const void *buf, size_t count);
-extern char *nbd_internal_printable_string (const char *str);
+extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 3));
+extern int nbd_internal_copy_string_list (string_vector *v, char **in)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
+extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
+extern int nbd_internal_set_querylist (struct nbd_handle *h, char **queries)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
+extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len)
+  LIBNBD_ATTRIBUTE_NONNULL((2));
+extern void nbd_internal_fork_safe_perror (const char *s)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
+extern char *nbd_internal_printable_buffer (const void *buf, size_t count)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
+extern char *nbd_internal_printable_string (const char *str)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern char *nbd_internal_printable_string_list (char **list);
 
 /* These are wrappers around socket(2) and socketpair(2).  They
@@ -494,6 +518,7 @@ extern char *nbd_internal_printable_string_list (char
**list);
 extern int nbd_internal_socket (int domain, int type, int protocol,
                                 bool nonblock);
 extern int nbd_internal_socketpair (int domain, int type, int protocol,
-                                    int *fds);
+                                    int *fds)
+  LIBNBD_ATTRIBUTE_NONNULL((4));
 
 #endif /* LIBNBD_INTERNAL_H */
-- 
2.37.0.rc2

We previously checked only that String parameters are not NULL,
returning an error + EFAULT if so.

However we did not check Bytes*, SockAddrAndLen, Path or StringList
parameters, also never NULL.  Be consistent about checks.

Thanks: Eric Blake for help and an earlier version of the patch
---
 generator/API.ml            | 7 +++++--
 generator/C.ml              | 7 ++++++-
 tests/errors-connect-null.c | 2 +-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml
index 6e3213ad26..69849102cf 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -3851,13 +3851,16 @@ let ()  
     (* !may_set_error is incompatible with certain parameters because
      * we have to do a NULL-check on those which may return an error.
+     * Refer to generator/C.ml:generator_lib_api_c.
      *)
     | name, { args; may_set_error = false }
          when List.exists
                 (function
-                 | Closure _ | Enum _ | Flags _ | String _ -> true
+                 | Closure _ | Enum _ | Flags _
+                 | BytesIn _ | BytesOut _ | BytesPersistIn _ | BytesPersistOut
_
+                 | SockAddrAndLen _ | Path _ | String _ | StringList _->
true
                  | _ -> false) args ->
-       failwithf "%s: if args contains Closure/Enum/Flags/String
parameter, may_set_error must be false" name
+       failwithf "%s: if args contains any non-null pointer parameter,
may_set_error must be false" name
 
     (* !may_set_error is incompatible with certain optargs too.
      *)
diff --git a/generator/C.ml b/generator/C.ml
index 6e4538fd34..2f22ad1e59 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -620,7 +620,12 @@ let
          need_out_label := true
       | Flags (n, flags) ->
          print_flags_check n flags None
-      | String n ->
+      | BytesIn (n, _) | BytesOut (n, _)
+      | BytesPersistIn (n, _) | BytesPersistOut (n, _)
+      | SockAddrAndLen (n, _)
+      | Path n
+      | String n
+      | StringList n ->
          let value = match errcode with
            | Some value -> value
            | None -> assert false in
diff --git a/tests/errors-connect-null.c b/tests/errors-connect-null.c
index b975181afb..5313c4c394 100644
--- a/tests/errors-connect-null.c
+++ b/tests/errors-connect-null.c
@@ -78,7 +78,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
   /* FIXME: If we add nonnull attributes, this might change to EFAULT */
-  check (EINVAL, "nbd_connect_command: ");
+  check (EFAULT, "nbd_connect_command: ");
 
   if (nbd_connect_command (nbd, (char **) cmd) != -1) {
     fprintf (stderr, "%s: test failed: "
-- 
2.37.0.rc2

In the C API documentation mention the potential problems of calling
non-nullable parameters with NULL.  Usually an error is returned, but
warnings and worse might happen too.

Thanks: Eric Blake
Acked-by: Laszlo Ersek <lersek at redhat.com>
---
 docs/libnbd.pod | 18 ++++++++++++++++++
 generator/C.ml  | 21 +++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index eae9bb413b..170fc1fa07 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -387,8 +387,26 @@ The library ran out of memory while performing some
operation.
 A request is too large, for example if you try to read too many bytes
 in a single L<nbd_pread(3)> call.
 
+=item C<EFAULT>
+
+A pointer parameter was C<NULL> when it should be non-NULL.
+See the section below.
+
 =back
 
+=head2 Non-NULL parameters
+
+Almost all libnbd functions when called from C take one or more
+pointer parameters that must not be C<NULL>.  For example, the handle
+parameter, strings and buffers should usually not be C<NULL>.
+
+If a C<NULL> is passed as one of these parameters, libnbd attempts to
+return an error with L<nbd_get_errno(3)> returning C<EFAULT>.
+
+However it may cause other compiler-related warnings and even
+undefined behaviour, so you should try to avoid this programming
+mistake.
+
 =head1 DEBUGGING MESSAGES
 
 Libnbd can print lots of debugging messages, useful if you have a
diff --git a/generator/C.ml b/generator/C.ml
index 2f22ad1e59..7d4bf7d509 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -107,6 +107,11 @@ let
   | UInt64 n -> [n]
   | UIntPtr n -> [n]
 
+let name_of_optarg +  function
+  | OClosure { cbname } -> cbname
+  | OFlags (n, _, _) -> n
+
 (* Map function arguments to __attribute__((nonnull)) annotations.
  * Because a single arg may map to multiple C parameters this
  * returns a list.  Note: true => add attribute nonnull.
@@ -980,6 +985,7 @@ let
 
   pr "=head1 ERRORS\n";
   pr "\n";
+
   if may_set_error then (
     let value = match errcode with
       | Some value -> value
@@ -993,6 +999,21 @@ let
     pr "This function does not fail.\n";
   pr "\n";
 
+  pr "The following parameters must not be NULL: C<h>";
+  List.iter (
+    fun arg ->
+      let nns = arg_attr_nonnull arg in
+      if List.mem true nns then pr ", C<%s>" (List.hd
(name_of_arg arg))
+  ) args;
+  List.iter (
+    fun arg ->
+      let nns = optarg_attr_nonnull arg in
+      if List.mem true nns then pr ", C<%s>" (name_of_optarg
arg)
+  ) optargs;
+  pr ".\n";
+  pr "For more information see L<libnbd(3)/Non-NULL
parameters>.\n";
+  pr "\n";
+
   if permitted_states <> [] then (
     pr "=head1 HANDLE STATE\n";
     pr "\n";
-- 
2.37.0.rc2

Change check into an assertion, and add detailed comments explaining
our assumptions.

Updates: commit d0fbb769286a97728b0d1358e7accc2eb708d795
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
---
 lib/utils.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/utils.c b/lib/utils.c
index b4043aa3bc..e538eeb4e6 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -79,11 +79,21 @@ nbd_internal_copy_string_list (string_vector *v, char **in)
 int
 nbd_internal_set_argv (struct nbd_handle *h, char **argv)
 {
-  /* FIXME: Do we want to assert(argv)? */
-  if (argv == NULL || argv[0] == NULL) {
+  /* This should never be NULL.  Normally the generator adds code to
+   * each StringList call in lib/api.c to check this and return an
+   * error.
+   */
+  assert (argv);
+
+  /* Because this function is only called from functions that take
+   * argv-style lists of strings (such as nbd_connect_command) we can
+   * check here that the command name is present.
+   */
+  if (argv[0] == NULL) {
     set_error (EINVAL, "missing command name in argv list");
     return -1;
   }
+
   if (nbd_internal_copy_string_list (&h->argv, argv) == -1) {
     set_error (errno, "realloc");
     return -1;
-- 
2.37.0.rc2

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 at redhat.com>

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