[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [libnbd PATCH] api: Add set_handshake_flags for integration



On Mon, Sep 16, 2019 at 02:35:33PM -0500, Eric Blake wrote:
> Similar to the recent --mask-handshake command line added to nbdkit to
> test client fallbacks to crippled servers, it can be worth testing
> server fallbacks to crippled clients.  And just as we have exposed
> whether the client will request structured replies, we can also expose
> whether the client will understand various handshake flags from the
> NBD protocol.
> 
> Of course, we default to supporting all flags that we understand and
> which are advertised by the server.  But clearing FIXED_NEWSTYLE lets
> us test NBD_OPT_EXPORT_NAME handling, and clearing NO_ZEROES lets us
> test whether the zero padding in response to NBD_OPT_EXPORT_NAME is
> correct.
> ---
> 
> I'm still considering the addition of tests/* against nbd, or interop/*
> against qemu, to ensure that we have interoperability between various
> degraded connection modes. But that can be a separate patch.
> 
>  lib/internal.h                              |  1 +
>  lib/nbd-protocol.h                          |  5 +-
>  generator/generator                         | 72 ++++++++++++++++++++-
>  generator/states-newstyle-opt-export-name.c |  2 +-
>  generator/states-newstyle.c                 | 14 ++--
>  generator/states-oldstyle.c                 |  5 ++
>  lib/handle.c                                | 18 ++++++
>  7 files changed, 107 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index ccaca32..998ca3d 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -157,6 +157,7 @@ struct nbd_handle {
>          } __attribute__((packed)) error;
>        } payload;
>      }  __attribute__((packed)) sr;
> +    uint16_t gflags;
>      uint32_t cflags;
>      uint32_t len;
>      uint16_t nrinfos;
> diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h
> index 04e93d3..699aa22 100644
> --- a/lib/nbd-protocol.h
> +++ b/lib/nbd-protocol.h
> @@ -93,9 +93,10 @@ struct nbd_fixed_new_option_reply {
> 
>  #define NBD_REP_MAGIC UINT64_C(0x3e889045565a9)
> 
> -/* Global flags. */
> +/* Global flags. Exposed by the generator as LIBNBD_HANDSHAKE_FLAG_* instead
>  #define NBD_FLAG_FIXED_NEWSTYLE 1
>  #define NBD_FLAG_NO_ZEROES      2
> + */
> 
>  /* Per-export flags. */
>  #define NBD_FLAG_HAS_FLAGS         (1 << 0)
> @@ -238,10 +239,12 @@ struct nbd_structured_reply_error {
>  #define NBD_CMD_WRITE_ZEROES      6
>  #define NBD_CMD_BLOCK_STATUS      7
> 
> +/* Command flags. Exposed by the generator as LIBNBD_CMD_FLAG_* instead
>  #define NBD_CMD_FLAG_FUA      (1<<0)
>  #define NBD_CMD_FLAG_NO_HOLE  (1<<1)
>  #define NBD_CMD_FLAG_DF       (1<<2)
>  #define NBD_CMD_FLAG_REQ_ONE  (1<<3)
> +*/
> 
>  /* NBD error codes. */
>  #define NBD_SUCCESS     0
> diff --git a/generator/generator b/generator/generator
> index a72f36c..170121e 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -971,7 +971,14 @@ let cmd_flags = {
>      "FAST_ZERO", 1 lsl 4;
>    ]
>  }
> -let all_flags = [ cmd_flags ]
> +let handshake_flags = {
> +  flag_prefix = "HANDSHAKE_FLAG";
> +  flags = [
> +    "FIXED_NEWSTYLE", 1 lsl 0;
> +    "NO_ZEROES",      1 lsl 1;
> +    ]
> +}
> +let all_flags = [ cmd_flags; handshake_flags ]
> 
>  (* Calls.
>   *
> @@ -1261,6 +1268,7 @@ 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.";
>      see_also = ["L<nbd_get_request_structured_replies(3)>";
> +                "L<nbd_set_handshake_flags(3)>";
>                  "L<nbd_can_meta_context(3)>"; "L<nbd_can_df(3)>"];
>    };
> 
> @@ -1277,6 +1285,66 @@ able to honor that request";
>      see_also = ["L<nbd_set_request_structured_replies(3)>"];
>    };
> 
> +  "set_handshake_flags", {
> +    default_call with
> +    args = [ Flags ("flags", handshake_flags) ]; ret = RErr;
> +    permitted_states = [ Created ];
> +    shortdesc = "control use of handshake flags";
> +    longdesc = "\
> +By default, libnbd tries to negotiate all possible handshake flags
> +that are also supported by the server; since omitting a handshake
> +flag can prevent the use of other functionality such as TLS encryption
> +or structured replies.  However, for integration testing, it can be
> +useful to reduce the set of flags supported by the client to test that
> +a particular server can handle various clients that were compliant to
> +older versions of the NBD specification.
> +
> +The C<flags> argument is a bitmask, including zero or more of the
> +following handshake flags:
> +
> +=over 4
> +
> +=item C<LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE> = 1
> +
> +The server gracefully handles unknown option requests from the
> +client, rather than disconnecting.  Without this flag, a client
> +cannot safely request to use extensions such as TLS encryption or
> +structured replies, as the request may cause an older server to
> +drop the connection.
> +
> +=item C<LIBNBD_HANDSHAKE_FLAG_NO_ZEROES> = 2
> +
> +If the client is forced to use C<NBD_OPT_EXPORT_NAME> instead of
> +the preferred C<NBD_OPT_GO>, this flag allows the server to send
> +fewer all-zero padding bytes over the connection.
> +
> +=back
> +
> +Future NBD extensions may add further flags.
> +";
> +    see_also = ["L<nbd_get_handshake_flags(3)>";
> +                "L<nbd_set_request_structured_replies(3)>"];
> +  };
> +
> +  "get_handshake_flags", {
> +    default_call with
> +    args = []; ret = RUInt;
> +    may_set_error = false;
> +    shortdesc = "see which handshake flags are supported";
> +    longdesc = "\
> +Return the state of the handshake flags on this handle.  When the
> +handle has not yet completed a connection (see C<nbd_aio_is_created>),
> +this returns the flags that the client is willing to use, provided
> +the server also advertises those flags.  After the connection is
> +ready (see C<nbd_aio_is_ready>), this returns the flags that were
> +actually agreed on between the server and client.  If the NBD
> +protocol defines new handshake flags, then the return value from
> +a newer library version may include bits that were undefined at
> +the time of compilation.";
> +    see_also = ["L<nbd_set_handshake_flags(3)>";
> +                "L<nbd_aio_is_created(3)>"; "L<nbd_aio_is_ready(3)>"];
> +  };
> +
>    "add_meta_context", {
>      default_call with
>      args = [ String "name" ]; ret = RErr;
> @@ -2521,6 +2589,8 @@ let first_version = [
>    "can_fast_zero", (1, 2);
>    "set_request_structured_replies", (1, 2);
>    "get_request_structured_replies", (1, 2);
> +  "set_handshake_flags", (1, 2);
> +  "get_handshake_flags", (1, 2);
> 
>    (* 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-export-name.c b/generator/states-newstyle-opt-export-name.c
> index ec73136..a46d851 100644
> --- a/generator/states-newstyle-opt-export-name.c
> +++ b/generator/states-newstyle-opt-export-name.c
> @@ -45,7 +45,7 @@
>    case 0:
>      h->rbuf = &h->sbuf;
>      h->rlen = sizeof h->sbuf.export_name_reply;
> -    if ((h->gflags & NBD_FLAG_NO_ZEROES) != 0)
> +    if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_NO_ZEROES) != 0)
>        h->rlen -= sizeof h->sbuf.export_name_reply.zeroes;
>      SET_NEXT_STATE (%RECV_REPLY);
>    }
> diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
> index b4f2b80..8c3ae8a 100644
> --- a/generator/states-newstyle.c
> +++ b/generator/states-newstyle.c
> @@ -112,8 +112,8 @@ handle_reply_error (struct nbd_handle *h)
> 
>  /* STATE MACHINE */ {
>   NEWSTYLE.START:
> -  h->rbuf = &h->gflags;
> -  h->rlen = 2;
> +  h->rbuf = &h->sbuf;
> +  h->rlen = sizeof h->sbuf.gflags;
>    SET_NEXT_STATE (%RECV_GFLAGS);
>    return 0;
> 
> @@ -127,16 +127,16 @@ handle_reply_error (struct nbd_handle *h)
>   NEWSTYLE.CHECK_GFLAGS:
>    uint32_t cflags;
> 
> -  h->gflags = be16toh (h->gflags);
> -  if ((h->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0 &&
> +  h->gflags &= be16toh (h->sbuf.gflags);
> +  if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0 &&
>        h->tls == LIBNBD_TLS_REQUIRE) {
>      SET_NEXT_STATE (%.DEAD);
> -    set_error (ENOTSUP, "handshake: server is not fixed newstyle, "
> +    set_error (ENOTSUP, "handshake: server is not using fixed newstyle, "
>                 "but handle TLS setting is 'require' (2)");
>      return 0;
>    }
> 
> -  cflags = h->gflags & (NBD_FLAG_FIXED_NEWSTYLE|NBD_FLAG_NO_ZEROES);
> +  cflags = h->gflags;
>    h->sbuf.cflags = htobe32 (cflags);
>    h->wbuf = &h->sbuf;
>    h->wlen = 4;
> @@ -148,7 +148,7 @@ handle_reply_error (struct nbd_handle *h)
>    case -1: SET_NEXT_STATE (%.DEAD); return 0;
>    case 0:
>      /* Start sending options. */
> -    if ((h->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0)
> +    if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0)
>        SET_NEXT_STATE (%OPT_EXPORT_NAME.START);
>      else
>        SET_NEXT_STATE (%OPT_STARTTLS.START);
> diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c
> index babefc0..c0d8af8 100644
> --- a/generator/states-oldstyle.c
> +++ b/generator/states-oldstyle.c
> @@ -58,6 +58,11 @@
> 
>    h->gflags = gflags;
>    debug (h, "gflags: 0x%" PRIx16, gflags);
> +  if (gflags) {
> +    set_error (0, "handshake: oldstyle server should not set gflags");
> +    SET_NEXT_STATE (%.DEAD);
> +    return 0;
> +  }
> 
>    if (nbd_internal_set_size_and_flags (h, exportsize, eflags) == -1) {
>      SET_NEXT_STATE (%.DEAD);
> diff --git a/lib/handle.c b/lib/handle.c
> index bc4206c..8ca2e5a 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -64,6 +64,8 @@ nbd_create (void)
>    h->unique = 1;
>    h->tls_verify_peer = true;
>    h->request_sr = true;
> +  h->gflags = (LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE |
> +               LIBNBD_HANDSHAKE_FLAG_NO_ZEROES);
> 
>    s = getenv ("LIBNBD_DEBUG");
>    h->debug = s && strcmp (s, "1") == 0;
> @@ -258,6 +260,22 @@ nbd_unlocked_get_request_structured_replies (struct nbd_handle *h)
>    return h->request_sr;
>  }
> 
> +int
> +nbd_unlocked_set_handshake_flags (struct nbd_handle *h,
> +                                  uint32_t flags)
> +{
> +  /* The generator already ensured flags was in range. */
> +  h->gflags = flags;
> +  return 0;
> +}
> +
> +/* NB: may_set_error = false. */
> +uint32_t
> +nbd_unlocked_get_handshake_flags (struct nbd_handle *h)
> +{
> +  return h->gflags;
> +}
> +
>  const char *
>  nbd_unlocked_get_package_name (struct nbd_handle *h)
>  {

As you say it could do with a test or interop test, but:

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]