Libguestfs - Sep 2022 - [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList

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

On Wed, Sep 28, 2022 at 06:25:34PM +0100, Richard W.M. Jones
wrote:
> 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.
Hmm.  I intentionally WANT to pass NULL to the
nbd_internal_set_querylist function (as a way to explicitly copy the
defaults), while wanting to forbid NULL to the public
nbd_opt_list_meta_context_queries() API.  I'm not sure this patch does
what you think...
> 
> 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) {
By passing an explicit list instead of requesting that we reuse
h->request_meta_contexts, we have broken the set of queries that
nbd_opt_go() will utilize (that's an API that does not have a
StringList parameter, so we do want to reuse our default global list
instead of slamming it to an explicit empty list).  That's probably
why you saw test failures.
>        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);
Also broken for the same reason.  I'm okay if we force our internal
attributes of nbd_internal_API to match the attributes of nbd_API, but
for that to work, we may need to introduce:

static int
nbd_unlocked_opt_meta_context_queries_helper (struct nbd_handle *h, uint16_t
option,
                                              string_vector *queries /* may be
NULL */,
                                              nbd_context_callback *context)

and then have our various existing nbd_internal_API calls (for
NBD_OPT_SET/LIST_META_CONTEXT) call in to the helper function, rather
than trying to make one of the internal API calls blindly manage the
work for all the other APIs by accepting a NULL list parameter
different than the restriction on the public API.

I can respin this patch along those lines, if it would be easier to
see in code than in prose.
> +++ 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]);
This is not what I intended.  For this function, I really DID want
NULL to mean "list not present, use the global list with
nbd_internal_copy_string_list), and non-NULL (including when the list
is empty because queries[0] is NULL) to mean "use this explicit list,
regardless of its length".

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

OK now I remember what the problem was.
> @@ -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);
In this original code you're calling the internal unlocked version of
nbd_opt_set_meta_context_queries.  However the generator is creating a
prototype for the unlocked function and it adds the
attribute((nonnull)) annotation for it, something like:

  extern int nbd_unlocked_opt_set_meta_context_queries (...)
    LIBNBD_ATTRIBUTE_NONNULL((1, 2));

This means that you cannot use queries == NULL here.

I think the generated annotation is correct, but we need a new
unannotated internal function that allows queries == NULL.

I'll try to come up with something.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW