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

[Richard W.M. Jones]

On Wed, Aug 31, 2022 at 09:39:19AM -0500, Eric Blake wrote:
> 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).
> +   */

In theory you could parse nbdkit --dump-config, although I agree this
approach is fine too.

Rich.

>    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);
>  }
> -- 
> 2.37.2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

-- 
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