Libguestfs - Aug 2022 - [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");
>

On Wed, Aug 24, 2022 at 01:56:03PM +0200, Laszlo Ersek
wrote:
> 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.)
Yep, exactly what happened; where the subsequent patch is still not
quite polished enough to push to the list (but hopefully later today).
Will fix before committing.
> 
> (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()?
Right now, the code base does:

nbd_set_export_name() => update h->export_name; no server interaction

nbd_add_meta_context() => append to h->request_meta_contexts; no
server interaction

nbd_clear_meta_contexts() => wipe all of h->request_meta_contexts; no
server interaction

nbd_opt_list_meta_context() => Issue NBD_OPT_LIST_META_CONTEXT to the
server using h->request_meta_contexts as its query list and
h->export_name, but collect the results only in the user's callback -
not stateful on the server, and only stateful in the client if the
callback does something to preserve the server's answers

nbd_opt_info()/nbd_opt_go() => Issue NBD_OPT_SET_META_CONTEXT using
h->request_meta_contexts as its query list and h->export_name, collect
the results into h->meta_contexts, then issue NBD_OPT_INFO/GO to the
server.  Stateful, because the server's replies map the users context
names (strings) to the servers ids (uint32_t); and later
NBD_CMD_BLOCK_STATUS refers only to the server's 32-bit id, even
though the libnbd API does not directly expose those ids.

nbd_can_meta_context() => Query whether h->meta_contexts contains a
server reply for an earlier call that triggered
NBD_OPT_SET_META_CONTEXT and which has not been wiped by changing
state via nbd_internal_reset_size_and_flags(); no server interaction.

with the additional caveat that all the nbd_opt_* functions are
unreachable unless you do nbd_set_opt_mode(true) to enable manual
option negotiation (the default is that nbd_connect_*() behaves as if
it does nbd_opt_go() on your behalf).

The promised upcoming patch will add:

nbd_set_request_meta_context(false) => Skip the
NBD_OPT_SET_META_CONTEXT portion in nbd_opt_info()/nbd_opt_go()

nbd_opt_set_meta_context() => Issue NBD_OPT_SET_META_CONTEXT to the
server using h->request_meta_contexts as its query list and
h->export_name, and collect the results into both the user's callback
and h->meta_contexts at the same time.  Stateful - wipes out any ids
returned by an earlier SET_META_CONTEXT, but the ids returned by the
server can survive a number of other NBD_OPT_ commands prior to
NBD_OPT_GO, provided that NBD_OPT_GO uses the same export name as
NBD_OPT_SET_META_CONTEXT.

In that manner, it will become possible for manual option haggling to
set the meta contexts at a different point in the negotiation phase,
useful when trying to trigger various server corner cases.

The server wipes meta context state any time it gets
NBD_OPT_SET_META_CONTEXT or NBD_OPT_GO with a different export name
than before.  But since we don't pass in export name as a direct
parameter to either nbd_opt_go() or the upcoming
nbd_opt_set_meta_context(), we instead need to wipe the state when we
change h->export_name (that is, at nbd_set_export_name()).  This does
mean that if a client calls nbd_set_export_name(nbd, "a"),
nbd_set_meta_context(...), nbd_set_export_name(nbd, "b"),
nbd_set_export_name(nbd, "a"), nbd_opt_go(nbd), that the server will
not observe the name change, and will actually support the
previously-negotiated contexts, but libnbd will have wiped access to
those, and trigger client-side failure on attempts to call
nbd_block_status() on the grounds that it doesn't remember any
negotiated contexts.  But a client actually doing this is unlikely.
> 
> > 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?
Typo on my part.  The API nbd_block_status() issues
NBD_CMD_BLOCK_STATUS to the server.
> 
> (4) The word "server" seems like a typo; libnbd is used by NBD
clients.
Here, it is intended.  The NBD spec says a client should not issue
NBD_OPT_BLOCK_STATUS to the server unless it successfully negotiated
NBD_OPT_SET_META_CONTEXT with the same export name later passed to
NBD_OPT_GO, without intervening commands that might wipe that state.
The client code in nbd_block_status() tries to enforce that it won't
confuse the server by checking whether h->meta_contexts is non-NULL
before it will issue NBD_CMD_BLOCK_STATUS.  But once we allow manual
control over setting meta contexts, rather than tightly coupling it
with NBD_OPT_GO, we need to make sure we wipe h->meta_contexts in at
least all places the server wipes state (even if we also happen to
wipe it in more places, as in my
"a"/set/"b"/"a"/go example).
> 
> > 
> > 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.
Maybe I can separate it; the test would be calling nbd_opt_info() to
set h->meta_contexts, nbd_can_meta_context() to verify that
"base:allocation" is supported by the server,
nbd_opt_list_meta_context() to do a query, then nbd_can_meta_context()
again to see whether "base:allocation" is still remembered or was
accidentally wiped.
> 
> (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! :)
Fair enough.  Basically, nbd_opt_list_meta_context() hands the
server's answers to the user's callback instead of by modifications to
h->meta_contexts, so it does not impact future nbd_can_meta_context().
I'll see if I can reword that a bit.
> 
> 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);
> > -
This hunk fixes the nbd_opt_list_meta_context() accidentally wiping
context when it doesn't have to,
> >    /* 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);
and this hunk fixes the missing wipe (the testsuite coverage so far
only covered this case).
> >    new_name = strdup (export_name);
> >    if (!new_name) {
> >      set_error (errno, "strdup");
> > 
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org