On 08/31/22 16:39, Eric Blake wrote:
> Add a new control knob nbd_set_request_meta_context(), modeled after
> the existing nbd_set_request_structured_replies(), to make it possible
> to skip the NBD_OPT_SET_META_CONTEXT half of the two-command sequence
> currently performed in nbd_opt_go() and nbd_opt_info(). Also add a
> counterpart nbd_get_request_meta_context() for symmetry.
>
> A later patch will then add the ability for the user to manually
> invoke nbd_opt_set_meta_context() at a time of their choosing during
> option negotiation; but even without that patch, this new API has some
> demonstrable effects by itself:
>
> - skipping meta contexts but not structured replies lets us test
> additional corner cases of servers (for example, while trying to
> write my unit tests, I quickly found out that with structured
> replies negotiated, current nbdkit ALWAYS emulates and advertises
> the base:allocation context rather than consulting .can_extents as a
> way to suppress it on a per-export basis, even when a corresponding
> .open would fail. A future nbdkit may make .can_extents tri-state to
> make it possible to do structured reads but not extents; however,
> the current nbdkit behavior appears to comply with the NBD spec,
> which allows but does not require NBD_OPT_SET_META_CONTEXT to pay
> attention to the export name)
>
> - back-to-back nbd_opt_info() and nbd_opt_go() was performing a
> redundant SET_META_CONTEXT; with this new API, we can get by with
> less network traffic during negotiation
>
> - nbd_opt_info() has to be client-side stateful (you check things like
> nbd_get_size after the fact), but based on the name, the fact that
> it was also server-side stateful was surprising. Skipping
> SET_META_CONTEXT during nbd_opt_info(), and instead using
> stateless[1] nbd_opt_list_meta_context(), avoids messing with server
> state and can also be more convenient in getting supported context
> names by callback instead of lots of post-process
> nbd_can_meta_context() calls
>
> Things to note in the patch: the choice of when to change h->meta_valid
> is moved around. Marking contexts invalid is no longer a side effect
> of clearing h->exportsize (that is, once set_request_meta_context is
> false, nbd_opt_go() and nbd_opt_info() should inherit what contexts
> are previously negotiated); it is now an explicit action when changing
> the export name[2], starting an actual NBD_OPT_SET_META_CONTEXT
> request, or upon failure of NBD_OPT_GO/INFO.
>
> The testsuite changes added here depend on the new API; therefore,
> there is no benefit to separating the C change to a separate patch (if
> split and you rearranged the series, it would fail to compile).
> However, for ease of review, porting the test to its counterparts in
> other languages is split out.
>
> [1] nbd_opt_list_meta_context() is slightly improved here, but has
> other client-side state effects that are left for a later patch to
> minimize the size of this one
> [2] nbd_set_export_name() should also reset size, but I'm leaving that
> for a later patch to minimize this one
> ---
> lib/internal.h | 1 +
> generator/API.ml | 71 ++++++-
> generator/states-newstyle-opt-go.c | 1 +
> generator/states-newstyle-opt-meta-context.c | 19 +-
> lib/flags.c | 4 +-
> lib/handle.c | 17 ++
> tests/opt-info.c | 67 +++++-
> tests/opt-set-meta | 210 +++++++++++++++++++
> 8 files changed, 369 insertions(+), 21 deletions(-)
> create mode 100755 tests/opt-set-meta
>
> diff --git a/lib/internal.h b/lib/internal.h
> index 8aaff15..9d329f0 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -111,6 +111,7 @@ struct nbd_handle {
>
> /* Desired metadata contexts. */
> bool request_sr;
> + bool request_meta;
> string_vector request_meta_contexts;
>
> /* Allowed in URIs, see lib/uri.c. */
> diff --git a/generator/API.ml b/generator/API.ml
> index 62e2d54..adafac6 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -747,6 +747,46 @@ "get_structured_replies_negotiated", {
> Link "get_protocol"];
> };
>
> + "set_request_meta_context", {
> + default_call with
> + args = [Bool "request"]; ret = RErr;
> + permitted_states = [ Created; Negotiating ];
> + shortdesc = "control whether connect automatically requests meta
> contexts";
> + longdesc = "\
> +This function controls whether the act of connecting to an export
> +(all C<nbd_connect_*> calls when L<nbd_set_opt_mode(3)> is false,
> +or L<nbd_opt_go(3)> and L<nbd_opt_info(3)> when option mode is
> +enabled) will also try to issue NBD_OPT_SET_META_CONTEXT when
> +the server supports structured replies and any contexts were
> +registered by L<nbd_add_meta_context(3)>. The default setting
> +is true; however the extra step of negotiating meta contexts is
> +not always desirable: performing both info and go on the same
> +export works without needing to re-negotiate contexts on the
> +second call; and even when using just L<nbd_opt_info(3)>, it
> +can be faster to collect the server's results by relying on the
> +callback function passed to L<nbd_opt_list_meta_context(3)> than
> +a series of post-process calls to L<nbd_can_meta_context(3)>.
> +
> +Note that this control has no effect if the server does not
> +negotiate structured replies, or if the client did not request
> +any contexts via L<nbd_add_meta_context(3)>. Setting this
> +control to false may cause L<nbd_block_status(3)> to fail.";
> + see_also = [Link "set_opt_mode"; Link "opt_go"; Link "opt_info";
> + Link "opt_list_meta_context";
> + Link "get_structured_replies_negotiated";
> + Link "get_request_meta_context"; Link "can_meta_context"];
> + };
> +
> + "get_request_meta_context", {
> + default_call with
> + args = []; ret = RBool;
> + permitted_states = [];
> + shortdesc = "see if connect automatically requests meta contexts";
> + longdesc = "\
> +Return the state of the automatic meta context request flag on this handle.";
> + see_also = [Link "set_request_meta_context"];
> + };
> +
> "set_handshake_flags", {
> default_call with
> args = [ Flags ("flags", handshake_flags) ]; ret = RErr;
> @@ -994,12 +1034,17 @@ "opt_go", {
> or L<nbd_connect_uri(3)>. This can only be used if
> L<nbd_set_opt_mode(3)> enabled option mode.
>
> +By default, libnbd will automatically request all meta contexts
> +registered by L<nbd_add_meta_context(3)> as part of this call; but
> +this can be suppressed with L<nbd_set_request_meta_context(3)>.
> +
> If this fails, the server may still be in negotiation, where it is
> possible to attempt another option such as a different export name;
> although older servers will instead have killed the connection.";
> example = Some "examples/list-exports.c";
> see_also = [Link "set_opt_mode"; Link "aio_opt_go"; Link "opt_abort";
> - Link "set_export_name"; Link "connect_uri"; Link "opt_info"];
> + Link "set_export_name"; Link "connect_uri"; Link "opt_info";
> + Link "add_meta_context"; Link "set_request_meta_context"];
> };
>
> "opt_abort", {
> @@ -1068,16 +1113,23 @@ "opt_info", {
> L<nbd_set_opt_mode(3)> enabled option mode.
>
> If successful, functions like L<nbd_is_read_only(3)> and
> -L<nbd_get_size(3)> will report details about that export. In
> -general, if L<nbd_opt_go(3)> is called next, that call will
> -likely succeed with the details remaining the same, although this
> -is not guaranteed by all servers.
> +L<nbd_get_size(3)> will report details about that export. If
> +L<nbd_set_request_meta_context(3)> is set (the default) and
> +structured replies were negotiated, it is also valid to use
> +L<nbd_can_meta_context(3)> after this call. However, it may be
> +more efficient to clear that setting and manually utilize
> +L<nbd_opt_list_meta_context(3)> with its callback approach, for
> +learning which contexts an export supports. In general, if
> +L<nbd_opt_go(3)> is called next, that call will likely succeed
> +with the details remaining the same, although this is not
> +guaranteed by all servers.
>
> Not all servers understand this request, and even when it is
> understood, the server might fail the request even when a
> corresponding L<nbd_opt_go(3)> would succeed.";
> see_also = [Link "set_opt_mode"; Link "aio_opt_info"; Link "opt_go";
> - Link "set_export_name"];
> + Link "set_export_name"; Link "set_request_meta_context";
> + Link "opt_list_meta_context"];
> };
>
> "opt_list_meta_context", {
> @@ -1797,7 +1849,8 @@ "can_meta_context", {
> ^ non_blocking_test_call_description;
> see_also = [SectionLink "Flag calls"; Link "opt_info";
> Link "add_meta_context";
> - Link "block_status"; Link "aio_block_status"];
> + Link "block_status"; Link "aio_block_status";
> + Link "set_request_meta_context"];
> };
>
> "get_protocol", {
> @@ -3246,6 +3299,10 @@ let first_version =
> "set_request_block_size", (1, 12);
> "get_request_block_size", (1, 12);
>
> + (* Added in 1.13.x development cycle, will be stable and supported in
> 1.14. *)
> + "set_request_meta_context", (1, 14);
> + "get_request_meta_context", (1, 14);
> +
> (* These calls are proposed for a future version of libnbd, but
> * have not been added to any released version so far.
> "get_tls_certificates", (1, ??);
> diff --git a/generator/states-newstyle-opt-go.c
> b/generator/states-newstyle-opt-go.c
> index b7354ae..1ca5f09 100644
> --- a/generator/states-newstyle-opt-go.c
> +++ b/generator/states-newstyle-opt-go.c
> @@ -269,6 +269,7 @@ STATE_MACHINE {
> reply);
> }
> nbd_internal_reset_size_and_flags (h);
> + h->meta_valid = false;
> err = nbd_get_errno () ? : ENOTSUP;
> break;
> case NBD_REP_ACK:
> diff --git a/generator/states-newstyle-opt-meta-context.c
> b/generator/states-newstyle-opt-meta-context.c
> index 5c65454..35d3cbc 100644
> --- a/generator/states-newstyle-opt-meta-context.c
> +++ b/generator/states-newstyle-opt-meta-context.c
> @@ -29,9 +29,6 @@ STATE_MACHINE {
> */
> assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
> nbd_internal_reset_size_and_flags (h);
> - for (i = 0; i < h->meta_contexts.len; ++i)
> - free (h->meta_contexts.ptr[i].name);
> - meta_vector_reset (&h->meta_contexts);
> if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> assert (h->opt_mode);
> assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context));
> @@ -40,13 +37,19 @@ STATE_MACHINE {
> else {
> assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> opt = NBD_OPT_SET_META_CONTEXT;
> - if (!h->structured_replies || h->request_meta_contexts.len == 0) {
> - SET_NEXT_STATE (%^OPT_GO.START);
> - return 0;
> + if (h->request_meta) {
> + for (i = 0; i < h->meta_contexts.len; ++i)
> + free (h->meta_contexts.ptr[i].name);
> + meta_vector_reset (&h->meta_contexts);
> + h->meta_valid = false;
> }
> }
> -
> - assert (!h->meta_valid);
> + if (opt != h->opt_current &&
> + (!h->request_meta || !h->structured_replies ||
> + h->request_meta_contexts.len == 0)) {
> + SET_NEXT_STATE (%^OPT_GO.START);
> + return 0;
> + }
>
> /* Calculate the length of the option request data. */
> len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries
> */;
I'm confused about moving the transition to GO.START out of its current
enclosing block, and then compensating (?) for that unnesting with the
additional (opt != h->opt_current) restriction.
More precisely, I don't understand how (opt != h->opt_current) could
ever evaluate to 1.
Here's the code, with this patch applied:
> if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> assert (h->opt_mode);
> assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context));
> opt = h->opt_current;
> }
> else {
> assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> opt = NBD_OPT_SET_META_CONTEXT;
> if (h->request_meta) {
> for (i = 0; i < h->meta_contexts.len; ++i)
> free (h->meta_contexts.ptr[i].name);
> meta_vector_reset (&h->meta_contexts);
> h->meta_valid = false;
> }
> }
> if (opt != h->opt_current &&
> (!h->request_meta || !h->structured_replies ||
> h->request_meta_contexts.len == 0)) {
> SET_NEXT_STATE (%^OPT_GO.START);
> return 0;
> }
My understanding is that we enter this code with either one of two
possible "h->opt_current" values: NBD_OPT_LIST_META_CONTEXT,
NBD_OPT_SET_META_CONTEXT.
- In the first case, when "h->opt_current" equals LIST, "opt" at the end
of the first branch will be LIST (via assignment from
"h->opt_current"), that is, equal "h->opt_current".
- In the second case (when "h->opt_current" differs from LIST, hence,
when it equals SET), "opt" at the end of the second branch will be SET
(after having directly been assigned SET) -- that is, "opt" will equal
"h->opt_current" *again*.
That suggests the (opt != h->opt_current) condition will never evaluate
to "true". (And it also suggests that there is no good reason for the
"opt" local variable to exist...)
Now, in case I'm wrong, and we enter this code with a *third*
"h->opt_current" value, then we have:
- opt = SET, and
- "h->opt_current" differing from both SET and LIST.
Then we indeed transition to GO.START -- but then, wouldn't the
following hunk be *simpler*?
> diff --git a/generator/states-newstyle-opt-meta-context.c
> b/generator/states-newstyle-opt-meta-context.c
> index 5c654543a228..1e31eedd4cef 100644
> --- a/generator/states-newstyle-opt-meta-context.c
> +++ b/generator/states-newstyle-opt-meta-context.c
> @@ -40,14 +40,21 @@ STATE_MACHINE {
> else {
> assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
> opt = NBD_OPT_SET_META_CONTEXT;
> - if (!h->structured_replies || h->request_meta_contexts.len == 0) {
> +
> + if (h->request_meta) {
> + for (i = 0; i < h->meta_contexts.len; ++i)
> + free (h->meta_contexts.ptr[i].name);
> + meta_vector_reset (&h->meta_contexts);
> + h->meta_valid = false;
> + }
> +
> + if (!h->structured_replies ||
> + !h->request_meta || h->request_meta_contexts.len == 0) {
> SET_NEXT_STATE (%^OPT_GO.START);
> return 0;
> }
> }
>
> - assert (!h->meta_valid);
> -
> /* 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)
... FWIW, I think the formulation I'm proposing is correct (unlike the
patch) even in case we can only enter this section with SET and LIST!
Back to the patch:
On 08/31/22 16:39, Eric Blake wrote:
> diff --git a/lib/flags.c b/lib/flags.c
> index 91efc1a..c8c68ea 100644
> --- a/lib/flags.c
> +++ b/lib/flags.c
> @@ -37,7 +37,6 @@ nbd_internal_reset_size_and_flags (struct nbd_handle *h)
>
> h->exportsize = 0;
> h->eflags = 0;
> - h->meta_valid = false;
> h->block_minimum = 0;
> h->block_preferred = 0;
> h->block_maximum = 0;
Shouldn't we clear "h->meta_valid" in nbd_close() as well?
nbd_close() calls nbd_internal_reset_size_and_flags(), and clears the
metacontext vector.
> @@ -73,7 +72,8 @@ nbd_internal_set_size_and_flags (struct nbd_handle *h,
> eflags &= ~NBD_FLAG_SEND_FAST_ZERO;
> }
>
> - if (!h->structured_replies || h->request_meta_contexts.len == 0) {
> + if (h->request_meta &&
> + (!h->structured_replies || h->request_meta_contexts.len == 0)) {
> assert (h->meta_contexts.len == 0);
> h->meta_valid = true;
> }
> diff --git a/lib/handle.c b/lib/handle.c
> index 03f45a4..4b373f5 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -64,6 +64,7 @@ nbd_create (void)
> h->unique = 1;
> h->tls_verify_peer = true;
> h->request_sr = true;
> + h->request_meta = true;
> h->request_block_size = true;
> h->pread_initialize = true;
>
> @@ -232,6 +233,7 @@ nbd_unlocked_set_export_name (struct nbd_handle *h, const
> char *export_name)
>
> free (h->export_name);
> h->export_name = new_name;
> + h->meta_valid = false;
> return 0;
> }
>
> @@ -391,6 +393,21 @@ nbd_unlocked_get_request_structured_replies (struct
> nbd_handle *h)
> return h->request_sr;
> }
>
> +int
> +nbd_unlocked_set_request_meta_context (struct nbd_handle *h,
> + bool request)
> +{
> + h->request_meta = request;
> + return 0;
> +}
> +
> +/* NB: may_set_error = false. */
> +int
> +nbd_unlocked_get_request_meta_context (struct nbd_handle *h)
> +{
> + return h->request_meta;
> +}
> +
> int
> nbd_unlocked_get_structured_replies_negotiated (struct nbd_handle *h)
> {
> diff --git a/tests/opt-info.c b/tests/opt-info.c
> index b9739a5..26de0ee 100644
> --- a/tests/opt-info.c
> +++ b/tests/opt-info.c
> @@ -1,5 +1,5 @@
> /* NBD client library in userspace
> - * 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
> @@ -102,11 +102,15 @@ main (int argc, char *argv[])
> exit (EXIT_FAILURE);
> }
>
> - /* info for a different export */
> + /* info for a different export, with automatic meta_context disabled */
> if (nbd_set_export_name (nbd, "b") == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> }
> + if (nbd_set_request_meta_context (nbd, 0) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> if (nbd_opt_info (nbd) == -1) {
> fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> @@ -119,8 +123,12 @@ main (int argc, char *argv[])
> fprintf (stderr, "expecting read-write export, got %" PRId64 "\n", r);
> exit (EXIT_FAILURE);
> }
> - if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 1)
> {
> - fprintf (stderr, "expecting can_meta_context true, got %" PRId64 "\n",
> r);
> + if (nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) != -1) {
> + fprintf (stderr, "expecting error for can_meta_context\n");
> + exit (EXIT_FAILURE);
> + }
> + if (nbd_set_request_meta_context (nbd, 1) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> exit (EXIT_FAILURE);
> }
>
> @@ -189,8 +197,59 @@ main (int argc, char *argv[])
> fprintf (stderr, "expecting size of 4, got %" PRId64 "\n", r);
> exit (EXIT_FAILURE);
> }
> + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 1)
> {
> + fprintf (stderr, "expecting can_meta_context true, got %" PRId64 "\n",
> r);
> + exit (EXIT_FAILURE);
> + }
>
> nbd_shutdown (nbd, 0);
> nbd_close (nbd);
> +
> + /* Another connection. This time, check that SET_META triggered by opt_info
> + * persists through nbd_opt_go with set_request_meta_context disabled. */
> + nbd = nbd_create ();
> + if (nbd == NULL ||
> + nbd_set_opt_mode (nbd, true) == -1 ||
> + nbd_connect_command (nbd, args) == -1 ||
> + nbd_add_meta_context (nbd, "x-unexpected:bogus") == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + if (nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) != -1) {
> + fprintf (stderr, "expecting error for can_meta_context\n");
> + exit (EXIT_FAILURE);
> + }
> + if (nbd_opt_info (nbd) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 0)
> {
> + fprintf (stderr, "expecting can_meta_context false, got %" PRId64 "\n",
> r);
> +
> + exit (EXIT_FAILURE);
> + }
> + if (nbd_set_request_meta_context (nbd, 0) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> + /* Adding to the request list now won't matter */
> + if (nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) != 0) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> + if (nbd_opt_go (nbd) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 0)
> {
> + fprintf (stderr, "expecting can_meta_context false, got %" PRId64 "\n",
> r);
> +
> + exit (EXIT_FAILURE);
> + }
> +
> + nbd_shutdown (nbd, 0);
> + nbd_close (nbd);
> +
> exit (EXIT_SUCCESS);
> }
> diff --git a/tests/opt-set-meta b/tests/opt-set-meta
> new file mode 100755
> index 0000000..6c7b044
> --- /dev/null
> +++ b/tests/opt-set-meta
> @@ -0,0 +1,210 @@
> +#! /bin/sh
> +
> +# opt-set-meta - temporary wrapper script for .libs/opt-set-meta
> +# Generated by libtool (GNU libtool) 2.4.6
... Ugh, I initially missed this "generated by" line, and wondered (a)
why the script read like line noise, (b) why you spent time on ZSH and
Tru64 (!!!) compatibility, (c) what the script *actually did*.
Did you git-add this (generated) script to the commit by mistake,
perhaps?
Looks OK to me otherwise.
Thanks,
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
