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 <ler...@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;
> }
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
