Re: [Libguestfs] [libnbd PATCH v2 04/12] api: Add nbd_set_request_meta_context()

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 */;

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; }

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)

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;

@@ -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