Re: [libnbd PATCH v3 19/22] api: Add nbd_[aio_]opt_extended_headers()

[Richard W.M. Jones]

On Thu, May 25, 2023 at 08:01:05AM -0500, Eric Blake wrote:
> Very similar to the recent addition of nbd_opt_structured_reply,
> giving us fine-grained control over an extended headers request.
> 
> Because nbdkit does not yet support extended headers, testsuite
> coverage is limited to interop testing with qemu-nbd.  It shows that
> extended headers imply structured replies, regardless of which order
> the two are manually negotiated in.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  generator/API.ml                              |  79 +++++++--
>  .../states-newstyle-opt-extended-headers.c    |  30 +++-
>  generator/states-newstyle.c                   |   3 +
>  lib/opt.c                                     |  44 +++++
>  interop/Makefile.am                           |   6 +
>  interop/opt-extended-headers.c                | 153 ++++++++++++++++++
>  interop/opt-extended-headers.sh               |  29 ++++
>  .gitignore                                    |   1 +
>  8 files changed, 329 insertions(+), 16 deletions(-)
>  create mode 100644 interop/opt-extended-headers.c
>  create mode 100755 interop/opt-extended-headers.sh
> 
> diff --git a/generator/API.ml b/generator/API.ml
> index 078f140f..85625bbd 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -825,6 +825,7 @@   "set_request_extended_headers", {
>  if L<nbd_set_request_structured_replies(3)> is also set to false,
>  since the use of extended headers implies structured replies.";
>      see_also = [Link "get_request_extended_headers";
> +                Link "opt_extended_headers";
>                  Link "set_handshake_flags"; Link "set_strict_mode";
>                  Link "get_extended_headers_negotiated";
>                  Link "zero"; Link "trim"; Link "cache";
> @@ -856,7 +857,9 @@   "get_extended_headers_negotiated", {
>      shortdesc = "see if extended headers are in use";
>      longdesc = "\
>  After connecting you may call this to find out if the connection is
> -using extended headers.
> +using extended headers.  Note that this setting is sticky; this
> +can return true even after a second L<nbd_opt_extended_headers(3)>
> +returns false because the server detected a duplicate request.
> 
>  When extended headers are not in use, commands are limited to a
>  32-bit length, even when the libnbd API uses a 64-bit parameter
> @@ -938,7 +941,7 @@   "get_structured_replies_negotiated", {
>  attempted.";
>      see_also = [Link "set_request_structured_replies";
>                  Link "get_request_structured_replies";
> -                Link "opt_structured_reply";
> +                Link "opt_structured_reply"; Link "opt_extended_headers";
>                  Link "get_protocol";
>                  Link "get_extended_headers_negotiated"];
>    };
> @@ -1211,12 +1214,13 @@   "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_STARTTLS> and
> -C<NBD_OPT_STRUCTURED_REPLY> before letting you control remaining
> -negotiation steps; if you need control over these steps as well,
> -first set L<nbd_set_tls(3)> to C<LIBNBD_TLS_DISABLE> and
> -L<nbd_set_request_structured_replies(3)> to false before starting
> -the connection attempt.
> +Note that libnbd defaults to attempting C<NBD_OPT_STARTTLS>,
> +C<NBD_OPT_EXTENDED_HEADERS>, and C<NBD_OPT_STRUCTURED_REPLY>
> +before letting you control remaining negotiation steps; if you
> +need control over these steps as well, first set L<nbd_set_tls(3)>
> +to C<LIBNBD_TLS_DISABLE>, and L<nbd_set_request_extended_headers(3)>
> +or 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
> @@ -1324,6 +1328,35 @@   "opt_starttls", {
>                  Link "supports_tls"]
>    };
> 
> +  "opt_extended_headers", {
> +    default_call with
> +    args = []; ret = RBool;
> +    permitted_states = [ Negotiating ];
> +    shortdesc = "request the server to enable extended headers";
> +    longdesc = "\
> +Request that the server use extended headers, by sending
> +C<NBD_OPT_EXTENDED_HEADERS>.  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_extended_headers(3)> or
> +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
> +extended headers are supported (as visible by
> +L<nbd_get_extended_headers_negotiated(3)>) regardless if
> +later calls to this function return false.  If this function
> +returns true, the use of structured replies is implied.";
> +    see_also = [Link "set_opt_mode"; Link "aio_opt_extended_headers";
> +                Link "opt_go"; Link "set_request_extended_headers";
> +                Link "set_request_structured_replies"]
> +  };
> +
>    "opt_structured_reply", {
>      default_call with
>      args = []; ret = RBool;
> @@ -1345,7 +1378,9 @@   "opt_structured_reply", {
>  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.";
> +later calls to this function return false.  Similarly, a
> +server may fail this request if extended headers are already
> +negotiated, since extended headers take priority.";
>      see_also = [Link "set_opt_mode"; Link "aio_opt_structured_reply";
>                  Link "opt_go"; Link "set_request_structured_replies"]
>    };
> @@ -3146,6 +3181,30 @@   "aio_opt_starttls", {
>      see_also = [Link "set_opt_mode"; Link "opt_starttls"];
>    };
> 
> +  "aio_opt_extended_headers", {
> +    default_call with
> +    args = [];
> +    optargs = [ OClosure completion_closure ];
> +    ret = RErr;
> +    permitted_states = [ Negotiating ];
> +    shortdesc = "request the server to enable extended headers";
> +    longdesc = "\
> +Request that the server use extended headers, by sending
> +C<NBD_OPT_EXTENDED_HEADERS>.  This behaves like the synchronous
> +counterpart L<nbd_opt_extended_headers(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_extended_headers"];
> +  };
> +
>    "aio_opt_structured_reply", {
>      default_call with
>      args = [];
> @@ -4122,6 +4181,8 @@ let first_version =
>    "set_request_extended_headers", (1, 18);
>    "get_request_extended_headers", (1, 18);
>    "get_extended_headers_negotiated", (1, 18);
> +  "opt_extended_headers", (1, 18);
> +  "aio_opt_extended_headers", (1, 18);
> 
>    (* 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-extended-headers.c 
> b/generator/states-newstyle-opt-extended-headers.c
> index 1ec25e97..5017a629 100644
> --- a/generator/states-newstyle-opt-extended-headers.c
> +++ b/generator/states-newstyle-opt-extended-headers.c
> @@ -21,11 +21,14 @@
>  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;
> +  if (h->opt_current == NBD_OPT_EXTENDED_HEADERS)
> +    assert (h->opt_mode);
> +  else {
> +    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);
> @@ -68,6 +71,7 @@  NEWSTYLE.OPT_EXTENDED_HEADERS.RECV_REPLY_PAYLOAD:
> 
>   NEWSTYLE.OPT_EXTENDED_HEADERS.CHECK_REPLY:
>    uint32_t reply;
> +  int err = ENOTSUP;
> 
>    reply = be32toh (h->sbuf.or.option_reply.reply);
>    switch (reply) {
> @@ -76,19 +80,31 @@  NEWSTYLE.OPT_EXTENDED_HEADERS.CHECK_REPLY:
>      h->extended_headers = true;
>      /* Extended headers trump structured replies, so skip ahead. */
>      h->structured_replies = true;
> +    err = 0;
>      break;
> +  case NBD_REP_ERR_INVALID:
> +    err = EINVAL;
> +    /* fallthrough */
>    default:
>      if (handle_reply_error (h) == -1) {
>        SET_NEXT_STATE (%.DEAD);
>        return 0;
>      }
> 
> -    debug (h, "extended headers are not supported by this server");
> +    if (h->extended_headers)
> +      debug (h, "extended headers already negotiated");
> +    else
> +      debug (h, "extended headers are not supported by this server");
>      break;
>    }
> 
>    /* Next option. */
> -  SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
> +  if (h->opt_current == NBD_OPT_EXTENDED_HEADERS)
> +    SET_NEXT_STATE (%.NEGOTIATING);
> +  else
> +    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.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 ad5bbf72..45893a8b 100644
> --- a/generator/states-newstyle.c
> +++ b/generator/states-newstyle.c
> @@ -146,6 +146,9 @@  NEWSTYLE.START:
>      case NBD_OPT_STRUCTURED_REPLY:
>        SET_NEXT_STATE (%OPT_STRUCTURED_REPLY.START);
>        return 0;
> +    case NBD_OPT_EXTENDED_HEADERS:
> +      SET_NEXT_STATE (%OPT_EXTENDED_HEADERS.START);
> +      return 0;
>      case NBD_OPT_STARTTLS:
>        SET_NEXT_STATE (%OPT_STARTTLS.START);
>        return 0;
> diff --git a/lib/opt.c b/lib/opt.c
> index f58d5e19..d48acdd1 100644
> --- a/lib/opt.c
> +++ b/lib/opt.c
> @@ -164,6 +164,31 @@ nbd_unlocked_opt_starttls (struct nbd_handle *h)
>    return r;
>  }
> 
> +/* Issue NBD_OPT_EXTENDED_HEADERS and wait for the reply. */
> +int
> +nbd_unlocked_opt_extended_headers (struct nbd_handle *h)
> +{
> +  int err;
> +  nbd_completion_callback c = { .callback = go_complete, .user_data = &err };
> +  int r = nbd_unlocked_aio_opt_extended_headers (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_extended_headers request");
> +      r = -1;
> +    }
> +  }
> +  return r;
> +}
> +
>  /* Issue NBD_OPT_STRUCTURED_REPLY and wait for the reply. */
>  int
>  nbd_unlocked_opt_structured_reply (struct nbd_handle *h)
> @@ -386,6 +411,25 @@ nbd_unlocked_aio_opt_starttls (struct nbd_handle *h,
>  #endif
>  }
> 
> +/* Issue NBD_OPT_EXTENDED_HEADERS without waiting. */
> +int
> +nbd_unlocked_aio_opt_extended_headers (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_EXTENDED_HEADERS;
> +  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_STRUCTURED_REPLY without waiting. */
>  int
>  nbd_unlocked_aio_opt_structured_reply (struct nbd_handle *h,
> diff --git a/interop/Makefile.am b/interop/Makefile.am
> index ec8ea0b2..3f81df0c 100644
> --- a/interop/Makefile.am
> +++ b/interop/Makefile.am
> @@ -25,6 +25,7 @@ EXTRA_DIST = \
>       list-exports-test-dir/disk1 \
>       list-exports-test-dir/disk2 \
>       structured-read.sh \
> +     opt-extended-headers.sh \
>       $(NULL)
> 
>  TESTS_ENVIRONMENT = \
> @@ -134,6 +135,7 @@ check_PROGRAMS += \
>       socket-activation-qemu-nbd \
>       dirty-bitmap \
>       structured-read \
> +     opt-extended-headers \
>       $(NULL)
>  TESTS += \
>       interop-qemu-nbd \
> @@ -144,6 +146,7 @@ TESTS += \
>       dirty-bitmap.sh \
>       structured-read.sh \
>       interop-qemu-block-size.sh \
> +     opt-extended-headers.sh \
>       $(NULL)
> 
>  interop_qemu_nbd_SOURCES = \
> @@ -235,6 +238,9 @@ dirty_bitmap_LDADD = $(top_builddir)/lib/libnbd.la
>  structured_read_SOURCES = structured-read.c
>  structured_read_LDADD = $(top_builddir)/lib/libnbd.la
> 
> +opt_extended_headers_SOURCES = opt-extended-headers.c
> +opt_extended_headers_LDADD = $(top_builddir)/lib/libnbd.la
> +
>  endif HAVE_QEMU_NBD
> 
>  #----------------------------------------------------------------------
> diff --git a/interop/opt-extended-headers.c b/interop/opt-extended-headers.c
> new file mode 100644
> index 00000000..f50cd78f
> --- /dev/null
> +++ b/interop/opt-extended-headers.c
> @@ -0,0 +1,153 @@
> +/* NBD client library in userspace
> + * 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
> + */
> +
> +/* Demonstrate low-level use of nbd_opt_extended_headers(). */
> +
> +#include <config.h>
> +
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +
> +#include <libnbd.h>
> +
> +#define check(got, exp) do_check (#got, got, exp)
> +
> +static void
> +do_check (const char *act, int64_t got, int64_t exp)
> +{
> +  fprintf (stderr, "trying %s\n", act);
> +  if (got == -1)
> +    fprintf (stderr, "%s\n", nbd_get_error ());
> +  else
> +    fprintf (stderr, "succeeded, result %" PRId64 "\n", got);
> +  if (got != exp) {
> +    fprintf (stderr, "got %" PRId64 ", but expected %" PRId64 "\n", got, 
> exp);
> +    exit (EXIT_FAILURE);
> +  }
> +}
> +
> +static int
> +cb (void *data, const char *metacontext, uint64_t offset,
> +         nbd_extent *entries, size_t nr_entries, int *error)
> +{
> +  /* If we got here, extents worked, implying at least structured replies */
> +  bool *seen = data;
> +
> +  *seen = true;
> +  return 0;
> +}
> +
> +struct nbd_handle *
> +prep (bool sr, bool eh, char **cmd)
> +{
> +  struct nbd_handle *nbd;
> +
> +  nbd = nbd_create ();
> +  if (nbd == NULL) {
> +    fprintf (stderr, "%s\n", nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Connect to the server in opt mode, disable client-side failsafes so
> +   * that we are testing server response even when client breaks protocol.
> +   */
> +  check (nbd_set_opt_mode (nbd, true), 0);
> +  check (nbd_set_strict_mode (nbd, 0), 0);
> +  check (nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION), 0);
> +  check (nbd_set_request_structured_replies (nbd, sr), 0);
> +  check (nbd_set_request_extended_headers (nbd, eh), 0);
> +  check (nbd_connect_systemd_socket_activation (nbd, cmd), 0);
> +
> +  return nbd;
> +}
> +
> +void
> +cleanup (struct nbd_handle *nbd, bool extents_exp)
> +{
> +  bool extents = false;
> +
> +  check (nbd_opt_go (nbd), 0);
> +  check (nbd_can_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION),
> +         extents_exp);
> +  check (nbd_block_status_64 (nbd, 512, 0,
> +                              (nbd_extent64_callback) { .callback = cb,
> +                                                        .user_data = 
> &extents },
> +                              0), extents_exp ? 0 : -1);
> +  check (extents, extents_exp);
> +  nbd_close (nbd);
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  struct nbd_handle *nbd;
> +  int64_t bytes_sent;
> +
> +  if (argc < 2) {
> +    fprintf (stderr, "%s qemu-nbd [args ...]\n", argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Default setup tries eh first, and skips sr request when eh works... */
> +  nbd = prep (true, true, &argv[1]);
> +  bytes_sent = nbd_stats_bytes_sent (nbd);
> +  check (nbd_get_extended_headers_negotiated (nbd), true);
> +  check (nbd_get_structured_replies_negotiated (nbd), true);
> +  /* Duplicate eh request is no-op as redundant, but does not change state */
> +  check (nbd_opt_extended_headers (nbd), false);
> +  /* Trying sr after eh is no-op as redundant, but does not change state */
> +  check (nbd_opt_structured_reply (nbd), false);
> +  check (nbd_get_extended_headers_negotiated (nbd), true);
> +  check (nbd_get_structured_replies_negotiated (nbd), true);
> +  cleanup (nbd, true);
> +
> +  /* ...which should result in the same amount of initial negotiation
> +   * traffic as explicitly requesting just structured replies, albeit
> +   * with different results on what got negotiated.
> +   */
> +  nbd = prep (true, false, &argv[1]);
> +  check (nbd_stats_bytes_sent (nbd), bytes_sent);
> +  check (nbd_get_extended_headers_negotiated (nbd), false);
> +  check (nbd_get_structured_replies_negotiated (nbd), true);
> +  cleanup (nbd, true);
> +
> +  /* request_eh is ignored if request_sr is false. */
> +  nbd = prep (false, true, &argv[1]);
> +  check (nbd_get_extended_headers_negotiated (nbd), false);
> +  check (nbd_get_structured_replies_negotiated (nbd), false);
> +  cleanup (nbd, false);
> +
> +  /* Swap order, requesting structured replies before extended headers */
> +  nbd = prep (false, false, &argv[1]);
> +  check (nbd_get_extended_headers_negotiated (nbd), false);
> +  check (nbd_get_structured_replies_negotiated (nbd), false);
> +  check (nbd_opt_structured_reply (nbd), true);
> +  check (nbd_get_extended_headers_negotiated (nbd), false);
> +  check (nbd_get_structured_replies_negotiated (nbd), true);
> +  check (nbd_opt_extended_headers (nbd), true);
> +  check (nbd_get_extended_headers_negotiated (nbd), true);
> +  check (nbd_get_structured_replies_negotiated (nbd), true);
> +  cleanup (nbd, true);
> +
> +  exit (EXIT_SUCCESS);
> +}
> diff --git a/interop/opt-extended-headers.sh b/interop/opt-extended-headers.sh
> new file mode 100755
> index 00000000..41322f36
> --- /dev/null
> +++ b/interop/opt-extended-headers.sh
> @@ -0,0 +1,29 @@
> +#!/usr/bin/env bash
> +# nbd client library in userspace
> +# 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
> +
> +# Test low-level nbd_opt_extended_headers() details with qemu-nbd
> +
> +source ../tests/functions.sh
> +set -e
> +set -x
> +
> +requires qemu-nbd --version
> +requires nbdinfo --can extended-headers -- [ qemu-nbd -r -f raw "$0" ]
> +
> +# Run the test.
> +$VG ./opt-extended-headers qemu-nbd -r -f raw "$0"
> diff --git a/.gitignore b/.gitignore
> index bc7c2c37..24642748 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -118,6 +118,7 @@ Makefile.in
>  /interop/list-exports-nbdkit
>  /interop/list-exports-qemu-nbd
>  /interop/nbd-server-tls.conf
> +/interop/opt-extended-headers
>  /interop/requires.c
>  /interop/socket-activation-nbdkit
>  /interop/socket-activation-qemu-nbd

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top