Re: [Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR

Upstream NBD clarified (see NBD commit 13a4e33a8) that since NBD_OPT_LIST_META_CONTEXT is stateless on the server side, it is acceptable (but not mandatory) for servers to accept it without the client having pre-negotiated structured replies. We aren't quite stateless on the client side yet - that will be fixed in a later patch to keep this one small and easier to test. The testsuite changes pass when using modern nbdkit; however, it skips[*] if we talk to an older server. But since the test also skips with our pre-patch behavior, it's not worth separating it into a separate patch. For more history, qemu-nbd prior to commit da24597d [v6.2] and nbdkit prior to commit c39cba73 [v1.27.3, also backported to 1.26.6] are servers that chose to reject NBD_OPT_LIST_META_CONTEXT without a prior NBD_OPT_STRUCTURED_REPLY. [*]It's easier to skip on server failure than to try and write an nbdkit patch to add yet another --config feature probe just to depend on new-enough nbdkit to gracefully probe in advance if the server should succeed. --- generator/states-newstyle-opt-meta-context.c | 1 - lib/opt.c | 6 +---- tests/opt-list-meta.c | 28 +++++++++++++++++--- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c index a6a5271..5c65454 100644 --- a/generator/states-newstyle-opt-meta-context.c +++ b/generator/states-newstyle-opt-meta-context.c @@ -34,7 +34,6 @@ STATE_MACHINE { meta_vector_reset (&h->meta_contexts); if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) { assert (h->opt_mode); - assert (h->structured_replies); assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context)); opt = h->opt_current; }

diff --git a/lib/opt.c b/lib/opt.c index e5802f4..d9114f4 100644 --- a/lib/opt.c +++ b/lib/opt.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2020-2021 Red Hat Inc. + * Copyright (C) 2020-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 @@ -290,10 +290,6 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle *h, set_error (ENOTSUP, "server is not using fixed newstyle protocol"); return -1; } - if (!h->structured_replies) { - set_error (ENOTSUP, "server lacks structured replies"); - return -1; - } assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); h->opt_cb.fn.context = *context; diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c index 9ad8e37..ccf58fc 100644 --- a/tests/opt-list-meta.c +++ b/tests/opt-list-meta.c @@ -17,6 +17,7 @@ */ /* Test behavior of nbd_opt_list_meta_context. */ +/* See also unit test 240 in the various language ports. */ #include <config.h> @@ -164,7 +165,10 @@ main (int argc, char *argv[]) nbd_opt_abort (nbd); nbd_close (nbd); - /* Repeat but this time without structured replies. */ + /* Repeat but this time without structured replies. Below this point, + * it is not worth porting this part of the test to non-C languages + * because of the potential to skip the rest of the test. + */ nbd = nbd_create (); if (nbd == NULL || nbd_set_opt_mode (nbd, true) == -1 || @@ -174,15 +178,31 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - /* FIXME: For now, we reject this client-side, but it is overly strict. */ + /* Older servers don't permit this, but there is no reliable indicator + * of whether nbdkit is new enough, so just skip the rest of the test + * if the attempt fails (then read the logs to see that the skip was + * indeed caused by the server, and not an accidental client-side bug). + */ 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, "not expecting command to succeed\n"); + if (r == -1) { + fprintf (stderr, "Skipping test; server probably too old for listing " + "without structured replies: %s\n", nbd_get_error ()); + exit (77); + } + 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); + exit (EXIT_SUCCESS); }