[Libguestfs] [libnbd PATCH v2 09/12] api: Reset state on changed nbd_set_export_name()

[Laszlo Ersek]

On 08/31/22 16:39, Eric Blake wrote:
> The documentation for nbd_internal_reset_size_and_flags() claims we
> should wipe all knowledge of a previously-negotiated export 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.  The next patch will add testsuite
> coverage (done separately, to make it easier to prove the test catches
> our flaw without this fix by applying patches out of order).
> 
> While at it, there is no need to wipe state if we change to the same
> name.  Note, however, that we do not bother to store the last name
> we've exposed to the server; as a result, there are instances where we
> clear state but the server does not.  But in general, this is not a
> problem; it's always okay to be more conservative and not utilize
> something the server supports, than to be be overly risky and use the
> server on the basis of stale state.  Here's an example (unlikely to
> happen in real code) where we are over-eager in wiping state, even
> though the server never knows we considered a different name:
> 
> nbd_set_export_name(nbd, "a");
> nbd_opt_info(nbd, callback);    => NBD_OPT_SET_META_CONTEXT("a")
>                                 => NBD_OPT_INFO("a")
> nbd_set_export_name(nbd, "b");
> nbd_set_request_meta_context(nbd, false);
> nbd_set_export_name(nbd, "a");
> nbd_opt_go(nbd);                => NBD_OPT_GO("a")
> 
> At any rate, this patch, plus several earlier ones, give us the
> following state transitions:
> 
> export information (nbd_get_size, nbd_is_read_only, nbd_can_trim, ...)
>  - gated by h->eflags being non-zero, but tracked in multiple
>    variables such as h->eflags, h->exportsize, h->block_minimum, ...
>  - made valid by successful NBD_OPT_INFO or NBD_OPT_GO
>  - wiped by:
>    - failed NBD_OPT_INFO or NBD_OPT_GO
>    - any NBD_OPT_STARTTLS (a later patch may add nbd_opt_starttls;
>      for now, it is not possible to tickle this)
>    - nbd_set_export_name() changing names (not directly caused by
>      server state, but because of how many of our interfaces
>      implicitly reuse h->export_name)
>  - persists over:
>    - NBD_OPT_LIST_META_CONTEXT
>    - NBD_OPT_SET_META_CONTEXT
> context mappings (nbd_can_meta_context)
>  - gated by h->meta_valid, tracked by h->meta_contexts
>  - made valid (returns 0/1 for all names) by:
>    - successful NBD_OPT_SET_META_CONTEXT
>    - if nbd_set_request_meta_context() is true, a successful
>      nbd_opt_info() or nbd_opt_go() that was unable to set contexts
>      (server was oldstyle, or structured replies are lacking, or
>      client didn't use nbd_add_meta_context)
>  - wiped (returns -1 for all names) by:
>    - failed NBD_OPT_SET_META_CONTEXT
>    - any NBD_OPT_STARTTLS
>    - nbd_set_export_name() changing names
>  - persists over:
>   - NBD_OPT_GO, NBD_OPT_LIST
>   - NBD_OPT_LIST_META_CONTEXT

I don't think I can internalize this without a state diagram and/or
being exposed to it repeatedly over a long time. (However, this is
obviously not a suggestion for you to *make* a diagram -- I'm not saying
that, just putting my review's worth in perspective.)

At the same time, the code looks sensible to me.

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Laszlo


> 
> Fixes: 437a472d ("api: Add nbd_opt_go and nbd_aio_opt_go", v1.3.12)
> ---
>  lib/handle.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/handle.c b/lib/handle.c
> index 4b373f5..d8c0dfe 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -225,6 +225,9 @@ 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;
> +
>    new_name = strdup (export_name);
>    if (!new_name) {
>      set_error (errno, "strdup");
> @@ -233,6 +236,7 @@ nbd_unlocked_set_export_name (struct nbd_handle *h, const char *export_name)
> 
>    free (h->export_name);
>    h->export_name = new_name;
> +  nbd_internal_reset_size_and_flags (h);
>    h->meta_valid = false;
>    return 0;
>  }
>