[Libguestfs] [libnbd PATCH v3 17/18] api: Add nbd_[aio_]opt_structured_reply()

Recent commits have lamented that we do not have a way to provoke atypical ordering of option negotiation during server integration testing. This commit adds nbd_opt_structured_reply() to make it possible to fine-tune when structured replies are requested, and fills in a couple of FIXME comments left in previous commits. The new API can be called more than once; the testsuite is also enhanced with a test that shows that nbdkit rejects the second attempt but libnbd still remembers the first one succeeded (this requires moving the wipe of h->structured_reply out of NBD_OPT_STRUCTURED_REPLY error handling, and instead putting it into NBD_OPT_STARTTLS success, which becomes important when a later patch adds nbd_opt_starttls()). The C test changes depend on a new API, so they can't reasonably be applied out of order. However, porting the FIXME changes of the testsuite to other language bindings will be done in the next patch, to ease review of this one. The test opt-structured-twice borders on being an integration test (in fact, nbdkit commit 9418ec58 [upcoming v1.34] changes nbdkit from accepting a second structured reply request to instead diagnosing it as redundant; both behaviors are compatible with the NBD spec), so that one will remain C only. --- generator/API.ml | 70 ++++++++- generator/states-newstyle-opt-starttls.c | 1 + .../states-newstyle-opt-structured-reply.c | 22 ++- generator/states-newstyle.c | 3 + lib/opt.c | 44 ++++++ tests/Makefile.am | 5 + tests/opt-list-meta.c | 29 +++- tests/opt-set-meta.c | 50 ++++-- tests/opt-structured-twice.c | 145 ++++++++++++++++++ .gitignore | 1 + 10 files changed, 347 insertions(+), 23 deletions(-) create mode 100644 tests/opt-structured-twice.c diff --git a/generator/API.ml b/generator/API.ml index 9ffe6516..1c9f890a 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -794,9 +794,12 @@ "set_request_structured_replies", { L<nbd_can_meta_context(3)> or L<nbd_can_df(3)> can return true. However, for integration testing, it can be useful to clear this flag rather than find a way to alter the server to fail the negotiation -request."; +request. It is also useful to set this to false prior to using +L<nbd_set_opt_mode(3)> if it is desired to control when to send +L<nbd_opt_structured_reply(3)> during negotiation."; see_also = [Link "get_request_structured_replies"; Link "set_handshake_flags"; Link "set_strict_mode"; + Link "opt_structured_reply"; Link "get_structured_replies_negotiated"; Link "can_meta_context"; Link "can_df"]; }; @@ -824,9 +827,12 @@ "get_structured_replies_negotiated", { shortdesc = "see if structured replies are in use"; longdesc = "\ After connecting you may call this to find out if the connection is -using structured replies."; +using structured replies. Note that this setting is sticky; this +can return true even after a second L<nbd_opt_structured_reply(3)> +returns false because the server detected a duplicate request."; see_also = [Link "set_request_structured_replies"; Link "get_request_structured_replies"; + Link "opt_structured_reply"; Link "get_protocol"]; }; @@ -1086,6 +1092,12 @@ "set_opt_mode", { newstyle server. This setting has no effect when connecting to an oldstyle server. +Note that libnbd defaults to attempting +C<NBD_OPT_STRUCTURED_REPLY> before letting you control remaining +negotiation steps; if you need control over this step as well, +first set L<nbd_set_request_structured_replies(3)> to false before +starting the connection attempt. + When option mode is enabled, you have fine-grained control over which options are negotiated, compared to the default of the server negotiating everything on your behalf using settings made before @@ -1099,6 +1111,8 @@ "set_opt_mode", { Link "opt_abort"; Link "opt_go"; Link "opt_list"; Link "opt_info"; Link "opt_list_meta_context"; Link "opt_set_meta_context"; + Link "opt_structured_reply"; + Link "set_request_structured_replies"; Link "aio_connect"]; }; @@ -1152,6 +1166,32 @@ "opt_abort", { see_also = [Link "set_opt_mode"; Link "aio_opt_abort"; Link "opt_go"]; }; + "opt_structured_reply", { + default_call with + args = []; ret = RBool; + permitted_states = [ Negotiating ]; + shortdesc = "request the server to enable structured replies"; + longdesc = "\ +Request that the server use structured replies, by sending +C<NBD_OPT_STRUCTURED_REPLY>. This can only be used if +L<nbd_set_opt_mode(3)> enabled option mode; furthermore, libnbd +defaults to automatically requesting this unless you use +L<nbd_set_request_structured_replies(3)> prior to connecting. +This function is mainly useful for integration testing of corner +cases in server handling. + +This function returns true if the server replies with success, +false if the server replies with an error, and fails only if +the server does not reply (such as for a loss of connection). +Note that some servers fail a second request as redundant; +libnbd assumes that once one request has succeeded, then +structured replies are supported (as visible by +L<nbd_get_structured_replies_negotiated(3)>) regardless if +later calls to this function return false."; + see_also = [Link "set_opt_mode"; Link "aio_opt_structured_reply"; + Link "opt_go"; Link "set_request_structured_replies"] + }; + "opt_list", { default_call with args = [ Closure list_closure ]; ret = RInt; @@ -2775,6 +2815,30 @@ "aio_opt_abort", { see_also = [Link "set_opt_mode"; Link "opt_abort"]; }; + "aio_opt_structured_reply", { + default_call with + args = []; + optargs = [ OClosure completion_closure ]; + ret = RErr; + permitted_states = [ Negotiating ]; + shortdesc = "request the server to enable structured replies"; + longdesc = "\ +Request that the server use structured replies, by sending +C<NBD_OPT_STRUCTURED_REPLY>. This behaves like the synchronous +counterpart L<nbd_opt_structured_reply(3)>, except that it does +not wait for the server's response. + +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_structured_reply"]; + }; + "aio_opt_list", { default_call with args = [ Closure list_closure ]; @@ -3678,6 +3742,8 @@ let first_version = "opt_set_meta_context_queries", (1, 16); "aio_opt_set_meta_context", (1, 16); "aio_opt_set_meta_context_queries", (1, 16); + "opt_structured_reply", (1, 16); + "aio_opt_structured_reply", (1, 16); (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c index a9383ced..afaad85a 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -72,6 +72,7 @@ NEWSTYLE.OPT_STARTTLS.CHECK_REPLY: reply = be32toh (h->sbuf.or.option_reply.reply); switch (reply) { case NBD_REP_ACK: + h->structured_replies = false; new_sock = nbd_internal_crypto_create_session (h, h->sock); if (new_sock == NULL) { SET_NEXT_STATE (%.DEAD); diff --git a/generator/states-newstyle-opt-structured-reply.c b/generator/states-newstyle-opt-structured-reply.c index 0ea0fd43..300cf20b 100644 --- a/generator/states-newstyle-opt-structured-reply.c +++ b/generator/states-newstyle-opt-structured-reply.c @@ -21,12 +21,17 @@ STATE_MACHINE { NEWSTYLE.OPT_STRUCTURED_REPLY.START: assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); - if (!h->request_sr) { - if (h->opt_mode) - SET_NEXT_STATE (%.NEGOTIATING); - else - SET_NEXT_STATE (%^OPT_META_CONTEXT.START); - return 0; + if (h->opt_current == NBD_OPT_STRUCTURED_REPLY) + assert (h->opt_mode); + else { + assert (CALLBACK_IS_NULL(h->opt_cb.completion)); + if (!h->request_sr) { + if (h->opt_mode) + SET_NEXT_STATE (%.NEGOTIATING); + else + SET_NEXT_STATE (%^OPT_META_CONTEXT.START); + return 0; + } } h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); @@ -69,12 +74,14 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.RECV_REPLY_PAYLOAD: NEWSTYLE.OPT_STRUCTURED_REPLY.CHECK_REPLY: uint32_t reply; + int err = ENOTSUP; reply = be32toh (h->sbuf.or.option_reply.reply); switch (reply) { case NBD_REP_ACK: debug (h, "negotiated structured replies on this connection"); h->structured_replies = true; + err = 0; break; default: if (handle_reply_error (h) == -1) { @@ -83,7 +90,6 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.CHECK_REPLY: } debug (h, "structured replies are not supported by this server"); - h->structured_replies = false; break; } @@ -92,6 +98,8 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); else SET_NEXT_STATE (%^OPT_META_CONTEXT.START); + CALL_CALLBACK (h->opt_cb.completion, &err); + nbd_internal_free_option (h); return 0; } /* END STATE MACHINE */ diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c index 60e0bc50..4652bc21 100644 --- a/generator/states-newstyle.c +++ b/generator/states-newstyle.c @@ -143,6 +143,9 @@ NEWSTYLE.START: case NBD_OPT_SET_META_CONTEXT: SET_NEXT_STATE (%OPT_META_CONTEXT.START); return 0; + case NBD_OPT_STRUCTURED_REPLY: + SET_NEXT_STATE (%OPT_STRUCTURED_REPLY.START); + return 0; case 0: break; default: diff --git a/lib/opt.c b/lib/opt.c index ee036235..1b18920f 100644 --- a/lib/opt.c +++ b/lib/opt.c @@ -127,6 +127,31 @@ nbd_unlocked_opt_abort (struct nbd_handle *h) return wait_for_option (h); } +/* Issue NBD_OPT_STRUCTURED_REPLY and wait for the reply. */ +int +nbd_unlocked_opt_structured_reply (struct nbd_handle *h) +{ + int err; + nbd_completion_callback c = { .callback = go_complete, .user_data = &err }; + int r = nbd_unlocked_aio_opt_structured_reply (h, &c); + + if (r == -1) + return r; + + r = wait_for_option (h); + if (r == 0) { + if (nbd_internal_is_state_negotiating (get_next_state (h))) + r = err == 0; + else { + assert (nbd_internal_is_state_dead (get_next_state (h))); + set_error (err, + "failed to get response to opt_structured_reply request"); + r = -1; + } + } + return r; +} + struct list_helper { int count; nbd_list_callback list; @@ -300,6 +325,25 @@ nbd_unlocked_aio_opt_abort (struct nbd_handle *h) return 0; } +/* Issue NBD_OPT_STRUCTURED_REPLY without waiting. */ +int +nbd_unlocked_aio_opt_structured_reply (struct nbd_handle *h, + 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; + } + + h->opt_current = NBD_OPT_STRUCTURED_REPLY; + h->opt_cb.completion = *complete; + SET_CALLBACK_TO_NULL (*complete); + + if (nbd_internal_run (h, cmd_issue) == -1) + debug (h, "option queued, ignoring state machine failure"); + return 0; +} + /* Issue NBD_OPT_LIST without waiting. */ int nbd_unlocked_aio_opt_list (struct nbd_handle *h, nbd_list_callback *list, diff --git a/tests/Makefile.am b/tests/Makefile.am index ec95495e..49ea6864 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -217,6 +217,7 @@ check_PROGRAMS += \ opt-list-meta-queries \ opt-set-meta \ opt-set-meta-queries \ + opt-structured-twice \ connect-systemd-socket-activation \ connect-unix \ connect-tcp \ @@ -286,6 +287,7 @@ TESTS += \ opt-list-meta-queries \ opt-set-meta \ opt-set-meta-queries \ + opt-structured-twice \ connect-systemd-socket-activation \ connect-unix \ connect-tcp \ @@ -557,6 +559,9 @@ opt_set_meta_LDADD = $(top_builddir)/lib/libnbd.la opt_set_meta_queries_SOURCES = opt-set-meta-queries.c opt_set_meta_queries_LDADD = $(top_builddir)/lib/libnbd.la +opt_structured_twice_SOURCES = opt-structured-twice.c +opt_structured_twice_LDADD = $(top_builddir)/lib/libnbd.la + connect_systemd_socket_activation_SOURCES = \ connect-systemd-socket-activation.c \ requires.c \ diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c index 05c1a5eb..b32dbf90 100644 --- a/tests/opt-list-meta.c +++ b/tests/opt-list-meta.c @@ -269,7 +269,34 @@ main (int argc, char *argv[]) } } - /* FIXME: Once nbd_opt_structured_reply() exists, use it here and retry. */ + /* Now enable structured replies, and a retry should pass. */ + r = nbd_opt_structured_reply (nbd); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "expecting structured replies to be supported\n"); + 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 != p.count) { + fprintf (stderr, "inconsistent return value %d, expected %d\n", + r, p.count); + exit (EXIT_FAILURE); + } + if (r < 1 || !p.seen) { + fprintf (stderr, "server did not reply with base:allocation\n"); + exit (EXIT_FAILURE); + } nbd_opt_abort (nbd); nbd_close (nbd); diff --git a/tests/opt-set-meta.c b/tests/opt-set-meta.c index a4366d24..48fce9bd 100644 --- a/tests/opt-set-meta.c +++ b/tests/opt-set-meta.c @@ -59,37 +59,61 @@ main (int argc, char *argv[]) char *args[] = { "nbdkit", "-s", "--exit-with-parent", "-v", "memory", "size=1M", NULL, NULL }; - /* First process, with structured replies. Get into negotiating state. */ + /* First process, delay structured replies. Get into negotiating state. */ nbd = nbd_create (); if (nbd == NULL || nbd_set_opt_mode (nbd, true) == -1 || + nbd_set_request_structured_replies (nbd, false) == -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)) != 0) { + fprintf (stderr, "structured replies got %d, expected 0\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); + } + + /* SET cannot succeed until SR is negotiated. */ + 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 failure of set without SR\n"); + exit (EXIT_FAILURE); + } + r = nbd_opt_structured_reply (nbd); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "expecting server support for SR\n"); + exit (EXIT_FAILURE); + } 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) { diff --git a/tests/opt-structured-twice.c b/tests/opt-structured-twice.c new file mode 100644 index 00000000..5924d22e --- /dev/null +++ b/tests/opt-structured-twice.c @@ -0,0 +1,145 @@ +/* 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 + */ + +/* Demonstrate low-level use of nbd_opt_structured_reply(). */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <errno.h> +#include <unistd.h> +#include <sys/stat.h> + +#include <libnbd.h> + + +static int +check_extent (void *data, const char *metacontext, uint64_t offset, + uint32_t *entries, size_t nr_entries, int *error) +{ + /* If we got here, structured replies were negotiated. */ + bool *seen = data; + + *seen = true; + return 0; +} + +int +main (int argc, char *argv[]) +{ + struct nbd_handle *nbd; + const char *cmd[] = { + "nbdkit", "-s", "-v", "--exit-with-parent", "memory", "1048576", NULL + }; + int r; + bool extents_worked = false; + + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Connect to the server in opt mode, without structured replies. */ + if (nbd_set_opt_mode (nbd, true) == -1 || + nbd_set_request_structured_replies (nbd, false) == -1 || + nbd_connect_command (nbd, (char **) cmd) == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + + r = nbd_get_structured_replies_negotiated (nbd); + if (r == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 0) { + fprintf (stderr, "%s: not expecting structured replies yet\n", argv[0]); + exit (EXIT_FAILURE); + } + + /* First request should succeed. */ + r = nbd_opt_structured_reply (nbd); + if (r == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "%s: expecting structured replies\n", argv[0]); + exit (EXIT_FAILURE); + } + r = nbd_get_structured_replies_negotiated (nbd); + if (r == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "%s: expecting structured replies\n", argv[0]); + exit (EXIT_FAILURE); + } + + /* nbdkit 1.32 allows a second request, nbdkit 1.34 diagnoses it. */ + r = nbd_opt_structured_reply (nbd); + if (r == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + printf ("%s: server's response to second request: %s\n", argv[0], + r ? "accepted" : "rejected"); + + /* Regardless of whether second request passed, structured replies were + * negotiated, so we should be able to do block status. + */ + r = nbd_get_structured_replies_negotiated (nbd); + if (r == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "%s: expecting structured replies\n", argv[0]); + exit (EXIT_FAILURE); + } + + if (nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) == -1 || + nbd_opt_go (nbd) == -1 || + (r = nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION)) == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "%s: expecting base:allocation support\n", argv[0]); + exit (EXIT_FAILURE); + } + + if (nbd_block_status (nbd, 65536, 0, + (nbd_extent_callback) { .callback = check_extent, + .user_data = &extents_worked }, + 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (!extents_worked) { + fprintf (stderr, "%s: expecting block_status success\n", argv[0]); + exit (EXIT_FAILURE); + } + + nbd_close (nbd); + exit (EXIT_SUCCESS); +} diff --git a/.gitignore b/.gitignore index 50e4616e..37166b73 100644 --- a/.gitignore +++ b/.gitignore @@ -236,6 +236,7 @@ Makefile.in /tests/opt-list-meta-queries /tests/opt-set-meta /tests/opt-set-meta-queries +/tests/opt-structured-twice /tests/pki/ /tests/pread-initialize /tests/private-data -- 2.37.3