Re: [Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList
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"
> 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 = };
> @@ -255,7 +278,7 @@ int
> nbd_unlocked_opt_set_meta_context (struct nbd_handle *h,
> nbd_context_callback *context)
> {
> - return
Re: [Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList
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"
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.
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);
+
/* 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);
}
/* 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);
+}
+
+static int
+opt_list_meta_context_queries (struct nbd_handle *h,
+ char **queries,
+ nbd_context_callback *context)
{
struct context_helper s = { .context = *context };
nbd_context_callback l = { .callback = context_visitor, .user_data = };
@@ -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);
}
/* 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);
+}
+
+static int
+opt_set_meta_context_queries (struct nbd_handle *h,
+ char **queries,
+ nbd_context_callback *context)
{
struct context_helper s = { .context = *context };
nbd_context_callback l = { .callback = context_visitor, .user_data = };
nbd_completion_callback c = { .callback = context_complete, .user_data =
};
- if (nbd_unlocked_aio_opt_set_meta_context_queries (h, queries, , ) == -1)
+ if (aio_opt_set_meta_context_queries (h, queries, , ) == -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
Re: [Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList
Let me try an alternate version of this. The problem is that I was getting GCC warnings even before adding the annotation to nbd_internal_set_querylist. Somehow the annotation on the public function "leaked" through to the internal function. 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList
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 (>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 (>querylist, h->querylist.len - 1);
>}
> - else {
> + else /* empty list */ {
> string_vector_reset (>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
