[Libguestfs] [libnbd PATCH] api: Add set_handshake_flags for integration
Richard W.M. Jones
rjones at redhat.com
Tue Sep 17 10:11:15 UTC 2019
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/
More information about the Libguestfs
mailing list