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
