[Libguestfs] [libnbd PATCH 1/2] api: Better use of nbd_internal_reset_size_and_flags
Laszlo Ersek
lersek at redhat.com
Wed Aug 24 11:56:03 UTC 2022
On 08/24/22 04:53, Eric Blake wrote:
> The documentation for nbd_internal_reset_size_and_flags() claims we
> should wipe all knowledge of previously-negotiated meta contexts when
> swapping export names. However, we weren't actually doing that, which
> could lead to a user calling nbd_opt_info() for one export, then
> switching to another export name, then having nbd_get_size() still
> report the size of the old export. At present, this is only mildly
> confusing, but once the next patch exposes nbd_opt_set_meta_context()
> for user control,
(1) The "next patch" does not expose nbd_opt_set_meta_context(). In
fact, I can't find nbd_opt_set_meta_context() -- the function -- in the
codebase anywhere (@ 9e5bf05a2196).
(I prefer writing "a subsequent patch in this series" rather than "next
patch", because that allows me to reshuffle patches without having to
rewrite commit messages, as long as inter-patch dependencies are still
satisfied.)
(2) I see the NBD_OPT_SET_META_CONTEXT opcode, but I'm not sure what it
is used for. There seems to be documentation for
<https://libguestfs.org/nbd_opt_list_meta_context.3.html>, but not for
nbd_opt_set_meta_context(). So perhaps "introduces" is a better verb
than "exposes"?
Is NBD_OPT_SET_META_CONTEXT internally used by nbd_add_meta_context()
and nbd_clear_meta_contexts()?
> it would become actively dangerous: negotiating meta
> contexts for one export, then switching to a different export name
> before nbd_opt_go, can lead us into calling nbd_cmd_block_status() when
> the server is not expecting it.
(3) What is nbd_cmd_block_status()? I can only find nbd_block_status().
Do you mean the NBD extent callback that is called once by metacontext?
(4) The word "server" seems like a typo; libnbd is used by NBD clients.
>
> While fixing this, code things to check whether we are actually
> swapping export names; an idempotent nbd_set_export_name() is not
> observable by the server, and therefore does not need to invalidate
> what has already been negotiated.
>
OK.
> Conversely, calling nbd_opt_list_meta_context() does not need to wipe
> the list - it does not touch h->meta_contexts, but relies on the
> user's callback instead.
(5) I kind of wish this were a separate patch, but I see the point of
keeping it unified, too.
(6) I don't understand the "but". I see no conflict (or even: relevance)
with the user's callback. Listing the meta contexts does not touch
"h->meta_contexts", so we should not clear that list, period. I don't
understand what specifically we rely on the user's callback *for*, for
this particular discussion. If it's important, please elaborate;
otherwise I'm just getting confused! :)
The code looks OK to my untrained eye, so I'm ready to R-b that.
Thanks!
Laszlo
> ---
> generator/states-newstyle-opt-meta-context.c | 7 +++----
> lib/handle.c | 4 ++++
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c
> index 30b9617..76f1032 100644
> --- a/generator/states-newstyle-opt-meta-context.c
> +++ b/generator/states-newstyle-opt-meta-context.c
> @@ -1,5 +1,5 @@
> /* nbd client library in userspace: state machine
> - * Copyright (C) 2013-2020 Red Hat Inc.
> + * Copyright (C) 2013-2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -28,7 +28,6 @@ STATE_MACHINE {
> * contexts, when doing SET (but an empty LIST is okay).
> */
> assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
> - nbd_internal_reset_size_and_flags (h);
> if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> assert (h->opt_mode);
> assert (h->structured_replies);
> @@ -37,6 +36,8 @@ STATE_MACHINE {
> }
> else {
> assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> + nbd_internal_reset_size_and_flags (h);
> + assert (h->meta_contexts == NULL);
> opt = NBD_OPT_SET_META_CONTEXT;
> if (!h->structured_replies || h->request_meta_contexts.len == 0) {
> SET_NEXT_STATE (%^OPT_GO.START);
> @@ -44,8 +45,6 @@ STATE_MACHINE {
> }
> }
>
> - assert (h->meta_contexts == NULL);
> -
> /* Calculate the length of the option request data. */
> len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries */;
> for (i = 0; i < h->request_meta_contexts.len; ++i)
> diff --git a/lib/handle.c b/lib/handle.c
> index 8713e18..2aa89b9 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -219,6 +219,10 @@ nbd_unlocked_set_export_name (struct nbd_handle *h, const char *export_name)
> return -1;
> }
>
> + if (strcmp (export_name, h->export_name) == 0)
> + return 0;
> +
> + nbd_internal_reset_size_and_flags (h);
> new_name = strdup (export_name);
> if (!new_name) {
> set_error (errno, "strdup");
>
More information about the Libguestfs
mailing list