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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs