On Wed, Sep 28, 2022 at 06:25:34PM +0100, Richard W.M. Jones wrote: 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... 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. 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. 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. 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

On Thu, Sep 29, 2022 at 09:10:21AM +0100, Richard W.M. Jones wrote: Yep, that was the approach I had in mind. You could check it in as-is; but I have a further simplification: 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); With fewer helpers, this would be: return opt_meta_context_queries (h, NBD_OPT_LIST_META_CONTEXT, NULL, context); this would be return opt_meta_context_queries (h, NBD_OPT_LIST_META_CONTEXT, queries, context); 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). This would be return opt_meta_context_queries (h, NBD_OPT_SET_META_CONTEXT, NULL, context) This would be return opt_meta_context_queries (h, NBD_OPT_SET_META_CONTEXT, queries, context) This one disappears as redundant (because we have one helper function handling both opt codes) 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

On Wed, Sep 28, 2022 at 06:25:35PM +0100, Richard W.M. Jones wrote: Comment typo: No ',' before the innermost '('. The typo's easy to fix, Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Wed, Sep 28, 2022 at 06:25:35PM +0100, Richard W.M. Jones wrote: Style question. Do we want to REQUIRE the clients of this macro to pass in (), or would it be better to have a varargs format? For generated code, it is just as easy to cope with either style (we can strip a layer of () if we want a varargs format). This definition is what requires us to pass in our own (). That is, our end result is going to be one of: __attribute__((__nonnull__(1) )) __attribute__((__nonnull__(1, 2) )) but the difference is whether we must pass exactly one macro argument, and where that argument must include () even when there is only one parameter to be marked (what you coded): LIBNBD_ATTRIBUTE_NONNULL((1)) LIBNBD_ATTRIBUTE_NONNULL((1, 3)) vs. ease-of-use in supplying the () as part of the macro definition itself by using #define MACRO(...) and __VA_ARGS__: LIBNBD_ATTRIBUTE_NONNULL(1) LIBNBD_ATTRIBUTE_NONNULL(1, 3) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

--- lib/internal.h | 75 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 25 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 7896b1d5ca..fb15ada1c3 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -384,20 +384,27 @@ struct command { } while (0) /* aio.c */ -extern void nbd_internal_retire_and_free_command (struct command *); +extern void nbd_internal_retire_and_free_command (struct command *) + LIBNBD_ATTRIBUTE_NONNULL((1)); /* connect.c */ -extern int nbd_internal_wait_until_connected (struct nbd_handle *h); +extern int nbd_internal_wait_until_connected (struct nbd_handle *h) + LIBNBD_ATTRIBUTE_NONNULL((1)); /* crypto.c */ -extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, struct socket *oldsock); -extern bool nbd_internal_crypto_is_reading (struct nbd_handle *); -extern int nbd_internal_crypto_handshake (struct nbd_handle *); -extern void nbd_internal_crypto_debug_tls_enabled (struct nbd_handle *); +extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, struct socket *oldsock) + LIBNBD_ATTRIBUTE_NONNULL((1, 2)); +extern bool nbd_internal_crypto_is_reading (struct nbd_handle *) + LIBNBD_ATTRIBUTE_NONNULL((1)); +extern int nbd_internal_crypto_handshake (struct nbd_handle *) + LIBNBD_ATTRIBUTE_NONNULL((1)); +extern void nbd_internal_crypto_debug_tls_enabled (struct nbd_handle *) + LIBNBD_ATTRIBUTE_NONNULL((1)); /* debug.c */ extern void nbd_internal_debug (struct nbd_handle *h, const char *context, - const char *fs, ...); + const char *fs, ...) + LIBNBD_ATTRIBUTE_NONNULL((1, 3)); #define debug(h, fs, ...) \ do { \ if_debug ((h)) \ @@ -410,9 +417,11 @@ extern void nbd_internal_debug (struct nbd_handle *h, const char *context, } while (0) /* errors.c */ -extern void nbd_internal_set_error_context (const char *context); +extern void nbd_internal_set_error_context (const char *context) + LIBNBD_ATTRIBUTE_NONNULL((1)); extern const char *nbd_internal_get_error_context (void); -extern void nbd_internal_set_last_error (int errnum, char *error); +extern void nbd_internal_set_last_error (int errnum, char *error) + LIBNBD_ATTRIBUTE_NONNULL((2)); #define set_error(errnum, fs, ...) \ do { \ int _e = (errnum); \ @@ -430,12 +439,15 @@ extern void nbd_internal_set_last_error (int errnum, char *error); } while (0) /* flags.c */ -extern void nbd_internal_reset_size_and_flags (struct nbd_handle *h); +extern void nbd_internal_reset_size_and_flags (struct nbd_handle *h) + LIBNBD_ATTRIBUTE_NONNULL((1)); extern int nbd_internal_set_size_and_flags (struct nbd_handle *h, uint64_t exportsize, - uint16_t eflags); + uint16_t eflags) + LIBNBD_ATTRIBUTE_NONNULL((1)); extern int nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min, - uint32_t pref, uint32_t max); + uint32_t pref, uint32_t max) + LIBNBD_ATTRIBUTE_NONNULL((1)); /* is-state.c */ extern bool nbd_internal_is_state_created (enum state state); @@ -447,7 +459,8 @@ extern bool nbd_internal_is_state_dead (enum state state); extern bool nbd_internal_is_state_closed (enum state state); /* opt.c */ -extern void nbd_internal_free_option (struct nbd_handle *h); +extern void nbd_internal_free_option (struct nbd_handle *h) + LIBNBD_ATTRIBUTE_NONNULL((1)); /* protocol.c */ extern int nbd_internal_errno_of_nbd_error (uint32_t error); @@ -458,15 +471,18 @@ extern int64_t nbd_internal_command_common (struct nbd_handle *h, uint16_t flags, uint16_t type, uint64_t offset, uint64_t count, int count_err, void *data, - struct command_cb *cb); + struct command_cb *cb) + LIBNBD_ATTRIBUTE_NONNULL((1)); /* socket.c */ struct socket *nbd_internal_socket_create (int fd); /* states.c */ extern void nbd_internal_abort_commands (struct nbd_handle *h, - struct command **list); -extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev); + struct command **list) + LIBNBD_ATTRIBUTE_NONNULL((1, 2)); +extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev) + LIBNBD_ATTRIBUTE_NONNULL((1)); extern const char *nbd_internal_state_short_string (enum state state); extern enum state_group nbd_internal_state_group (enum state state); extern enum state_group nbd_internal_state_group_parent (enum state_group group); @@ -477,14 +493,22 @@ extern int nbd_internal_aio_get_direction (enum state state); #define get_public_state(h) ((h)->public_state) /* utils.c */ -extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp); -extern int nbd_internal_copy_string_list (string_vector *v, char **in); -extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv); -extern int nbd_internal_set_querylist (struct nbd_handle *h, char **queries); -extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len); -extern void nbd_internal_fork_safe_perror (const char *s); -extern char *nbd_internal_printable_buffer (const void *buf, size_t count); -extern char *nbd_internal_printable_string (const char *str); +extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp) + LIBNBD_ATTRIBUTE_NONNULL((1, 3)); +extern int nbd_internal_copy_string_list (string_vector *v, char **in) + LIBNBD_ATTRIBUTE_NONNULL((1, 2)); +extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv) + LIBNBD_ATTRIBUTE_NONNULL((1, 2)); +extern int nbd_internal_set_querylist (struct nbd_handle *h, char **queries) + LIBNBD_ATTRIBUTE_NONNULL((1, 2)); +extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len) + LIBNBD_ATTRIBUTE_NONNULL((2)); +extern void nbd_internal_fork_safe_perror (const char *s) + LIBNBD_ATTRIBUTE_NONNULL((1)); +extern char *nbd_internal_printable_buffer (const void *buf, size_t count) + LIBNBD_ATTRIBUTE_NONNULL((1)); +extern char *nbd_internal_printable_string (const char *str) + LIBNBD_ATTRIBUTE_NONNULL((1)); extern char *nbd_internal_printable_string_list (char **list); /* These are wrappers around socket(2) and socketpair(2). They @@ -494,6 +518,7 @@ extern char *nbd_internal_printable_string_list (char **list); extern int nbd_internal_socket (int domain, int type, int protocol, bool nonblock); extern int nbd_internal_socketpair (int domain, int type, int protocol, - int *fds); + int *fds) + LIBNBD_ATTRIBUTE_NONNULL((4)); #endif /* LIBNBD_INTERNAL_H */ -- 2.37.0.rc2

On Wed, Sep 28, 2022 at 06:25:37PM +0100, Richard W.M. Jones wrote: The FIXME comment is now stale and can be removed. With that fixed, this one looks good. tests/opt-list-meta-queries.c has a similar FIXME from commit 05b274ef, but where I ripped out the code that was in the version of the patch I had posted to the mailing list when I ripped out the EFAULT handling. Can be reinstated in a separate patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

In the C API documentation mention the potential problems of calling non-nullable parameters with NULL. Usually an error is returned, but warnings and worse might happen too. Thanks: Eric Blake Acked-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- docs/libnbd.pod | 18 ++++++++++++++++++ generator/C.ml | 21 +++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/docs/libnbd.pod b/docs/libnbd.pod index eae9bb413b..170fc1fa07 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -387,8 +387,26 @@ The library ran out of memory while performing some operation. A request is too large, for example if you try to read too many bytes in a single L<nbd_pread(3)> call. +=item C<EFAULT> + +A pointer parameter was C<NULL> when it should be non-NULL. +See the section below. + =back +=head2 Non-NULL parameters + +Almost all libnbd functions when called from C take one or more +pointer parameters that must not be C<NULL>. For example, the handle +parameter, strings and buffers should usually not be C<NULL>. + +If a C<NULL> is passed as one of these parameters, libnbd attempts to +return an error with L<nbd_get_errno(3)> returning C<EFAULT>. + +However it may cause other compiler-related warnings and even +undefined behaviour, so you should try to avoid this programming +mistake. + =head1 DEBUGGING MESSAGES Libnbd can print lots of debugging messages, useful if you have a diff --git a/generator/C.ml b/generator/C.ml index 2f22ad1e59..7d4bf7d509 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -107,6 +107,11 @@ let | UInt64 n -> [n] | UIntPtr n -> [n] +let name_of_optarg = + function + | OClosure { cbname } -> cbname + | OFlags (n, _, _) -> n + (* Map function arguments to __attribute__((nonnull)) annotations. * Because a single arg may map to multiple C parameters this * returns a list. Note: true => add attribute nonnull. @@ -980,6 +985,7 @@ let pr "=head1 ERRORS\n"; pr "\n"; + if may_set_error then ( let value = match errcode with | Some value -> value @@ -993,6 +999,21 @@ let pr "This function does not fail.\n"; pr "\n"; + pr "The following parameters must not be NULL: C<h>"; + List.iter ( + fun arg -> + let nns = arg_attr_nonnull arg in + if List.mem true nns then pr ", C<%s>" (List.hd (name_of_arg arg)) + ) args; + List.iter ( + fun arg -> + let nns = optarg_attr_nonnull arg in + if List.mem true nns then pr ", C<%s>" (name_of_optarg arg) + ) optargs; + pr ".\n"; + pr "For more information see L<libnbd(3)/Non-NULL parameters>.\n"; + pr "\n"; + if permitted_states <> [] then ( pr "=head1 HANDLE STATE\n"; pr "\n"; -- 2.37.0.rc2

Change check into an assertion, and add detailed comments explaining our assumptions. Updates: commit d0fbb769286a97728b0d1358e7accc2eb708d795 Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- lib/utils.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/utils.c b/lib/utils.c index b4043aa3bc..e538eeb4e6 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -79,11 +79,21 @@ nbd_internal_copy_string_list (string_vector *v, char **in) int nbd_internal_set_argv (struct nbd_handle *h, char **argv) { - /* FIXME: Do we want to assert(argv)? */ - if (argv == NULL || argv[0] == NULL) { + /* This should never be NULL. Normally the generator adds code to + * each StringList call in lib/api.c to check this and return an + * error. + */ + assert (argv); + + /* Because this function is only called from functions that take + * argv-style lists of strings (such as nbd_connect_command) we can + * check here that the command name is present. + */ + if (argv[0] == NULL) { set_error (EINVAL, "missing command name in argv list"); return -1; } + if (nbd_internal_copy_string_list (&h->argv, argv) == -1) { set_error (errno, "realloc"); return -1; -- 2.37.0.rc2