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