[Libguestfs] [libnbd PATCH v4 20/25] generator: Actually request extended headers

This is the culmination of the previous patches' preparation work for using extended headers when possible. The new states in the state machine are copied extensively from our handling of OPT_STRUCTURED_REPLY. The next patch will then expose a new API nbd_opt_extended_headers() for manual control. At the same time I posted this patch, I had patches for qemu-nbd to support extended headers as server (nbdkit is a bit tougher). The next patches will add some interop tests that pass when using a new enough qemu-nbd, showing that we have cross-project interoperability and therefore an extension worth standardizing. NOTE: This patch causes an observable (albeit uncommon) change in behavior to a specific corner case involving a meta-context with 64-bit flags (so far, no such meta-context exists, but I'll use "x-context:big" as a placeholder for such a meta-context). An application compiled and run with libnbd 1.16 that requests nbd_add_meta_context(h, "x-context:big") will fail to negotiate that context, but can still succeed at negotiating "base:allocation". What's more, that application was compiled at a time when the nbd_block_status_64() API did not exist, so it will necessarily be using the older 32-bit nbd_block_status() API. With the approach done in this patch (that is, the same client now linking against libnbd 1.18 defaults to unconditionally requesting extended headers), the negotiation for "x-context:big" will now succeed, but calls to nbd_block_status() will encounter an EOVERFLOW error when the server's 64-bit flags cannot be passed back to the caller's 32-bit callback. The caller will still see the "base:allocation" results, but the overall API will fail. Thus, we have taken a case that used to pass into something that now fails. Still, it is easy to make the following mitigating arguments: 1) most applications _don't_ blindly request "x-context:big" while insisting on a 32-bit API; since interpreting the context bits is context-specific, any app that knows what those bits actually mean (or which want to support arbitrary user input for meta contexts, like 'nbdinfo --map') will want to be compiled against libnbd 1.18 in the first place, to take advantage of nbd_block_status_64(). 2) If the application REALLY needs to preserve 1.16 behavior, even when compiled against 1.18 or newer, it is easy enough to add the following escape hatch: #if LIBNBD_HAVE_NBD_SET_REQUEST_EXTENDED_HEADERS nbd_set_request_extended_headers(h, 0); #endif And since such an escape hatch is only needed in the finite (possibly empty?) set of programs that need to preserve the older behavior, it does not introduce scaling problems. Because my approach is a subtle change, I also evaluated two other potential solutions, documented here, and why I did not go with them: Approach 2) No change to the default. ALL applications that want to use extended headers have to explicitly opt-in (possibly taking advantage of LIBNBD_HAVE_... witness macros to allow successful compilation to fallback APIs when compiling against older libnbd). This avoids the subtle change, but comes at an open-ended cost: all future libnbd clients that WANT to benefit from extended headers will have to add boilerplate to their connection setup, which does not scale well, just to benefit the few applications that depended on the older behavior. It becomes highly likely that someone will write a new client but does not use the new boilerplate, and thereby inadvertently suffers from worse performace because of the older defaults being set in stone. Approach 3) Resort to versioned symbols, to make the default of whether to request extended headers (in the absence of an explicit nbd_set_request_extended_headers() call) depend on which version of libnbd one compiles against. That is, teach our linker script to produce aliased symbols, where nbd_create becomes an alias to either __nbd_create@@1_16 (default off) for backwards compatibility to apps compiled against older libnbd but run against the new .so, or to __nbd_create@@1_18 (default on) with preprocessor magic in <libnbd.h> that you get this alias by default when compiling against newer libnbd. This scales well for clients (old apps continue to run with identical behavior, new apps automatically get new features unless they request preprocessor magic to specifically favor the older aliasing styles), but is dreadful to maintain (look at glibc for the hoops they have to jump through for such versioned-symbol shenanigans needed to provide transparent multi-standard support). I don't see libnbd as such a critical or high-user-base project that we need to take on this extra level of work. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- v4: expand commit message to document other alternatives I did not code up --- generator/API.ml | 87 ++++++++--------- generator/state_machine.ml | 41 ++++++++ .../states-newstyle-opt-extended-headers.c | 94 +++++++++++++++++++ generator/states-newstyle-opt-starttls.c | 6 +- generator/Makefile.am | 1 + 5 files changed, 184 insertions(+), 45 deletions(-) create mode 100644 generator/states-newstyle-opt-extended-headers.c diff --git a/generator/API.ml b/generator/API.ml index 36033817..d1849710 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -953,23 +953,24 @@ "set_request_meta_context", { (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; 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 +the server supports structured replies or extended headers 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; 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."; +negotiate structured replies or extended headers, 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_set_meta_context"; Link "get_structured_replies_negotiated"; @@ -1404,11 +1405,11 @@ "opt_info", { If successful, functions like L<nbd_is_read_only(3)> and 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 +structured replies or extended headers 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. @@ -1538,12 +1539,12 @@ "opt_set_meta_context", { 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 +automatically does the same task if structured replies or extended +headers 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 @@ -1597,12 +1598,13 @@ "opt_set_meta_context_queries", { 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 same task if structured replies or extended headers 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. This function takes an explicit list of queries; to @@ -3281,13 +3283,13 @@ "aio_opt_set_meta_context", { 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)>. +automatically does the same task if structured replies or +extended headers 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 @@ -3314,12 +3316,13 @@ "aio_opt_set_meta_context_queries", { 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 same task if structured replies or extended headers 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 diff --git a/generator/state_machine.ml b/generator/state_machine.ml index 7a5bc59b..2d912ef8 100644 --- a/generator/state_machine.ml +++ b/generator/state_machine.ml @@ -297,6 +297,7 @@ and * NEGOTIATING after OPT_STRUCTURED_REPLY or any failed OPT_GO. *) Group ("OPT_STARTTLS", newstyle_opt_starttls_state_machine); + Group ("OPT_EXTENDED_HEADERS", newstyle_opt_extended_headers_state_machine); Group ("OPT_STRUCTURED_REPLY", newstyle_opt_structured_reply_state_machine); Group ("OPT_META_CONTEXT", newstyle_opt_meta_context_state_machine); Group ("OPT_GO", newstyle_opt_go_state_machine); @@ -441,6 +442,46 @@ and }; ] +(* Fixed newstyle NBD_OPT_EXTENDED_HEADERS option. + * Implementation: generator/states-newstyle-opt-extended-headers.c + *) +and newstyle_opt_extended_headers_state_machine = [ + State { + default_state with + name = "START"; + comment = "Try to negotiate newstyle NBD_OPT_EXTENDED_HEADERS"; + external_events = []; + }; + + State { + default_state with + name = "SEND"; + comment = "Send newstyle NBD_OPT_EXTENDED_HEADERS negotiation request"; + external_events = [ NotifyWrite, "" ]; + }; + + State { + default_state with + name = "RECV_REPLY"; + comment = "Receive newstyle NBD_OPT_EXTENDED_HEADERS option reply"; + external_events = [ NotifyRead, "" ]; + }; + + State { + default_state with + name = "RECV_REPLY_PAYLOAD"; + comment = "Receive any newstyle NBD_OPT_EXTENDED_HEADERS reply payload"; + external_events = [ NotifyRead, "" ]; + }; + + State { + default_state with + name = "CHECK_REPLY"; + comment = "Check newstyle NBD_OPT_EXTENDED_HEADERS option reply"; + external_events = []; + }; +] + (* Fixed newstyle NBD_OPT_STRUCTURED_REPLY option. * Implementation: generator/states-newstyle-opt-structured-reply.c *) diff --git a/generator/states-newstyle-opt-extended-headers.c b/generator/states-newstyle-opt-extended-headers.c new file mode 100644 index 00000000..1ec25e97 --- /dev/null +++ b/generator/states-newstyle-opt-extended-headers.c @@ -0,0 +1,94 @@ +/* nbd client library in userspace: state machine + * Copyright Red Hat + * + * 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 + */ + +/* State machine for negotiating NBD_OPT_EXTENDED_HEADERS. */ + +STATE_MACHINE { + NEWSTYLE.OPT_EXTENDED_HEADERS.START: + assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); + assert (h->opt_current != NBD_OPT_EXTENDED_HEADERS); + assert (CALLBACK_IS_NULL (h->opt_cb.completion)); + if (!h->request_eh || !h->request_sr) { + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + return 0; + } + + h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); + h->sbuf.option.option = htobe32 (NBD_OPT_EXTENDED_HEADERS); + h->sbuf.option.optlen = htobe32 (0); + h->chunks_sent++; + h->wbuf = &h->sbuf; + h->wlen = sizeof h->sbuf.option; + SET_NEXT_STATE (%SEND); + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.SEND: + switch (send_from_wbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 0: + h->rbuf = &h->sbuf; + h->rlen = sizeof h->sbuf.or.option_reply; + SET_NEXT_STATE (%RECV_REPLY); + } + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.RECV_REPLY: + switch (recv_into_rbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 0: + if (prepare_for_reply_payload (h, NBD_OPT_EXTENDED_HEADERS) == -1) { + SET_NEXT_STATE (%.DEAD); + return 0; + } + SET_NEXT_STATE (%RECV_REPLY_PAYLOAD); + } + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.RECV_REPLY_PAYLOAD: + switch (recv_into_rbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 0: SET_NEXT_STATE (%CHECK_REPLY); + } + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.CHECK_REPLY: + uint32_t reply; + + reply = be32toh (h->sbuf.or.option_reply.reply); + switch (reply) { + case NBD_REP_ACK: + debug (h, "negotiated extended headers on this connection"); + h->extended_headers = true; + /* Extended headers trump structured replies, so skip ahead. */ + h->structured_replies = true; + break; + default: + if (handle_reply_error (h) == -1) { + SET_NEXT_STATE (%.DEAD); + return 0; + } + + debug (h, "extended headers are not supported by this server"); + break; + } + + /* Next option. */ + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + return 0; + +} /* END STATE MACHINE */ diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c index e497548c..1e2997a3 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -26,7 +26,7 @@ NEWSTYLE.OPT_STARTTLS.START: else { /* If TLS was not requested we skip this option and go to the next one. */ if (h->tls == LIBNBD_TLS_DISABLE) { - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + SET_NEXT_STATE (%^OPT_EXTENDED_HEADERS.START); return 0; } assert (CALLBACK_IS_NULL (h->opt_cb.completion)); @@ -128,7 +128,7 @@ NEWSTYLE.OPT_STARTTLS.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); else { debug (h, "continuing with unencrypted connection"); - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + SET_NEXT_STATE (%^OPT_EXTENDED_HEADERS.START); } return 0; } @@ -185,7 +185,7 @@ NEWSTYLE.OPT_STARTTLS.TLS_HANDSHAKE_DONE: if (h->opt_current == NBD_OPT_STARTTLS) SET_NEXT_STATE (%.NEGOTIATING); else - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + SET_NEXT_STATE (%^OPT_EXTENDED_HEADERS.START); return 0; } /* END STATE MACHINE */ diff --git a/generator/Makefile.am b/generator/Makefile.am index c3d53b26..c8477842 100644 --- a/generator/Makefile.am +++ b/generator/Makefile.am @@ -30,6 +30,7 @@ states_code = \ states-issue-command.c \ states-magic.c \ states-newstyle-opt-export-name.c \ + states-newstyle-opt-extended-headers.c \ states-newstyle-opt-list.c \ states-newstyle-opt-go.c \ states-newstyle-opt-meta-context.c \ -- 2.41.0