Instead of open-coding a linked list, we can utilize our vector.h helpers. No semantic change intended. --- lib/internal.h | 17 +++++---- generator/states-newstyle-opt-meta-context.c | 36 ++++++++------------ generator/states-reply-structured.c | 10 +++--- lib/flags.c | 19 ++++------- lib/rw.c | 2 +- 5 files changed, 35 insertions(+), 49 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 0a61b15..d601d5d 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -64,10 +64,15 @@ */ #define MAX_REQUEST_SIZE (64 * 1024 * 1024) -struct meta_context; struct socket; struct command; +struct meta_context { + char *name; /* Name of meta context. */ + uint32_t context_id; /* Context ID negotiated with the server. */ +}; +DEFINE_VECTOR_TYPE(meta_vector, struct meta_context); + struct export { char *name; char *description; @@ -174,8 +179,8 @@ struct nbd_handle { bool structured_replies; /* If we negotiated NBD_OPT_STRUCTURED_REPLY */ - /* Linked list of negotiated metadata contexts. */ - struct meta_context *meta_contexts; + /* Vector of negotiated metadata contexts. */ + meta_vector meta_contexts; /* The socket or a wrapper if using GnuTLS. */ struct socket *sock; @@ -311,12 +316,6 @@ struct nbd_handle { bool tls_shut_writes; /* Used by lib/crypto.c to track disconnect. */ }; -struct meta_context { - struct meta_context *next; /* Linked list. */ - char *name; /* Name of meta context. */ - uint32_t context_id; /* Context ID negotiated with the server. */ -}; - struct socket_ops { ssize_t (*recv) (struct nbd_handle *h, struct socket *sock, void *buf, size_t len); diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 30b9617..8f4dee2 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * 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 @@ -44,7 +44,7 @@ STATE_MACHINE { } } - assert (h->meta_contexts == NULL); + assert (h->meta_contexts.len == 0); /* Calculate the length of the option request data. */ len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries */; @@ -176,7 +176,7 @@ STATE_MACHINE { uint32_t reply; uint32_t len; const size_t maxpayload = sizeof h->sbuf.or.payload.context; - struct meta_context *meta_context; + struct meta_context meta_context; uint32_t opt; int err = 0; @@ -202,33 +202,27 @@ STATE_MACHINE { debug (h, "skipping too large meta context"); else { assert (len > sizeof h->sbuf.or.payload.context.context.context_id); - meta_context = malloc (sizeof *meta_context); - if (meta_context == NULL) { - set_error (errno, "malloc"); - SET_NEXT_STATE (%.DEAD); - return 0; - } - meta_context->context_id = + meta_context.context_id = be32toh (h->sbuf.or.payload.context.context.context_id); /* String payload is not NUL-terminated. */ - meta_context->name = strndup (h->sbuf.or.payload.context.str, - len - sizeof meta_context->context_id); - if (meta_context->name == NULL) { + meta_context.name = strndup (h->sbuf.or.payload.context.str, + len - sizeof meta_context.context_id); + if (meta_context.name == NULL) { set_error (errno, "strdup"); SET_NEXT_STATE (%.DEAD); - free (meta_context); return 0; } debug (h, "negotiated %s with context ID %" PRIu32, - meta_context->name, meta_context->context_id); + meta_context.name, meta_context.context_id); if (opt == NBD_OPT_LIST_META_CONTEXT) { - CALL_CALLBACK (h->opt_cb.fn.context, meta_context->name); - free (meta_context->name); - free (meta_context); + CALL_CALLBACK (h->opt_cb.fn.context, meta_context.name); + free (meta_context.name); } - else { - meta_context->next = h->meta_contexts; - h->meta_contexts = meta_context; + else if (meta_vector_append (&h->meta_contexts, meta_context) == -1) { + set_error (errno, "realloc"); + free (meta_context.name); + SET_NEXT_STATE (%.DEAD); + return 0; } } SET_NEXT_STATE (%PREPARE_FOR_REPLY); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 19d9fb2..aaf75b8 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -454,18 +454,16 @@ STATE_MACHINE { /* Look up the context ID. */ context_id = h->bs_entries[0]; - for (meta_context = h->meta_contexts; - meta_context; - meta_context = meta_context->next) - if (context_id == meta_context->context_id) + for (i = 0; i < h->meta_contexts.len; ++i) + if (context_id == h->meta_contexts.ptr[i].context_id) break; - if (meta_context) { + if (i < h->meta_contexts.len) { /* Call the caller's extent function. */ int error = cmd->error; if (CALL_CALLBACK (cmd->cb.fn.extent, - meta_context->name, cmd->offset, + h->meta_contexts.ptr[i].name, cmd->offset, &h->bs_entries[1], (length-4) / 4, &error) == -1) if (cmd->error == 0) diff --git a/lib/flags.c b/lib/flags.c index 44d61c8..87c20ee 100644 --- a/lib/flags.c +++ b/lib/flags.c @@ -33,16 +33,13 @@ void nbd_internal_reset_size_and_flags (struct nbd_handle *h) { - struct meta_context *m, *m_next; + size_t i; h->exportsize = 0; h->eflags = 0; - for (m = h->meta_contexts; m != NULL; m = m_next) { - m_next = m->next; - free (m->name); - free (m); - } - h->meta_contexts = NULL; + for (i = 0; i < h->meta_contexts.len; ++i) + free (h->meta_contexts.ptr[i].name); + meta_vector_reset (&h->meta_contexts); h->block_minimum = 0; h->block_preferred = 0; h->block_maximum = 0; @@ -195,7 +192,7 @@ nbd_unlocked_can_cache (struct nbd_handle *h) int nbd_unlocked_can_meta_context (struct nbd_handle *h, const char *name) { - struct meta_context *meta_context; + size_t i; if (h->eflags == 0) { set_error (EINVAL, "server has not returned export flags, " @@ -203,10 +200,8 @@ nbd_unlocked_can_meta_context (struct nbd_handle *h, const char *name) return -1; } - for (meta_context = h->meta_contexts; - meta_context; - meta_context = meta_context->next) - if (strcmp (meta_context->name, name) == 0) + for (i = 0; i < h->meta_contexts.len; ++i) + if (strcmp (h->meta_contexts.ptr[i].name, name) == 0) return 1; return 0; } diff --git a/lib/rw.c b/lib/rw.c index f32481a..81f8f35 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 == NULL) { + if (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"); -- 2.37.2

If a server replies to NBD_OPT_SET_META_CONTEXT first with NBD_REP_META_CONTEXT, then with NBD_REP_ERR_*, we have already modified h->meta_contexts, but fail to clean it up before moving on to OPT_GO in the OPT_META_CONTEXT.CHECK_REPLY's default: handler (this corner-case bug has been present since we got meta context support working back in commits a97e2779 and 1b560b62). Per the NBD spec, because the overall SET_META_CONTEXT did not succeed, we should therefore not attempt NBD_CMD_BLOCK_STATUS; but because h->meta_contexts was left non-empty, we mistakenly report nbd_can_meta_context(first_string) as true, and allow nbd_block_status() to proceed; this forms a protocol violation not pre-filtered by our usual litany of nbd_set_strict_mode() checks, and therefore we can trigger unspecified server behavior. 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 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 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 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 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. 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 done when starting a new NBD_OPT_SET_META_CONTEXT or when closing struct nbd_handle. There are still some oddities to be cleaned up in 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 contexts, but the act of NBD_OPT_GO should not wipe contexts, and the act of NBD_OPT_SET_META_CONTEXT should not wipe flags), but I'm leaving those further tweaks for later patches to make this one easier to review. Testing this is surprisingly tricky; compliant servers don't do this (hence I don't really have anything automatic to add to libnbd's testsuite). However, I came up with the following temporary hack to nbdkit to demonstrate the problem: | diff --git i/server/protocol-handshake-newstyle.c w/server/protocol-handshake-newstyle.c | index fedee48f..b3f34c98 100644 | --- i/server/protocol-handshake-newstyle.c | +++ w/server/protocol-handshake-newstyle.c | @@ -884,7 +884,7 @@ negotiate_handshake_newstyle_options (void) | opt_index += querylen; | nr_queries--; | } | - if (send_newstyle_option_reply (option, NBD_REP_ACK) == -1) | + if (send_newstyle_option_reply (option, NBD_REP_ERR_POLICY) == -1) | return -1; | } | debug ("newstyle negotiation: %s: reply complete", optname); | diff --git i/server/protocol.c w/server/protocol.c | index 2ac77055..97235bda 100644 | --- i/server/protocol.c | +++ w/server/protocol.c | @@ -191,7 +191,7 @@ validate_request (uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, | | /* Block status allowed? */ | if (cmd == NBD_CMD_BLOCK_STATUS) { | - if (!conn->structured_replies) { | + if (!conn->structured_replies || 1) { | nbdkit_error ("invalid request: " | "%s: structured replies was not negotiated", | name_of_nbd_cmd (cmd)); Coupled with this command line: $ ./run /path/to/nbdkit -U- memory 1 --run 'nbdsh --base -u "$uri" -c " try: print(h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION)) except nbd.Error as ex: print(ex.string) def f(c, o, e, er): pass try: print(h.block_status(1, 0, f)) except nbd.Error as ex: print(ex.string) "' Pre-patch, we see can_meta_context succeed, and that we let NBD_CMD_BLOCK_STATUS leak through causing an nbdkit error message: True nbdkit: memory.0: error: invalid request: NBD_CMD_BLOCK_STATUS: structured replies was not negotiated nbd_block_status: block-status: command failed: Invalid argument Post-patch, can_meta_context diagnoses the problem, and block_status is blocked client-side with no messages needed from nbdkit: nbd_can_meta_context: need a successful server meta context request first: Invalid argument nbd_block_status: did not negotiate any metadata contexts, either you did not call nbd_add_meta_context before connecting or the server does not support it: Operation not supported Fixes: a97e2779 ("states: Negotiate base:allocation with the server.", v0.1) --- lib/internal.h | 1 + generator/API.ml | 4 +++- generator/states-newstyle-opt-meta-context.c | 9 +++++++-- generator/states-reply-structured.c | 1 + lib/flags.c | 14 ++++++++------ lib/handle.c | 5 +++++ lib/rw.c | 2 +- 7 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index d601d5d..8aaff15 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -180,6 +180,7 @@ struct nbd_handle { bool structured_replies; /* If we negotiated NBD_OPT_STRUCTURED_REPLY */ /* Vector of negotiated metadata contexts. */ + bool meta_valid; meta_vector meta_contexts; /* The socket or a wrapper if using GnuTLS. */ diff --git a/generator/API.ml b/generator/API.ml index 3e948aa..62e2d54 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -1785,7 +1785,9 @@ "can_meta_context", { longdesc = "\ Returns true if the server supports the given meta context (see L<nbd_add_meta_context(3)>). Returns false if -the server does not. +the server does not. It is possible for this command to fail if +meta contexts were requested but there is a missing or failed +attempt at NBD_OPT_SET_META_CONTEXT during option negotiation. The single parameter is the name of the metadata context, for example C<LIBNBD_CONTEXT_BASE_ALLOCATION>. diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index 8f4dee2..a6a5271 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ 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); 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); /* 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) 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. 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); + h->meta_valid = false; 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; @@ -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; } 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); 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"); -- 2.37.2

On Thu, Sep 01, 2022 at 11:04:09AM +0200, Laszlo Ersek wrote: ... [1] [2] [3] patch 4/12 adds a test for this patch 11/12 adds a test for this [4] [5] [6] patch 9/12 patch 4/12 patch 7/12 ... yep, this is the wipe [5] moved here because it was taken out of [4] yep, matches up with [4] 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. 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. This is [4]; it is actually moved to both NEWSTYLE.OPT_META_CONTEXT.START [5], and to handle cleanup [6]. 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. 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... ... 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]. Correct reading. This is [6], needed because it was moved out of [4]. 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

On 08/31/22 16:39, Eric Blake wrote: Can we introduce some xfuncname pattern for these "state machine" C files? The STATE_MACHINE hunk header is totally useless. I'd like "NEWSTYLE.OPT_META_CONTEXT.START". :) Regarding the code -- with the removal of the assertion from the LIST branch, but preserving a similar check (albeit not an assert()) on the SET branch, the comment covering *both* branches is now out of date: /* If the server doesn't support SRs then we must skip this group. On a tangent: why do we have nbd_opt_abort() separately from nbd_close()? What further steps would be valid between the two? Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;

On Thu, Sep 01, 2022 at 11:21:38AM +0200, Laszlo Ersek wrote: Yes, that's a side ask, but I would love it too. Git has a default xfuncname pattern for .c files, so it will be interesting to see if I can figure out .gitattributes that would let us attach additional patterns on to the generator/*.c files without penalizing normal .c files. Indeed it is. I already posted a followup patch to 11/12 where I improved the comment to its final state; I will grab the appropriate parts of that comment and revise it through the series to match changes as they come in. For 2/12, that results in: diff --git c/generator/states-newstyle-opt-meta-context.c w/generator/states-newstyle-opt-meta-context.c index a6a5271a..dd5a6ded 100644 --- c/generator/states-newstyle-opt-meta-context.c +++ w/generator/states-newstyle-opt-meta-context.c @@ -23,9 +23,27 @@ STATE_MACHINE { size_t i; uint32_t len, opt; - /* If the server doesn't support SRs then we must skip this group. - * Also we skip the group if the client didn't request any metadata - * contexts, when doing SET (but an empty LIST is okay). + /* This state group is reached from: + * h->opt_mode == false (h->current_opt == 0): + * nbd_connect_*() + * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO + * h->opt_mode == true (h->current_opt matches calling API): + * nbd_opt_info() + * -> conditionally use SET, next state OPT_GO for NBD_OPT_INFO + * nbd_opt_go() + * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO + * nbd_opt_list_meta_context() + * -> conditionally 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. + * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. + * If SET is conditional, we skip it if structured replies were + * not negotiated, or if there were no contexts to request. + * If OPT_GO is later successful, it populates h->exportsize and friends, + * and also sets h->meta_valid if we skipped SET here. + * LIST is conditional, skipped if structured replies were not negotiated. + * 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 this patch, it changes as: diff --git c/generator/states-newstyle-opt-meta-context.c w/generator/states-newstyle-opt-meta-context.c index c45ff129..020a7adf 100644 --- c/generator/states-newstyle-opt-meta-context.c +++ w/generator/states-newstyle-opt-meta-context.c @@ -33,7 +33,7 @@ STATE_MACHINE { * nbd_opt_go() * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO * nbd_opt_list_meta_context() - * -> conditionally use LIST, next state NEGOTIATING + * -> 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. @@ -42,8 +42,7 @@ STATE_MACHINE { * not negotiated, or if there were no contexts to request. * If OPT_GO is later successful, it populates h->exportsize and friends, * and also sets h->meta_valid if we skipped SET here. - * LIST is conditional, skipped if structured replies were not negotiated. - * There is a callback if and only if the command is LIST. + * There is a callback if and only if the command is unconditional. */ assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); nbd_internal_reset_size_and_flags (h); In 4/12, it is further modified: diff --git c/generator/states-newstyle-opt-meta-context.c w/generator/states-newstyle-opt-meta-context.c index 37022594..28100199 100644 --- c/generator/states-newstyle-opt-meta-context.c +++ w/generator/states-newstyle-opt-meta-context.c @@ -35,13 +35,12 @@ STATE_MACHINE { * 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. - * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. - * 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. * 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. + * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. * There is a callback if and only if the command is unconditional. */ assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); And having typed that up, I'm now starting to think I should tweak patch 7/12 to move the call to nbd_internal_reset_size_and_flags() out of NEWSTYLE.OPT_META_CONTEXT.START and into NEWSTYLE.OPT_GO.START, to further solidify that it is the act of NBD_OPT_INFO/GO that wipes h->exportsize, not the act of NBD_OPT_SET/LIST_META_CONTEXT. ... nbd_opt_abort() is optional; it says to inform the server of our intent to do a clean disconnect during negotiation. If you skip it and just do nbd_close(), the server is more likely to see an unexpected EOF and warn that we went away unexpectedly. It is a direct counterpart to nbd_shutdown(0) informing the server that we want a clean disconnect during transmission. As for what we can do between the two - nbd_opt_abort() generally moves the handle to state CLOSED, but there you can still do things like nbd_get_size() to see what had happened while the handle was alive; while nbd_close() wipes the handle altogether. Should I apply your R-b with my incremental doc tweaks incorporated along the way, or are you going to want to see a v3 respin with everything in one place (especially now that I'm thinking of further tweaks in 7/12)? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Thu, Sep 01, 2022 at 05:25:55PM +0100, Richard W.M. Jones wrote: Probably also want _0-9 in that []. We could also put it in .gitattributes (we've already done it for *.ml files, where we give the user the 'git config' command to run to take advantage of it). It's a shame it's not quite automatic from a fresh git checkout, but the idea should work if done right. Here's what I tested. In .gitattributes: # Use the following one-time config to get nicer diffs of generator state files: # git config diff.states.xfuncname '^([a-zA-Z_]| [A-Z._0-9]*:$)' generator/states-*.c diff=states then run that git config, then looking at 'git log -p generator/states-*.c', the diff for 30ee21f6 is a lot more legible, with lines like: +++ b/generator/states-reply-structured.c @@ -89,13 +89,8 @@ REPLY.STRUCTURED_REPLY.CHECK: } /* Skip an unexpected structured reply, including to an unknown cookie. */ - if (cmd == NULL || !h->structured_replies) { where the @@ line points to the right sub-state. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Wed, Aug 31, 2022 at 09:39:19AM -0500, Eric Blake wrote: In theory you could parse nbdkit --dump-config, although I agree this approach is fine too. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

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 */; 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 +# +# The opt-set-meta program cannot be directly executed until all the libtool +# libraries that it depends on are installed. +# +# This wrapper script should never be moved out of the build directory. +# If it is, it will not operate correctly. + +# Sed substitution that helps us do robust quoting. It backslashifies +# metacharacters that are still active within double-quoted strings. +sed_quote_subst='s|\([`"$\\]\)|\\\1|g' + +# Be Bourne compatible +if test -n "${ZSH_VERSION+set}" && (emulate sh) >/dev/null 2>&1; then + emulate sh + NULLCMD=: + # Zsh 3.x and 4.x performs word splitting on ${1+"$@"}, which + # is contrary to our usage. Disable this feature. + alias -g '${1+"$@"}'='"$@"' + setopt NO_GLOB_SUBST +else + case `(set -o) 2>/dev/null` in *posix*) set -o posix;; esac +fi +BIN_SH=xpg4; export BIN_SH # for Tru64 +DUALCASE=1; export DUALCASE # for MKS sh + +# The HP-UX ksh and POSIX shell print the target directory to stdout +# if CDPATH is set. +(unset CDPATH) >/dev/null 2>&1 && unset CDPATH + +relink_command="" + +# This environment variable determines our operation mode. +if test "$libtool_install_magic" = "%%%MAGIC variable%%%"; then + # install mode needs the following variables: + generated_by_libtool_version='2.4.6' + notinst_deplibs=' ../lib/libnbd.la' +else + # When we are sourced in execute mode, $file and $ECHO are already set. + if test "$libtool_execute_magic" != "%%%MAGIC variable%%%"; then + file="$0" + +# A function that is used when there is no print builtin or printf. +func_fallback_echo () +{ + eval 'cat <<_LTECHO_EOF +$1 +_LTECHO_EOF' +} + ECHO="printf %s\\n" + fi + +# Very basic option parsing. These options are (a) specific to +# the libtool wrapper, (b) are identical between the wrapper +# /script/ and the wrapper /executable/ that is used only on +# windows platforms, and (c) all begin with the string --lt- +# (application programs are unlikely to have options that match +# this pattern). +# +# There are only two supported options: --lt-debug and +# --lt-dump-script. There is, deliberately, no --lt-help. +# +# The first argument to this parsing function should be the +# script's ../libtool value, followed by no. +lt_option_debug= +func_parse_lt_options () +{ + lt_script_arg0=$0 + shift + for lt_opt + do + case "$lt_opt" in + --lt-debug) lt_option_debug=1 ;; + --lt-dump-script) + lt_dump_D=`$ECHO "X$lt_script_arg0" | /usr/bin/sed -e 's/^X//' -e 's%/[^/]*$%%'` + test "X$lt_dump_D" = "X$lt_script_arg0" && lt_dump_D=. + lt_dump_F=`$ECHO "X$lt_script_arg0" | /usr/bin/sed -e 's/^X//' -e 's%^.*/%%'` + cat "$lt_dump_D/$lt_dump_F" + exit 0 + ;; + --lt-*) + $ECHO "Unrecognized --lt- option: '$lt_opt'" 1>&2 + exit 1 + ;; + esac + done + + # Print the debug banner immediately: + if test -n "$lt_option_debug"; then + echo "opt-set-meta:opt-set-meta:$LINENO: libtool wrapper (GNU libtool) 2.4.6" 1>&2 + fi +} + +# Used when --lt-debug. Prints its arguments to stdout +# (redirection is the responsibility of the caller) +func_lt_dump_args () +{ + lt_dump_args_N=1; + for lt_arg + do + $ECHO "opt-set-meta:opt-set-meta:$LINENO: newargv[$lt_dump_args_N]: $lt_arg" + lt_dump_args_N=`expr $lt_dump_args_N + 1` + done +} + +# Core function for launching the target application +func_exec_program_core () +{ + + if test -n "$lt_option_debug"; then + $ECHO "opt-set-meta:opt-set-meta:$LINENO: newargv[0]: $progdir/$program" 1>&2 + func_lt_dump_args ${1+"$@"} 1>&2 + fi + exec "$progdir/$program" ${1+"$@"} + + $ECHO "$0: cannot exec $program $*" 1>&2 + exit 1 +} + +# A function to encapsulate launching the target application +# Strips options in the --lt-* namespace from $@ and +# launches target application with the remaining arguments. +func_exec_program () +{ + case " $* " in + *\ --lt-*) + for lt_wr_arg + do + case $lt_wr_arg in + --lt-*) ;; + *) set x "$@" "$lt_wr_arg"; shift;; + esac + shift + done ;; + esac + func_exec_program_core ${1+"$@"} +} + + # Parse options + func_parse_lt_options "$0" ${1+"$@"} + + # Find the directory that this script lives in. + thisdir=`$ECHO "$file" | /usr/bin/sed 's%/[^/]*$%%'` + test "x$thisdir" = "x$file" && thisdir=. + + # Follow symbolic links until we get to the real thisdir. + file=`ls -ld "$file" | /usr/bin/sed -n 's/.*-> //p'` + while test -n "$file"; do + destdir=`$ECHO "$file" | /usr/bin/sed 's%/[^/]*$%%'` + + # If there was a directory component, then change thisdir. + if test "x$destdir" != "x$file"; then + case "$destdir" in + [\\/]* | [A-Za-z]:[\\/]*) thisdir="$destdir" ;; + *) thisdir="$thisdir/$destdir" ;; + esac + fi + + file=`$ECHO "$file" | /usr/bin/sed 's%^.*/%%'` + file=`ls -ld "$thisdir/$file" | /usr/bin/sed -n 's/.*-> //p'` + done + + # Usually 'no', except on cygwin/mingw when embedded into + # the cwrapper. + WRAPPER_SCRIPT_BELONGS_IN_OBJDIR=no + if test "$WRAPPER_SCRIPT_BELONGS_IN_OBJDIR" = "yes"; then + # special case for '.' + if test "$thisdir" = "."; then + thisdir=`pwd` + fi + # remove .libs from thisdir + case "$thisdir" in + *[\\/].libs ) thisdir=`$ECHO "$thisdir" | /usr/bin/sed 's%[\\/][^\\/]*$%%'` ;; + .libs ) thisdir=. ;; + esac + fi + + # Try to get the absolute directory name. + absdir=`cd "$thisdir" && pwd` + test -n "$absdir" && thisdir="$absdir" + + program='opt-set-meta' + progdir="$thisdir/.libs" + + + if test -f "$progdir/$program"; then + # Add our own library path to LD_LIBRARY_PATH + LD_LIBRARY_PATH="/home/eblake/libnbd/lib/.libs:$LD_LIBRARY_PATH" + + # Some systems cannot cope with colon-terminated LD_LIBRARY_PATH + # The second colon is a workaround for a bug in BeOS R4 sed + LD_LIBRARY_PATH=`$ECHO "$LD_LIBRARY_PATH" | /usr/bin/sed 's/::*$//'` + + export LD_LIBRARY_PATH + + if test "$libtool_execute_magic" != "%%%MAGIC variable%%%"; then + # Run the actual program with our arguments. + func_exec_program ${1+"$@"} + fi + else + # The program doesn't exist. + $ECHO "$0: error: '$progdir/$program' does not exist" 1>&2 + $ECHO "This script is just a wrapper for $program." 1>&2 + $ECHO "See the libtool documentation for more information." 1>&2 + exit 1 + fi +fi -- 2.37.2

On Thu, Sep 01, 2022 at 05:29:21PM +0100, Richard W.M. Jones wrote: Will do. Sounds like I have enough incremental things to improve that it will be worth a v3 series, although I'll continue to let more reviews come on in the rest of the series before I post that. I'm wondering whether it is easier to modify the existing examples/list-exports.c, or to write a second example. Over the course of this series, as more and more APIs are added, the example can cover more steps. By the end of the series, it would look something like: // error checking omitted here for brevity, but I'll probably add it in the example nbd = nbd_create (); nbd_set_tls (nbd, LIBNBD_TLS_DISABLE); // we'll manually enable tls later nbd_set_request_structured_reply (nbd, false); // we'll manually request SR later nbd_set_request_meta_context (nbd, false); // we'll do this ourselves nbd_set_opt_mode (nbd, true); nbd_connect_...(); if (!nbd_aio_is_negotiating (nbd)) fatal ("server does not support listing"); // Determine which exports (if any) are exposed without TLS printf ("unencrypted exports:\n"); nbd_opt_list (nbd, callback_exports); // Now enable TLS, and try again if (nbd_opt_starttls (nbd) != 0) fatal ("server does not support TLS"); printf ("encrypted exports:\n"); nbd_opt_list (nbd, callback_exports); printf ("which export to learn more about?"); get user's choice... nbd_set_export_name (nbd, name); if (nbd_opt_structured_reply (nbd) == 1) { printf ("metacontexts available:"); nbd_opt_list_meta_context (nbd, callback_contexts); while (1) { printf ("which context to request, or -1 to finish connecting"); get user's choice if (choice == -1) break; nbd_add_meta_context (nbd, choice); } nbd_opt_set_meta_context (nbd, callback_silent); } else printf ("metacontexts not available\n"); nbd_opt_go (nbd); You're right. Fixed for the nbd_supports_vsock() patch, since that one will beat this series in. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On 09/01/22 22:46, Richard W.M. Jones wrote: I'd be OK with the reviewed front of the series being merged, and then a v3 of the tail being posted (if that's not too much burden for Eric). I'm between patches #3 and #4, so if Eric agrees, I could resume my review with *v3* of patch#4. I can also continue reviewing this version if that's preferred. (The main reason I'm trying to review this series meticulously is to learn about libnbd for the long term; it's a learning investment on my part. It's not because I insist that the series not be merged without my approval! :)) Laszlo

On 08/31/22 16:39, Eric Blake wrote: 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: 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*? ... 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: 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. ... 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

On 08/31/22 16:39, Eric Blake wrote: Acked-by: Laszlo Ersek <lersek(a)redhat.com&gt;

On Wed, Aug 31, 2022 at 09:39:23AM -0500, Eric Blake wrote: Given the review comments on 3/12, I'm thinking of squashing in: diff --git c/generator/states-newstyle-opt-go.c w/generator/states-newstyle-opt-go.c index 1ca5f099..9b05bec1 100644 --- c/generator/states-newstyle-opt-go.c +++ w/generator/states-newstyle-opt-go.c @@ -22,6 +22,7 @@ STATE_MACHINE { NEWSTYLE.OPT_GO.START: uint16_t nrinfos = 0; + nbd_internal_reset_size_and_flags (h); if (h->request_block_size) nrinfos++; if (h->full_info) diff --git c/generator/states-newstyle-opt-meta-context.c w/generator/states-newstyle-opt-meta-context.c index 3e66b106..6149d61d 100644 --- c/generator/states-newstyle-opt-meta-context.c +++ w/generator/states-newstyle-opt-meta-context.c @@ -35,12 +35,12 @@ STATE_MACHINE { * nbd_opt_list_meta_context() * -> unconditionally use LIST, next state NEGOTIATING * - * 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. * If OPT_GO is later successful, it populates h->exportsize and friends, * and also sets h->meta_valid if h->request_meta but we skipped SET here. - * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. + * SET then manipulates h->meta_contexts, and sets h->meta_valid on + * success, while LIST is stateless. * There is a callback if and only if the command is unconditional. */ assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); @@ -52,7 +52,6 @@ STATE_MACHINE { else { assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); opt = NBD_OPT_SET_META_CONTEXT; - nbd_internal_reset_size_and_flags (h); if (h->request_meta) { for (i = 0; i < h->meta_contexts.len; ++i) free (h->meta_contexts.ptr[i].name); -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

Add test coverage for the previous patch not wiping state during nbd_opt_list_meta_context. The change is logically identical in each language that has the same unit test. --- python/t/240-opt-list-meta.py | 29 +++++++++- ocaml/tests/test_240_opt_list_meta.ml | 34 ++++++++++- tests/opt-list-meta.c | 76 +++++++++++++++++++++++-- golang/libnbd_240_opt_list_meta_test.go | 64 ++++++++++++++++++++- 4 files changed, 193 insertions(+), 10 deletions(-) diff --git a/python/t/240-opt-list-meta.py b/python/t/240-opt-list-meta.py index 8341f9c..591703a 100644 --- a/python/t/240-opt-list-meta.py +++ b/python/t/240-opt-list-meta.py @@ -1,5 +1,5 @@ # libnbd Python bindings -# Copyright (C) 2010-2020 Red Hat Inc. +# Copyright (C) 2010-2022 Red Hat Inc. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -31,6 +31,14 @@ def f(user_data, name): seen = True +def must_fail(f, *args, **kwds): + try: + f(*args, **kwds) + assert False + except nbd.Error: + pass + + h = nbd.NBD() h.set_opt_mode(True) h.connect_command(["nbdkit", "-s", "--exit-with-parent", "-v", @@ -65,10 +73,27 @@ assert r == 1 assert count == 1 assert seen -# Final pass: "base:" query should get at least "base:allocation" +# Fourth pass: opt_list_meta_context is stateless, so it should +# not wipe status learned during opt_info count = 0 seen = False +must_fail(h.can_meta_context, nbd.CONTEXT_BASE_ALLOCATION) +must_fail(h.get_size) +h.opt_info() +assert h.get_size() == 1024*1024 +assert h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION) is True h.clear_meta_contexts() +h.add_meta_context("x-nosuch:") +r = h.opt_list_meta_context(lambda *args: f(42, *args)) +assert r == 0 +assert count == 0 +assert not seen +assert h.get_size() == 1048576 +assert h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION) is True + +# Final pass: "base:" query should get at least "base:allocation" +count = 0 +seen = False h.add_meta_context("base:") r = h.opt_list_meta_context(lambda *args: f(42, *args)) assert r >= 1 diff --git a/ocaml/tests/test_240_opt_list_meta.ml b/ocaml/tests/test_240_opt_list_meta.ml index 639d33c..afab084 100644 --- a/ocaml/tests/test_240_opt_list_meta.ml +++ b/ocaml/tests/test_240_opt_list_meta.ml @@ -64,10 +64,42 @@ let assert (r = !count); assert !seen; - (* Final pass: "base:" query should get at least "base:allocation" *) + (* Fourth pass: opt_list_meta_context is stateless, so it should + * not wipe status learned during opt_info + *) count := 0; seen := false; + (try + let _ = NBD.can_meta_context nbd NBD.context_base_allocation in + assert false + with + NBD.Error (errstr, errno) -> () + ); + (try + let _ = NBD.get_size nbd in + assert false + with + NBD.Error (errstr, errno) -> () + ); + NBD.opt_info nbd; + let s = NBD.get_size nbd in + assert (s = 1048576_L); + let m = NBD.can_meta_context nbd NBD.context_base_allocation in + assert m; NBD.clear_meta_contexts nbd; + NBD.add_meta_context nbd "x-nosuch:"; + let r = NBD.opt_list_meta_context nbd (f 42) in + assert (r = 0); + assert (r = !count); + assert (not !seen); + let s = NBD.get_size nbd in + assert (s = 1048576_L); + let m = NBD.can_meta_context nbd NBD.context_base_allocation in + assert m; + + (* Final pass: "base:" query should get at least "base:allocation" *) + count := 0; + seen := false; NBD.add_meta_context nbd "base:"; let r = NBD.opt_list_meta_context nbd (f 42) in assert (r >= 1); diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c index ccf58fc..12d6f51 100644 --- a/tests/opt-list-meta.c +++ b/tests/opt-list-meta.c @@ -138,13 +138,79 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } + /* Fourth pass: nbd_opt_list_meta_context is stateless, so it should + * not wipe status learned during nbd_opt_info(). + */ + r = nbd_get_size (nbd); + if (r != -1) { + fprintf (stderr, "expecting get_size to fail, got %d\n", r); + exit (EXIT_FAILURE); + } + r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION); + if (r != -1) { + fprintf (stderr, "expecting can_meta_context to fail, got %d\n", r); + exit (EXIT_FAILURE); + } + if (nbd_opt_info (nbd) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + r = nbd_get_size (nbd); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1024*1024) { + fprintf (stderr, "expecting get_size of 1M, got %d\n", r); + exit (EXIT_FAILURE); + } + r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "expecting can_meta_context to succeed, got %d\n", r); + exit (EXIT_FAILURE); + } + if (nbd_clear_meta_contexts (nbd) == -1 || + nbd_add_meta_context (nbd, "x-nosuch:") == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + p = (struct progress) { .count = 0 }; + r = nbd_opt_list_meta_context (nbd, + (nbd_context_callback) { .callback = check, + .user_data = &p}); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 0 || p.count != 0 || p.seen) { + fprintf (stderr, "expecting no contexts, got %d\n", r); + exit (EXIT_FAILURE); + } + r = nbd_get_size (nbd); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1048576) { + fprintf (stderr, "expecting get_size of 1M, got %d\n", r); + exit (EXIT_FAILURE); + } + r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "expecting can_meta_context to succeed, got %d\n", r); + exit (EXIT_FAILURE); + } + /* Final pass: "base:" query should get at least "base:allocation" */ p = (struct progress) { .count = 0 }; - r = nbd_clear_meta_contexts (nbd); - if (r == -1) { - fprintf (stderr, "%s\n", nbd_get_error ()); - exit (EXIT_FAILURE); - } r = nbd_add_meta_context (nbd, "base:"); if (r == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); diff --git a/golang/libnbd_240_opt_list_meta_test.go b/golang/libnbd_240_opt_list_meta_test.go index d732275..47df787 100644 --- a/golang/libnbd_240_opt_list_meta_test.go +++ b/golang/libnbd_240_opt_list_meta_test.go @@ -1,5 +1,5 @@ /* libnbd golang tests - * Copyright (C) 2013-2021 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 @@ -118,13 +118,73 @@ func Test240OptListMeta(t *testing.T) { t.Fatalf("unexpected count after opt_list_meta_context") } - /* Final pass: "base:" query should get at least "base:allocation" */ + /* Fourth pass: opt_list_meta_context is stateless, so it should + * not wipe status learned during opt_info + */ count = 0 seen = false + _, err = h.GetSize() + if err == nil { + t.Fatalf("expected error") + } + _, err = h.CanMetaContext(context_base_allocation) + if err == nil { + t.Fatalf("expected error") + } + err = h.OptInfo() + if err != nil { + t.Fatalf("opt_info failed unexpectedly: %s", err) + } + size, err := h.GetSize() + if err != nil { + t.Fatalf("get_size failed unexpectedly: %s", err) + } + if size != 1048576 { + t.Fatalf("get_size gave wrong size") + } + meta, err := h.CanMetaContext(context_base_allocation) + if err != nil { + t.Fatalf("can_meta_context failed unexpectedly: %s", err) + } + if !meta { + t.Fatalf("unexpected count after can_meta_context") + } err = h.ClearMetaContexts() if err != nil { t.Fatalf("could not request clear_meta_contexts: %s", err) } + err = h.AddMetaContext("x-nosuch:") + if err != nil { + t.Fatalf("could not request add_meta_context: %s", err) + } + r, err = h.OptListMetaContext(func(name string) int { + return listmetaf(42, name) + }) + if err != nil { + t.Fatalf("could not request opt_list_meta_context: %s", err) + } + if r != 0 || r != count || seen { + t.Fatalf("unexpected count after opt_list_meta_context") + } + size, err = h.GetSize() + if err != nil { + t.Fatalf("get_size failed unexpectedly: %s", err) + } + if size != 1048576 { + t.Fatalf("get_size gave wrong size") + } + meta, err = h.CanMetaContext(context_base_allocation) + if err != nil { + t.Fatalf("can_meta_context failed unexpectedly: %s", err) + } + if !meta { + t.Fatalf("unexpected count after can_meta_context") + } + err = h.ClearMetaContexts() + + /* Final pass: "base:" query should get at least "base:allocation" */ + count = 0 + seen = false err = h.AddMetaContext("base:") if err != nil { t.Fatalf("could not request add_meta_context: %s", err) -- 2.37.2

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

Add test coverage for the sequences fixed in the previous patch, where changing the export name should clean up anything remembered about a former export name. The tests in other language bindings are logically equivalent. --- python/t/230-opt-info.py | 11 ++++++++--- ocaml/tests/test_230_opt_info.ml | 11 ++++++++--- tests/opt-info.c | 24 ++++++++++++++++++------ golang/libnbd_230_opt_info_test.go | 23 +++++++++++++++++------ 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/python/t/230-opt-info.py b/python/t/230-opt-info.py index 8aa47ae..fc22a97 100644 --- a/python/t/230-opt-info.py +++ b/python/t/230-opt-info.py @@ -45,17 +45,22 @@ assert h.get_size() == 0 assert h.is_read_only() is True assert h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION) is True -# info on something not present fails, wipes out prior info -h.set_export_name("a") -must_fail(h.opt_info) +# changing export wipes out prior info +h.set_export_name("b") must_fail(h.get_size) must_fail(h.is_read_only) must_fail(h.can_meta_context, nbd.CONTEXT_BASE_ALLOCATION) +# info on something not present fails +h.set_export_name("a") +must_fail(h.opt_info) + # info for a different export, with automatic meta_context disabled h.set_export_name("b") h.set_request_meta_context(False) h.opt_info() +# idempotent name change is no-op +h.set_export_name("b") assert h.get_size() == 1 assert h.is_read_only() is False must_fail(h.can_meta_context, nbd.CONTEXT_BASE_ALLOCATION) diff --git a/ocaml/tests/test_230_opt_info.ml b/ocaml/tests/test_230_opt_info.ml index ec735ff..0403d14 100644 --- a/ocaml/tests/test_230_opt_info.ml +++ b/ocaml/tests/test_230_opt_info.ml @@ -62,17 +62,22 @@ let let meta = NBD.can_meta_context nbd NBD.context_base_allocation in assert meta; - (* info on something not present fails, wipes out prior info *) - NBD.set_export_name nbd "a"; - fail_unary NBD.opt_info nbd; + (* changing export wipes out prior info *) + NBD.set_export_name nbd "b"; fail_unary NBD.get_size nbd; fail_unary NBD.is_read_only nbd; fail_binary NBD.can_meta_context nbd NBD.context_base_allocation; + (* info on something not present fails *) + NBD.set_export_name nbd "a"; + fail_unary NBD.opt_info nbd; + (* info for a different export, with automatic meta_context disabled *) NBD.set_export_name nbd "b"; NBD.set_request_meta_context nbd false; NBD.opt_info nbd; + (* idempotent name change is no-op *) + NBD.set_export_name nbd "b"; let size = NBD.get_size nbd in assert (size = 1L); let ro = NBD.is_read_only nbd in diff --git a/tests/opt-info.c b/tests/opt-info.c index 26de0ee..70028ff 100644 --- a/tests/opt-info.c +++ b/tests/opt-info.c @@ -17,6 +17,7 @@ */ /* Test behavior of nbd_opt_info. */ +/* See also unit test 230 in the various language ports. */ #include <config.h> @@ -80,15 +81,11 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - /* info on something not present fails, wipes out prior info */ - if (nbd_set_export_name (nbd, "a") == -1) { + /* changing export wipes out prior info */ + if (nbd_set_export_name (nbd, "b") == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } - if (nbd_opt_info (nbd) != -1) { - fprintf (stderr, "expecting error for opt_info\n"); - exit (EXIT_FAILURE); - } if (nbd_get_size (nbd) != -1) { fprintf (stderr, "expecting error for get_size\n"); exit (EXIT_FAILURE); @@ -102,6 +99,16 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } + /* info on something not present fails */ + if (nbd_set_export_name (nbd, "a") == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_opt_info (nbd) != -1) { + fprintf (stderr, "expecting error for opt_info\n"); + exit (EXIT_FAILURE); + } + /* 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 ()); @@ -115,6 +122,11 @@ main (int argc, char *argv[]) fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } + /* idempotent name change is no-op */ + if (nbd_set_export_name (nbd, "b") == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if ((r = nbd_get_size (nbd)) != 1) { fprintf (stderr, "expecting size of 1, got %" PRId64 "\n", r); exit (EXIT_FAILURE); diff --git a/golang/libnbd_230_opt_info_test.go b/golang/libnbd_230_opt_info_test.go index bc4cadf..e16f661 100644 --- a/golang/libnbd_230_opt_info_test.go +++ b/golang/libnbd_230_opt_info_test.go @@ -91,15 +91,11 @@ func Test230OptInfo(t *testing.T) { t.Fatalf("unexpected meta context") } - /* info on something not present fails, wipes out prior info */ - err = h.SetExportName("a") + /* changing export wipes out prior info */ + err = h.SetExportName("b") if err != nil { t.Fatalf("set export name failed unexpectedly: %s", err) } - err = h.OptInfo() - if err == nil { - t.Fatalf("expected error") - } _, err = h.GetSize() if err == nil { t.Fatalf("expected error") @@ -113,6 +109,16 @@ func Test230OptInfo(t *testing.T) { t.Fatalf("expected error") } + /* info on something not present fails */ + err = h.SetExportName("a") + if err != nil { + t.Fatalf("set export name failed unexpectedly: %s", err) + } + err = h.OptInfo() + if err == nil { + t.Fatalf("expected error") + } + /* info for a different export, with automatic meta_context disabled */ err = h.SetExportName("b") if err != nil { @@ -126,6 +132,11 @@ func Test230OptInfo(t *testing.T) { if err != nil { t.Fatalf("opt_info failed unexpectedly: %s", err) } + /* idempotent name change is no-op */ + err = h.SetExportName("b") + if err != nil { + t.Fatalf("set export name failed unexpectedly: %s", err) + } size, err = h.GetSize() if err != nil { t.Fatalf("size failed unexpectedly: %s", err) -- 2.37.2

Make it possible for the client to choose when to manually request meta contexts when using nbd_set_opt_mode(). By itself, this patch allows us to test how a server reacts to NBD_OPT_SET_META_CONTEXT without a previous negotiation of structured headers; then an upcoming patch will also add APIs for choosing when to negotiate OPT_STARTTLS and OPT_STRUCTURED_REPLY for even more fine-grained testing control. Most users won't need this API (the automatic defaults are good enough), but when writing a server, this level of integration testing can be useful. Overhaul the comment on OPT_META_CONTEXT.START to explain 5 different scenarios that this state group can be reached in, to better document which conditionals are in use later in the state machine: - h->current_opt == opt is a witness of whether we are doing a mandatory manual LIST/SET request with an expected callback and transition to NEGOTIATIONG, or a conditional SET request dependent on nbd_set_request_meta_context() and prior negotiation and transition to OPT_GO - h->current_opt == NBD_OPT_LIST_META_CONTEXT is a witness of whether we are stateless or modifying h->meta_contexts As this is a new API, the unit test in C is included in this patch; there is no point in splitting it as reordering the patches would not compile. However, porting the tests to other languages will be done in the next commit. --- Patches to add nbd_opt_starttls and nbd_opt_structured_reply are not written yet... --- generator/API.ml | 115 ++++++++- generator/states-newstyle-opt-meta-context.c | 38 +-- generator/states-newstyle.c | 1 + lib/opt.c | 47 +++- tests/Makefile.am | 9 + tests/opt-set-meta.c | 235 +++++++++++++++++++ .gitignore | 1 + 7 files changed, 420 insertions(+), 26 deletions(-) create mode 100644 tests/opt-set-meta.c diff --git a/generator/API.ml b/generator/API.ml index adafac6..24cbabc 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -762,17 +762,20 @@ "set_request_meta_context", { 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)>. +second call; integration testing of other servers may benefit +from manual invocation of L<nbd_opt_set_meta_context(3)> at +other times in the negotiation sequence; 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 "opt_list_meta_context"; Link "opt_set_meta_context"; Link "get_structured_replies_negotiated"; Link "get_request_meta_context"; Link "can_meta_context"]; }; @@ -1036,7 +1039,9 @@ "opt_go", { 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)>. +this can be suppressed with L<nbd_set_request_meta_context(3)>, +particularly if L<nbd_opt_set_meta_context(3)> was used earlier +in the negotiation sequence. If this fails, the server may still be in negotiation, where it is possible to attempt another option such as a different export name; @@ -1044,7 +1049,8 @@ "opt_go", { 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 "add_meta_context"; Link "set_request_meta_context"]; + Link "add_meta_context"; Link "set_request_meta_context"; + Link "opt_set_meta_context"]; }; "opt_abort", { @@ -1179,7 +1185,64 @@ "opt_list_meta_context", { a server may send a lengthy list."; see_also = [Link "set_opt_mode"; Link "aio_opt_list_meta_context"; Link "add_meta_context"; Link "clear_meta_contexts"; - Link "opt_go"; Link "set_export_name"]; + Link "opt_go"; Link "set_export_name"; + Link "opt_set_meta_context"]; + }; + + "opt_set_meta_context", { + default_call with + args = [ Closure context_closure ]; ret = RInt; + permitted_states = [ Negotiating ]; + shortdesc = "request the server to select specific meta contexts"; + longdesc = "\ +Request that the server supply all recognized meta contexts +registered through prior calls to L<nbd_add_meta_context(3)>, in +conjunction with the export previously specified by the most +recent L<nbd_set_export_name(3)> or L<nbd_connect_uri(3)>. +This can only be used if L<nbd_set_opt_mode(3)> enabled option +mode. Normally, this function is redundant, as L<nbd_opt_go(3)> +automatically does the same task if structured replies have +already been negotiated. But manual control over meta context +requests can be useful for fine-grained testing of how a server +handles unusual negotiation sequences. Often, use of this +function is coupled with L<nbd_set_request_meta_context(3)> to +bypass the automatic context request normally performed by +L<nbd_opt_go(3)>. + +The NBD protocol allows a client to decide how many queries to ask +the server. Rather than taking that list of queries as a parameter +to this function, libnbd reuses the current list of requested meta +contexts as set by L<nbd_add_meta_context(3)>; you can use +L<nbd_clear_meta_contexts(3)> to set up a different list of queries. +Since this function is primarily designed for testing servers, libnbd +does not prevent the use of this function on an empty list or when +L<nbd_set_request_structured_replies(3)> has disabled structured +replies, in order to see how a server behaves. + +The C<context> function is called once per server reply, with any +C<user_data> passed to this function, and with C<name> supplied by +the server. Additionally, each server name will remain visible through +L<nbd_can_meta_context(3)> until the next attempt at +L<nbd_set_export_name(3)> or L<nbd_opt_set_meta_context(3)>, as +well as L<nbd_opt_go(3)> or L<nbd_opt_info(3)> that trigger an +automatic meta context request. Remember that it is not safe to +call any C<nbd_*> APIs from within the context of the callback +function. At present, the return value of the callback is +ignored, although a return of -1 should be avoided. + +For convenience, when this function succeeds, it returns the number +of replies returned by the server. + +Not all servers understand this request, and even when it is understood, +the server might intentionally send an empty list because it does not +support the requested context, or may encounter a failure after +delivering partial results. Thus, this function may succeed even when +no contexts are reported, or may fail but have a non-empty list."; + see_also = [Link "set_opt_mode"; Link "aio_opt_set_meta_context"; + Link "add_meta_context"; Link "clear_meta_contexts"; + Link "opt_go"; Link "set_export_name"; + Link "opt_list_meta_context"; Link "set_request_meta_context"; + Link "can_meta_context"]; }; "add_meta_context", { @@ -1850,7 +1913,7 @@ "can_meta_context", { see_also = [SectionLink "Flag calls"; Link "opt_info"; Link "add_meta_context"; Link "block_status"; Link "aio_block_status"; - Link "set_request_meta_context"]; + Link "set_request_meta_context"; Link "opt_set_meta_context"]; }; "get_protocol", { @@ -2592,6 +2655,38 @@ "aio_opt_list_meta_context", { see_also = [Link "set_opt_mode"; Link "opt_list_meta_context"]; }; + "aio_opt_set_meta_context", { + default_call with + args = [ Closure context_closure ]; ret = RInt; + optargs = [ OClosure completion_closure ]; + permitted_states = [ Negotiating ]; + shortdesc = "request the server to select specific meta contexts"; + longdesc = "\ +Request that the server supply all recognized meta contexts +registered through prior calls to L<nbd_add_meta_context(3)>, in +conjunction with the export previously specified by the most +recent L<nbd_set_export_name(3)> or L<nbd_connect_uri(3)>. +This can only be used if L<nbd_set_opt_mode(3)> enabled option +mode. Normally, this function is redundant, as L<nbd_opt_go(3)> +automatically does the same task if structured replies have +already been negotiated. But manual control over meta context +requests can be useful for fine-grained testing of how a server +handles unusual negotiation sequences. Often, use of this +function is coupled with L<nbd_set_request_meta_context(3)> to +bypass the automatic context request normally performed by +L<nbd_opt_go(3)>. + +To determine when the request completes, wait for +L<nbd_aio_is_connecting(3)> to return false. Or supply the optional +C<completion_callback> which will be invoked as described in +L<libnbd(3)/Completion callbacks>, except that it is automatically +retired regardless of return value. Note that detecting whether the +server returns an error (as is done by the return value of the +synchronous counterpart) is only possible with a completion +callback."; + see_also = [Link "set_opt_mode"; Link "opt_set_meta_context"]; + }; + "aio_pread", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset" ]; @@ -3302,6 +3397,8 @@ let first_version = (* 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); + "opt_set_meta_context", (1, 14); + "aio_opt_set_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. diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index a281b05..03b63c6 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -34,10 +34,14 @@ STATE_MACHINE { opt = h->opt_current; } else { - assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); + if (h->opt_current == NBD_OPT_SET_META_CONTEXT) + assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context)); + else { + assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); + nbd_internal_reset_size_and_flags (h); + } opt = NBD_OPT_SET_META_CONTEXT; - nbd_internal_reset_size_and_flags (h); - if (h->request_meta) { + if (h->request_meta || h->opt_current == opt) { for (i = 0; i < h->meta_contexts.len; ++i) free (h->meta_contexts.ptr[i].name); meta_vector_reset (&h->meta_contexts); @@ -194,15 +198,15 @@ STATE_MACHINE { len = be32toh (h->sbuf.or.option_reply.replylen); switch (reply) { case NBD_REP_ACK: /* End of list of replies. */ - if (opt == NBD_OPT_LIST_META_CONTEXT) { - SET_NEXT_STATE (%.NEGOTIATING); - CALL_CALLBACK (h->opt_cb.completion, &err); - nbd_internal_free_option (h); - } - else { - SET_NEXT_STATE (%^OPT_GO.START); + if (opt == NBD_OPT_SET_META_CONTEXT) h->meta_valid = true; + if (opt == h->opt_current) { + SET_NEXT_STATE (%.NEGOTIATING); + CALL_CALLBACK (h->opt_cb.completion, &err); + nbd_internal_free_option (h); } + else + SET_NEXT_STATE (%^OPT_GO.START); break; case NBD_REP_META_CONTEXT: /* A context. */ if (len > maxpayload) @@ -221,10 +225,10 @@ STATE_MACHINE { } debug (h, "negotiated %s with context ID %" PRIu32, meta_context.name, meta_context.context_id); - if (opt == NBD_OPT_LIST_META_CONTEXT) { + if (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context)) CALL_CALLBACK (h->opt_cb.fn.context, meta_context.name); + if (opt == NBD_OPT_LIST_META_CONTEXT) free (meta_context.name); - } else if (meta_vector_append (&h->meta_contexts, meta_context) == -1) { set_error (errno, "realloc"); free (meta_context.name); @@ -235,25 +239,27 @@ STATE_MACHINE { SET_NEXT_STATE (%PREPARE_FOR_REPLY); break; default: - /* Anything else is an error, ignore it for SET, report it for LIST */ + /* Anything else is an error, report it for explicit LIST/SET, ignore it + * for automatic progress (nbd_connect_*, nbd_opt_info, nbd_opt_go). + */ if (handle_reply_error (h) == -1) { SET_NEXT_STATE (%.DEAD); return 0; } - if (opt == NBD_OPT_LIST_META_CONTEXT) { + if (opt == h->opt_current) { /* XXX Should we decode specific expected errors, like * REP_ERR_UNKNOWN to ENOENT or REP_ERR_TOO_BIG to ERANGE? */ err = ENOTSUP; set_error (err, "unexpected response, possibly the server does not " - "support listing contexts"); + "support meta contexts"); CALL_CALLBACK (h->opt_cb.completion, &err); nbd_internal_free_option (h); SET_NEXT_STATE (%.NEGOTIATING); } else { - debug (h, "handshake: unexpected error from " + debug (h, "handshake: ignoring unexpected error from " "NBD_OPT_SET_META_CONTEXT (%" PRIu32 ")", reply); SET_NEXT_STATE (%^OPT_GO.START); } diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c index 41e7cc3..62f2372 100644 --- a/generator/states-newstyle.c +++ b/generator/states-newstyle.c @@ -140,6 +140,7 @@ STATE_MACHINE { SET_NEXT_STATE (%PREPARE_OPT_ABORT); return 0; case NBD_OPT_LIST_META_CONTEXT: + case NBD_OPT_SET_META_CONTEXT: SET_NEXT_STATE (%OPT_META_CONTEXT.START); return 0; case 0: diff --git a/lib/opt.c b/lib/opt.c index d9114f4..2ad8c9e 100644 --- a/lib/opt.c +++ b/lib/opt.c @@ -32,7 +32,8 @@ nbd_internal_free_option (struct nbd_handle *h) { if (h->opt_current == NBD_OPT_LIST) FREE_CALLBACK (h->opt_cb.fn.list); - else if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) + else if (h->opt_current == NBD_OPT_LIST_META_CONTEXT || + h->opt_current == NBD_OPT_SET_META_CONTEXT) FREE_CALLBACK (h->opt_cb.fn.context); FREE_CALLBACK (h->opt_cb.completion); } @@ -215,6 +216,28 @@ nbd_unlocked_opt_list_meta_context (struct nbd_handle *h, return s.count; } +/* Issue NBD_OPT_SET_META_CONTEXT and wait for the reply. */ +int +nbd_unlocked_opt_set_meta_context (struct nbd_handle *h, + nbd_context_callback *context) +{ + struct context_helper s = { .context = *context }; + nbd_context_callback l = { .callback = context_visitor, .user_data = &s }; + nbd_completion_callback c = { .callback = context_complete, .user_data = &s }; + + if (nbd_unlocked_aio_opt_set_meta_context (h, &l, &c) == -1) + return -1; + + SET_CALLBACK_TO_NULL (*context); + if (wait_for_option (h) == -1) + return -1; + if (s.err) { + set_error (s.err, "server replied with error to set meta context request"); + return -1; + } + return s.count; +} + /* Issue NBD_OPT_GO (or NBD_OPT_EXPORT_NAME) without waiting. */ int nbd_unlocked_aio_opt_go (struct nbd_handle *h, @@ -301,3 +324,25 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle *h, debug (h, "option queued, ignoring state machine failure"); return 0; } + +/* Issue NBD_OPT_SET_META_CONTEXT without waiting. */ +int +nbd_unlocked_aio_opt_set_meta_context (struct nbd_handle *h, + nbd_context_callback *context, + nbd_completion_callback *complete) +{ + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) { + set_error (ENOTSUP, "server is not using fixed newstyle protocol"); + return -1; + } + + assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); + h->opt_cb.fn.context = *context; + SET_CALLBACK_TO_NULL (*context); + h->opt_cb.completion = *complete; + SET_CALLBACK_TO_NULL (*complete); + h->opt_current = NBD_OPT_SET_META_CONTEXT; + if (nbd_internal_run (h, cmd_issue) == -1) + debug (h, "option queued, ignoring state machine failure"); + return 0; +} diff --git a/tests/Makefile.am b/tests/Makefile.am index 7fc7dec..1c6f137 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -213,6 +213,7 @@ check_PROGRAMS += \ opt-list \ opt-info \ opt-list-meta \ + opt-set-meta \ connect-systemd-socket-activation \ connect-unix \ connect-tcp \ @@ -278,6 +279,7 @@ TESTS += \ opt-list \ opt-info \ opt-list-meta \ + opt-set-meta \ connect-systemd-socket-activation \ connect-unix \ connect-tcp \ @@ -533,6 +535,13 @@ opt_info_LDADD = $(top_builddir)/lib/libnbd.la opt_list_meta_SOURCES = opt-list-meta.c opt_list_meta_LDADD = $(top_builddir)/lib/libnbd.la +opt_set_meta_SOURCES = opt-set-meta.c +opt_set_meta_CPPFLAGS = \ + $(AM_CPPFLAGS) \ + -I$(top_srcdir)/common/include \ + $(NULL) +opt_set_meta_LDADD = $(top_builddir)/lib/libnbd.la + connect_systemd_socket_activation_SOURCES = \ connect-systemd-socket-activation.c \ requires.c \ diff --git a/tests/opt-set-meta.c b/tests/opt-set-meta.c new file mode 100644 index 0000000..01c9604 --- /dev/null +++ b/tests/opt-set-meta.c @@ -0,0 +1,235 @@ +/* NBD client library in userspace + * 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 + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Test behavior of nbd_opt_set_meta_context. */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdint.h> +#include <stdbool.h> +#include <inttypes.h> +#include <string.h> +#include <errno.h> + +#include <libnbd.h> + +#include "array-size.h" + +struct progress { + int count; + bool seen; +}; + +static int +check (void *user_data, const char *name) +{ + struct progress *p = user_data; + + p->count++; + if (strcmp (name, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) + p->seen = true; + return 0; +} + +int +main (int argc, char *argv[]) +{ + struct nbd_handle *nbd; + int r; + struct progress p; + /* Leave room for --no-sr in second process */ + char *args[] = { "nbdkit", "-s", "--exit-with-parent", "-v", + "memory", "size=1M", NULL, NULL }; + char *tmp; + + /* First process, with structured replies. Get into negotiating state. */ + nbd = nbd_create (); + if (nbd == NULL || + nbd_set_opt_mode (nbd, true) == -1 || + nbd_connect_command (nbd, args) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* No contexts negotiated yet; can_meta should be error if any requested */ + if ((r = nbd_get_structured_replies_negotiated (nbd)) != 1) { + fprintf (stderr, "structured replies got %d, expected 1\n", r); + exit (EXIT_FAILURE); + } + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 0) { + fprintf (stderr, "can_meta_context got %d, expected 0\n", r); + exit (EXIT_FAILURE); + } + if (nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != -1) { + fprintf (stderr, "can_meta_context got %d, expected -1\n", r); + exit (EXIT_FAILURE); + } + + /* FIXME: Once nbd_opt_structured_reply exists, check that set before + * SR fails server-side, then enable SR for rest of process. + */ + + /* nbdkit does not match wildcard for SET, even though it does for LIST */ + if (nbd_clear_meta_contexts (nbd) == -1 || + nbd_add_meta_context (nbd, "base:") == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + p = (struct progress) { .count = 0 }; + r = nbd_opt_set_meta_context (nbd, + (nbd_context_callback) { .callback = check, + .user_data = &p}); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != p.count || r != 0 || p.seen) { + fprintf (stderr, "inconsistent return value %d, expected %d\n", r, p.count); + exit (EXIT_FAILURE); + } + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 0) { + fprintf (stderr, "can_meta_context got %d, expected 0\n", r); + exit (EXIT_FAILURE); + } + + /* Negotiating with no contexts is not an error, but selects nothing */ + r = nbd_clear_meta_contexts (nbd); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + p = (struct progress) { .count = 0 }; + r = nbd_opt_set_meta_context (nbd, + (nbd_context_callback) { .callback = check, + .user_data = &p}); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 0 || p.count || p.seen) { + fprintf (stderr, "expecting set_meta to select nothing\n"); + exit (EXIT_FAILURE); + } + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 0) { + fprintf (stderr, "can_meta_context got %d, expected 0\n", r); + exit (EXIT_FAILURE); + } + + /* Request 2 with expectation of 1; with set_request_meta_context off */ + if (nbd_add_meta_context (nbd, "x-nosuch:context") == -1 || + nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) == -1 || + nbd_set_request_meta_context (nbd, 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + p = (struct progress) { .count = 0 }; + r = nbd_opt_set_meta_context (nbd, + (nbd_context_callback) { .callback = check, + .user_data = &p}); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1 || p.count != 1 || !p.seen) { + fprintf (stderr, "expecting one context, got %d\n", r); + exit (EXIT_FAILURE); + } + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 1) { + fprintf (stderr, "can_meta_context got %d, expected 1\n", r); + exit (EXIT_FAILURE); + } + + /* Transition to transmission phase; our last set should remain active */ + if (nbd_clear_meta_contexts (nbd) == -1 || + nbd_add_meta_context (nbd, "x-nosuch:context") == -1 || + 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)) != 1) { + fprintf (stderr, "can_meta_context got %d, expected 1\n", r); + exit (EXIT_FAILURE); + } + + /* Now too late to set; but should not lose earlier state */ + p = (struct progress) { .count = 0 }; + r = nbd_opt_set_meta_context (nbd, + (nbd_context_callback) { .callback = check, + .user_data = &p}); + if (r != -1 || p.count || p.seen) { + fprintf (stderr, "expecting set_meta failure\n"); + exit (EXIT_FAILURE); + } + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != 1) { + fprintf (stderr, "can_meta_context got %d, expected 1\n", r); + exit (EXIT_FAILURE); + } + + nbd_shutdown (nbd, 0); + nbd_close (nbd); + + /* Second process, this time without structured replies server-side. */ + args[ARRAY_SIZE (args) - 2] = (char *) "--no-sr"; + nbd = nbd_create (); + if (nbd == NULL || + nbd_set_opt_mode (nbd, true) == -1 || + nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) == -1 || + nbd_connect_command (nbd, args) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if ((r = nbd_get_structured_replies_negotiated (nbd)) != 0) { + fprintf (stderr, "structured replies got %d, expected 0\n", r); + exit (EXIT_FAILURE); + } + + /* Expect server-side failure here */ + p = (struct progress) { .count = 0 }; + r = nbd_opt_set_meta_context (nbd, + (nbd_context_callback) { .callback = check, + .user_data = &p}); + if (r != -1 || p.count || p.seen) { + fprintf (stderr, "expecting set_meta failure\n"); + exit (EXIT_FAILURE); + } + if ((r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) != -1) { + fprintf (stderr, "can_meta_context got %d, expected -1\n", r); + exit (EXIT_FAILURE); + } + + /* Even though can_meta fails after failed SET, it returns 0 after go */ + 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, "can_meta_context got %d, expected 0\n", r); + exit (EXIT_FAILURE); + } + + nbd_shutdown (nbd, 0); + nbd_close (nbd); + + exit (EXIT_SUCCESS); +} diff --git a/.gitignore b/.gitignore index bd4650d..5603de5 100644 --- a/.gitignore +++ b/.gitignore @@ -231,6 +231,7 @@ Makefile.in /tests/opt-info /tests/opt-list /tests/opt-list-meta +/tests/opt-set-meta /tests/pki/ /tests/pread-initialize /tests/private-data -- 2.37.2

On 08/31/22 20:28, Eric Blake wrote: I'd like to review this one in its final v3 state. Thanks Laszlo