Eric Blake
2022-Aug-24 02:53 UTC
[Libguestfs] [libnbd PATCH 1/2] api: Better use of nbd_internal_reset_size_and_flags
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, 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.
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.
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.
---
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");
--
2.37.2
Laszlo Ersek
2022-Aug-24 11:56 UTC
[Libguestfs] [libnbd PATCH 1/2] api: Better use of nbd_internal_reset_size_and_flags
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"); >