5:05 p.m.
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. Most clients don't attempt
this (nbd_opt_info is used by 'nbdinfo --list', but wasn't using
nbd_add_meta_context(); most other clients don't use nbd_opt_info)
- 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.
Likewise, the action of copying the implicit list of contexts during
NBD_OPT_SET_META_CONTEXT is relocated into a code block guarded by opt
!= h->current_opt; per the comments on this function, this is
currently only for nbd_opt_list_meta_context[_queries], but it sets
the stage for adding nbd_opt_set_meta_context[_queries] in a coming
patch.
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 | 69 ++++++++++++++++++--
generator/states-newstyle-opt-go.c | 1 +
generator/states-newstyle-opt-meta-context.c | 25 +++----
lib/flags.c | 4 +-
lib/handle.c | 17 +++++
tests/opt-info.c | 68 +++++++++++++++++--
7 files changed, 161 insertions(+), 24 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 308e65ef..3c9a2c10 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 0c72c356..0f70dcd1 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -830,6 +830,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;
@@ -1079,12 +1119,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", {
@@ -1153,16 +1198,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", {
@@ -1946,7 +1998,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", {
@@ -3434,6 +3487,8 @@ let first_version =
"stats_chunks_received", (1, 16);
"opt_list_meta_context_queries", (1, 16);
"aio_opt_list_meta_context_queries", (1, 16);
+ "set_request_meta_context", (1, 16);
+ "get_request_meta_context", (1, 16);
(* These calls are proposed for a future version of libnbd, but
* have not been added to any released version so far.
diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c
index 4c6e9900..3c29137d 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -270,6 +270,7 @@ NEWSTYLE.OPT_GO.CHECK_REPLY:
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 1eb9fbe7..5dc7e9ad 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -35,20 +35,16 @@ NEWSTYLE.OPT_META_CONTEXT.START:
* nbd_opt_list_meta_context()
* -> unconditionally use LIST, next state NEGOTIATING
*
- * For now, we start by unconditionally clearing h->exportsize and friends,
- * as well as h->meta_contexts and h->meta_valid.
- * If SET is conditional, we skip it if structured replies were
- * not negotiated, or if there were no contexts to request.
+ * For now, we start by unconditionally clearing h->exportsize and friends.
+ * If SET is conditional, we skip it if h->request_meta is false, if
+ * structured replies were not negotiated, or if no contexts to request.
* SET then manipulates h->meta_contexts, and sets h->meta_valid on success.
* If OPT_GO is later successful, it populates h->exportsize and friends,
- * and also sets h->meta_valid if we skipped SET here.
+ * and also sets h->meta_valid if h->request_meta but we skipped SET here.
* There is a callback if and only if the command is LIST.
*/
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));
@@ -57,7 +53,16 @@ NEWSTYLE.OPT_META_CONTEXT.START:
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 (opt != h->opt_current) {
+ if (!h->request_meta || !h->structured_replies ||
+ h->request_meta_contexts.len == 0) {
SET_NEXT_STATE (%^OPT_GO.START);
return 0;
}
@@ -67,8 +72,6 @@ NEWSTYLE.OPT_META_CONTEXT.START:
}
}
- 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->querylist.len; ++i)
diff --git a/lib/flags.c b/lib/flags.c
index 9619f235..03a1b12d 100644
--- a/lib/flags.c
+++ b/lib/flags.c
@@ -35,7 +35,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;
@@ -71,7 +70,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 6f262ba3..e59e6160 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -70,6 +70,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;
@@ -239,6 +240,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;
}
@@ -398,6 +400,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 b9739a5f..cf423d5c 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,60 @@ 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);
}
--
2.37.3