8:57 a.m.
On Thu, Sep 01, 2022 at 11:04:09AM +0200, Laszlo Ersek wrote:
...
>
> Rather than open-code a cleanup loop on all error paths, I instead fix
> the problem by adding a meta_valid witness that is only set to true in
> select places. The obvious place is when handling NBD_REP_ACK; but it
[1]
> also makes sense if we didn't call NBD_OPT_SET_META_CONTEXT
at all: if
> negotiating structured replies failed (including oldstyle servers), or
> if the user never called nbd_add_meta_context() and therefore doesn't
> care (blindly returning 0 to nbd_can_meta_context() in those cases is
> fine; our existing tests/oldstyle and tests/newstyle-limited cover
[2]
> that). However, whereas we previously always returned a 0/1
answer
> for nbd_can_meta_context once we are in transmission phase, this new
> witness now means that in the corner case of explicit server failure
> to NBD_OPT_SET_META_CONTEXT, nbd_can_meta_context() returns -1 to call
> attention to the earlier server failure (although it is something that
> is unlikely enough in practice that I doubt anyone will experience it
> in the wild).
>
> The change in nbd_can_meta_context() to use h->meta_valid instead of
> h->eflags has an additional latent semantic difference not observable
> at this time (because both h->meta_valid and h->eflags are set in
> tandem by successful nbd_opt_go()/nbd_opt_info() in their current
> two-command sequence), but which matters to future patches adding new
[3]
> APIs. On the one hand, once we add
nbd_set_request_meta_context() to
> stop nbd_opt_go() from performing NBD_OPT_SET_META_CONTEXT, we want
> nbd_can_meta_context() to fail during transmission phase if the
> context was not set elsewhere during negotiation, rather than blindly
patch 4/12 adds a test for this
> returning 0. On the other hand, when manually calling
> nbd_opt_set_meta_context() during negotiation, we do not want it to
> touch h->eflags, but do want nbd_can_meta_context() to start working
> right away.
patch 11/12 adds a test for this
>
> Note that the use of a witness flag also allows me to slightly change
> when the list of previously-negotiated contexts is freed - instead of
> doing it in nbd_internal_reset_size_and_flags(), that function now
> simply marks any existing vector as invalid; and the actual wipe is
[4]
> done when starting a new NBD_OPT_SET_META_CONTEXT or when
closing
[5]
> struct nbd_handle. There are still some oddities to be cleaned
up in
[6]
> later patches by moving when the flag is marked invalid (for
example,
> we really want nbd_set_export_name() to wipe both flags and meta
patch 9/12
> contexts, but the act of NBD_OPT_GO should not wipe contexts,
and the
patch 4/12
> act of NBD_OPT_SET_META_CONTEXT should not wipe flags), but
I'm
patch 7/12
> leaving those further tweaks for later patches to make this one
easier
> to review.
...
> +++ b/generator/states-newstyle-opt-meta-context.c
> @@ -29,6 +29,9 @@ 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);
This is code movement out of nbd_internal_reset_size_and_flags(), OK.
yep, this is the wipe [5] moved here because it was taken out of [4]
> if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
> assert (h->opt_mode);
> assert (h->structured_replies);
> @@ -44,7 +47,7 @@ STATE_MACHINE {
> }
> }
>
> - assert (h->meta_contexts.len == 0);
> + assert (!h->meta_valid);
OK, this is the new predicate that nbd_internal_reset_size_and_flags()
now ensures.
yep, matches up with [4]
>
> /* Calculate the length of the option request data. */
> len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries */;
> @@ -194,8 +197,10 @@ STATE_MACHINE {
> CALL_CALLBACK (h->opt_cb.completion, &err);
> nbd_internal_free_option (h);
> }
> - else
> + else {
> SET_NEXT_STATE (%^OPT_GO.START);
> + h->meta_valid = true;
> + }
> break;
> case NBD_REP_META_CONTEXT: /* A context. */
> if (len > maxpayload)
IIUC, this modifies the NBD_REP_ACK handling for
NBD_OPT_SET_META_CONTEXT. IIUC we've consumed all the mappings, so we
can now set the witness to "OK". Seems OK.
yep, matches to [1]. All other error paths out of this state no
longer set meta_valid, which saves me the effort of having to
open-code cleanup into each of those points.
> diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
> index aaf75b8..f18c999 100644
> --- a/generator/states-reply-structured.c
> +++ b/generator/states-reply-structured.c
> @@ -445,6 +445,7 @@ STATE_MACHINE {
> assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent));
> assert (h->bs_entries);
> assert (length >= 12);
> + assert (h->meta_valid);
>
> /* Need to byte-swap the entries returned, but apart from that we
> * don't validate them.
This seems to express that we may only have initiated a block status
request (for which we are now processing the replies --
REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES) in case we had a valid meta
context list first. Seems plausible. (A bit lower down in the context,
not quoted, we "Look up the context ID" in the meta context vector, so
yes, very sensible assertion.)
Yeah, the assertion is useful to prove we aren't reading a
half-populated h->meta_contexts vector left half-populated after a
failed NBD_OPT_SET_META_CONTEXT; and it is met because
nbd_unlocked_aio_block_status() checked before ever issuing
NBD_CMD_BLOCK_STATUS.
> diff --git a/lib/flags.c b/lib/flags.c
> index 87c20ee..91efc1a 100644
> --- a/lib/flags.c
> +++ b/lib/flags.c
> @@ -37,9 +37,7 @@ nbd_internal_reset_size_and_flags (struct nbd_handle *h)
>
> h->exportsize = 0;
> h->eflags = 0;
> - for (i = 0; i < h->meta_contexts.len; ++i)
> - free (h->meta_contexts.ptr[i].name);
> - meta_vector_reset (&h->meta_contexts);
Moved to NEWSTYLE.OPT_META_CONTEXT.START; OK.
This is [4]; it is actually moved to both
NEWSTYLE.OPT_META_CONTEXT.START [5], and to handle cleanup [6].
(This is the only place we reset the meta context vector, pre-patch.)
> + h->meta_valid = false;
Replacing the above logic, OK.
Yes. As of this patch, calling nbd_internal_reset_size_and_flags()
still wipes both size and contexts state, unchanged from status quo
before; but later patches in the series separate the wipe of contexts
state to other locations.
> h->block_minimum = 0;
> h->block_preferred = 0;
> h->block_maximum = 0;
> @@ -75,6 +73,11 @@ 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) {
> + assert (h->meta_contexts.len == 0);
> + h->meta_valid = true;
> + }
> +
> h->exportsize = exportsize;
> h->eflags = eflags;
> return 0;
Hmmm. This gives me a run for my money...
I think the condition here describes the case when we're never going to
send an NBD_OPT_SET_META_CONTEXT -- meaning that we're never going to
reach the "meta_valid = true" assignment in the ACK handling for
NBD_OPT_SET_META_CONTEXT. Still we need to qualify our empty metacontext
vector as valid somewhere, so nbd_internal_set_size_and_flags() is being
proposed as the location for that.
The question is where nbd_internal_set_size_and_flags() is called
*from*. I find:
- NEWSTYLE.OPT_EXPORT_NAME.CHECK_REPLY
- NBD_REP_INFO | NBD_INFO_EXPORT
- OLDSTYLE.CHECK
We also have a comment on nbd_internal_set_size_and_flags() saying
/* Set the export size and eflags on the handle, validating them.
* This is called from the state machine when either the newstyle or
* oldstyle negotiation reaches the point that these are available.
*/
Aha! So, also in accordance with the commit message, I need to replace
"we're never going to reach the ACK for NBD_OPT_SET_META_CONTEXT" with
"we could never have reached the ACK for NBD_OPT_SET_META_CONTEXT".
I'm not sure if this is the *only place* (or the *best place*) to set
the witness [*], but it does look like a "good one".
This is point [2] in the commit message. Yeah, my alternatives were
to either find common code that is reached by both oldstyle and
OPT_EXPORT_NAME code (the two cases where we can't even attempt
NBD_OPT_SET_META_CONTEXT) - which is this function, or to open-code
the same effect into both callers. I chose to put it once in the
common code, in part because...
[*] The commit message speaks about a "latent semantic difference" which
is currently masked by our managing meta_valid and eflags strictly in
tandem. Hopefully I'll understand that paragraph better when looking at
later patches in the series.
... later in the series (4/12), this gets tweaked again by further
gating the logic here to only set h->meta_valid=true when
h->request_meta is set, rather than having to tweak all three points
that transition into transmission phase (even though only one of those
three points could have even attempted NBD_OPT_SET_META_CONTEXT), per
point [3].
> @@ -194,9 +197,8 @@ nbd_unlocked_can_meta_context (struct nbd_handle *h, const char
*name)
> {
> size_t i;
>
> - if (h->eflags == 0) {
> - set_error (EINVAL, "server has not returned export flags, "
> - "you need to connect to the server first");
> + if (h->request_meta_contexts.len && !h->meta_valid) {
> + set_error (EINVAL, "need a successful server meta context request
first");
> return -1;
> }
This is not easy to understand at first, but I kind of convinced myself
with the following argument: the condition for successfully querying the
context list is
h->request_meta_contexts.len == 0 || h->meta_valid
i.e., we never needed to ask the server, or the server responded (and we
parsed the response successfully). The *result* list may or may not be
empty at this point (it could be empty for two reasons: we never asked
for any contexts, or we did but the server supports none); either way,
the result list is valid for searching.
OK.
Correct reading.
>
> diff --git a/lib/handle.c b/lib/handle.c
> index 8713e18..03f45a4 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -115,6 +115,8 @@ nbd_create (void)
> void
> nbd_close (struct nbd_handle *h)
> {
> + size_t i;
> +
> nbd_internal_set_error_context ("nbd_close");
>
> if (h == NULL)
> @@ -127,6 +129,9 @@ nbd_close (struct nbd_handle *h)
>
> free (h->bs_entries);
> 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);
> nbd_internal_free_option (h);
> free_cmd_list (h->cmds_to_issue);
> free_cmd_list (h->cmds_in_flight);
OK.
This is [6], needed because it was moved out of [4].
> diff --git a/lib/rw.c b/lib/rw.c
> index 81f8f35..2ab85f3 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -482,7 +482,7 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h,
> return -1;
> }
>
> - if (h->meta_contexts.len == 0) {
> + if (!h->meta_valid || h->meta_contexts.len == 0) {
> set_error (ENOTSUP, "did not negotiate any metadata contexts, "
> "either you did not call nbd_add_meta_context before "
> "connecting or the server does not support it");
>
Conversely, here the condition for passing is
h->meta_valid && h->meta_contexts.len > 0
meaning we asked the server, and it supports at least one of the
requested meta contexts.
I think the patch is correct, but whether it is *complete* as well is
something I can't easily evaluate. With that caveat:
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
Thank you for a thorough review, and I'm glad my thorough commit
message helped.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org