On Thu, Sep 29, 2022 at 09:10:21AM +0100, Richard W.M. Jones wrote:
>
> How about this alternate to patch 1.
>
> I have also adjusted a later patch so it no longer sets
> attribute((nonnull)) on the queries parameter of
> nbd_internal_set_querylist, so that function is unchanged
> from before.
>
> Also the tests pass.
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> nbdkit - Flexible, fast NBD server with plugins
> https://gitlab.com/nbdkit/nbdkit
> From bc0de6d378dd78da13210a315fd134f9d063b1ba Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjo...@redhat.com>
> Date: Wed, 28 Sep 2022 18:01:40 +0100
> Subject: [PATCH] lib/opt: Don't call nbd_unlocked_*_meta_context_queries with
> NULL
>
> StringList parameters (char ** in C) will be marked with
> __attribute__((nonnull)), including the internal nbd_unlocked_*
> functions, so we cannot overload a new meaning with queries == NULL.
>
> Add internal common functions that allow this instead.
Yep, that was the approach I had in mind. You could check it in
as-is; but I have a further simplification:
>
> Fixes: commit e33762a86cd5f064e5ef841520baa5fe7361d2c2
> ---
> lib/opt.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 54 insertions(+), 7 deletions(-)
>
> diff --git a/lib/opt.c b/lib/opt.c
> index 1b18920fdb..68850a2ee9 100644
> --- a/lib/opt.c
> +++ b/lib/opt.c
> @@ -26,6 +26,21 @@
>
> #include "internal.h"
>
> +static int opt_set_meta_context_queries (struct nbd_handle *h,
> + char **queries,
> + nbd_context_callback *context);
> +static int opt_list_meta_context_queries (struct nbd_handle *h,
> + char **queries,
> + nbd_context_callback *context);
> +static int aio_opt_set_meta_context_queries (struct nbd_handle *h,
> + char **queries,
> + nbd_context_callback *context,
> + nbd_completion_callback
> *complete);
> +static int aio_opt_list_meta_context_queries (struct nbd_handle *h,
> + char **queries,
> + nbd_context_callback *context,
> + nbd_completion_callback
> *complete);
We could also get away with just two helper functions:
static int opt_meta_context_queries (struct nbd_handle *h,
uint16_t opt,
char **queries,
nbd_context_callback *context)
LIBNBD_ATTRIBUTE_NONNULL(1, 4);
static int aio_opt_meta_context_queries (struct nbd_handle *h,
uint16_t opt,
char **queries,
nbd_context_callback *context,
nbd_completion_callback *complete)
LIBNBD_ATTRIBUTE_NONNULL(1, 4);
> +
> /* Internal function which frees an option with callback. */
> void
> nbd_internal_free_option (struct nbd_handle *h)
> @@ -224,7 +239,7 @@ 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);
> + return opt_list_meta_context_queries (h, NULL, context);
With fewer helpers, this would be:
return opt_meta_context_queries (h, NBD_OPT_LIST_META_CONTEXT, NULL, context);
> }
>
> /* Issue NBD_OPT_LIST_META_CONTEXT and wait for the reply. */
> @@ -232,6 +247,14 @@ int
> nbd_unlocked_opt_list_meta_context_queries (struct nbd_handle *h,
> char **queries,
> nbd_context_callback *context)
> +{
> + return opt_list_meta_context_queries (h, queries, context);
this would be
return opt_meta_context_queries (h, NBD_OPT_LIST_META_CONTEXT, queries,
context);
> +}
> +
> +static int
> +opt_list_meta_context_queries (struct nbd_handle *h,
This would be opt_meta_context_queries, and take the additional opt
parameter. Its job is to then populate 'complete' and call into
aio_opt_meta_context_queries (h, opt, queries, context, complete).
> + char **queries,
> + nbd_context_callback *context)
> {
> struct context_helper s = { .context = *context };
> nbd_context_callback l = { .callback = context_visitor, .user_data = &s };
> @@ -255,7 +278,7 @@ 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);
> + return opt_set_meta_context_queries (h, NULL, context);
This would be
return opt_meta_context_queries (h, NBD_OPT_SET_META_CONTEXT, NULL, context)
> }
>
> /* Issue NBD_OPT_SET_META_CONTEXT and wait for the reply. */
> @@ -263,12 +286,20 @@ int
> nbd_unlocked_opt_set_meta_context_queries (struct nbd_handle *h,
> char **queries,
> nbd_context_callback *context)
> +{
> + return opt_set_meta_context_queries (h, queries, context);
This would be
return opt_meta_context_queries (h, NBD_OPT_SET_META_CONTEXT, queries,
context)
> +}
> +
> +static int
> +opt_set_meta_context_queries (struct nbd_handle *h,
> + char **queries,
> + nbd_context_callback *context)
> {
This one disappears as redundant (because we have one helper function
handling both opt codes)
> struct context_helper s = { .context = *context };
> nbd_context_callback l = { .callback = context_visitor, .user_data = &s };
> nbd_completion_callback c = { .callback = context_complete, .user_data =
> &s };
>
> - if (nbd_unlocked_aio_opt_set_meta_context_queries (h, queries, &l, &c) ==
> -1)
> + if (aio_opt_set_meta_context_queries (h, queries, &l, &c) == -1)
> return -1;
>
> SET_CALLBACK_TO_NULL (*context);
> @@ -371,8 +402,7 @@ 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,
> - complete);
> + return aio_opt_list_meta_context_queries (h, NULL, context, complete);
this would be
return aio_opt_meta_context_queries (h, NBD_OPT_LIST_META_CONTEXT, NULL,
context, complete)
and so forth.
--
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
