Previously, we had not been doing any validation of server extent responses, which means a client query at an offset near the end of the export can result in a buggy server sending a response longer than the export length and potentially confusing the client. The NBD spec also says that an extent length should be non-zero so that a successful block status call makes progress. It is easy enough to track that the server has not overflowed the export size, and that we ensure an error on no progress even when the buggy server claims success. Since the spec says a client should be prepared for a block status result to be truncated, the client should not care whether the truncation happened at the server or at libnbd after validating the server's response. In the process, this patch reorganizes some of the code so that early exits are obvious, leading for less indentation in the success path. Adding this sanity checking now makes it easier for future patches to do orthogonal support for a server's 32- or 64-bit reply, vs. a client's 32- or 64-bit API call. Once 64-bit replies are in play, we will additionally have to worry about a 64-bit reply that overflows a 32-bit API callback without exceeding the exportsize. Similarly, since nbd_get_size() already caps export size at 63 bits (based on off_t limitations), we have guaranteed that a 64-bit API callback will never see an extent length that could appear negative in a 64-bit signed type (at least OCaml benefits from that guarantee, since its only native 64-bit integer type is signed). Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: new patch --- generator/states-reply-chunk.c | 78 +++++++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 20 deletions(-) diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index 17bb5149..735f9456 100644 --- a/generator/states-reply-chunk.c +++ b/generator/states-reply-chunk.c @@ -461,6 +461,11 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: struct command *cmd = h->reply_cmd; size_t i; uint32_t context_id; + int error; + const char *name; + uint32_t orig_len, len, flags; + uint64_t total, cap; + bool stop; switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; @@ -481,30 +486,63 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: if (context_id == h->meta_contexts.ptr[i].context_id) break; - if (i < h->meta_contexts.len) { - int error = cmd->error; - const char *name = h->meta_contexts.ptr[i].name; - - /* Need to byte-swap the entries returned, but apart from that - * we don't validate them. Yes, our 32-bit public API foolishly - * tracks the number of uint32_t instead of block descriptors; - * see _block_desc_is_multiple_of_bs_entry above. - */ - for (i = 0; i < h->bs_count * 2; ++i) - h->bs_entries[i] = be32toh (h->bs_entries[i]); - - /* Call the caller's extent function. */ - if (CALL_CALLBACK (cmd->cb.fn.extent, name, cmd->offset, - h->bs_entries, h->bs_count * 2, &error) == -1) - if (cmd->error == 0) - cmd->error = error ? error : EPROTO; - } - else + SET_NEXT_STATE (%FINISH); + if (i == h->meta_contexts.len) { /* Emit a debug message, but ignore it. */ debug (h, "server sent unexpected meta context ID %" PRIu32, context_id); + break; + } - SET_NEXT_STATE (%FINISH); + name = h->meta_contexts.ptr[i].name; + total = 0; + cap = h->exportsize - cmd->offset; + assert (cap <= h->exportsize); + + /* Need to byte-swap the entries returned. The NBD protocol + * allows truncation as long as progress is made; the client + * cannot tell the difference between a server's truncation or if + * we truncate on a length we don't like. We stop iterating on a + * zero-length extent (error only if it is the first extent), and + * on an extent beyond the exportsize (unconditional error after + * truncating to exportsize); but do not diagnose issues with the + * server's length alignments, flag values, nor compliance with + * the REQ_ONE command flag. + */ + for (i = 0, stop = false; i < h->bs_count && !stop; ++i) { + orig_len = len = be32toh (h->bs_entries[i * 2]); + flags = be32toh (h->bs_entries[i * 2 + 1]); + total += len; + if (len == 0) { + stop = true; + if (i > 0) + break; /* Skip this and later extents; we already made progress */ + /* Expose this extent as an error; we made no progress */ + cmd->error = cmd->error ? : EPROTO; + } + else if (total > cap) { + /* Expose this extent as an error, after truncating to make progress */ + stop = true; + cmd->error = cmd->error ? : EPROTO; + len -= total - cap; + } + h->bs_entries[i * 2] = len; + h->bs_entries[i * 2 + 1] = flags; + } + + /* Call the caller's extent function. Yes, our 32-bit public API + * foolishly tracks the number of uint32_t instead of block + * descriptors; see _block_desc_is_multiple_of_bs_entry above. + */ + if (stop) + debug (h, "truncating server's response at unexpected extent length %" + PRIu32 " and total %" PRIu64 " near extent %zu", + orig_len, total, i); + error = cmd->error; + if (CALL_CALLBACK (cmd->cb.fn.extent, name, cmd->offset, + h->bs_entries, i * 2, &error) == -1) + if (cmd->error == 0) + cmd->error = error ? error : EPROTO; } return 0; -- 2.41.0

The existing nbd_block_status() callback is permanently stuck with an array of uint32_t pairs (each of the h->bs_count extents is represented by a pair of uint32_t; we have to pass bs_count*2 as the array size to the callback, and the user has to compute len/2 to determine how many extents to process). Furthermore, the 32-bit nature of both values adds some constraints: we cannot represent extents larger than 4G, and status flags must fit in 32 bits. While the "base:allocation" metacontext will never exceed 32 bits, it is not hard to envision other future extension metacontexts where a 64-bit status would be useful (for example, Zoned Block Devices expressing a 64-bit offset[1]). Exposing 64-bit extents will require a new API; we now have the decision of whether to copy the existing API practice of returning a bare array containing h->bs_count*2 raw uint64_t (where the user continues the same practice of pairing those off into extents), or going with a saner idea of an array of h->bs_count structs directly describing extents. Returning an array of structs has an advantage: although both items in the 64-bit extent are 64-bit values over the wire, they are conceptually different types (an extent length is inherently bounded by 63-bit off_t limitations; while the extent flags can be a full 64 bits of unsigned flags), and this difference in types can be more precisely represented in non-C languages. This patch starts the ball rolling by adding a new arg type for use in the generator, although all match statements that process args are minimally changed to merely assert that we aren't using the type yet. Then the next few patches can provide an implementation per language binding (for ease of review), prior to a final patch that finally adds the new nbd_block_status_64() API that actually takes advantage of the new type. [1] https://zonedstorage.io/docs/linux/zbd-api Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: improve commit message [Laszlo], split into several smaller patches [Laszlo] --- generator/API.mli | 1 + generator/API.ml | 12 +++++++++++- generator/C.ml | 7 +++++++ generator/GoLang.ml | 4 ++++ generator/OCaml.ml | 4 ++++ generator/Python.ml | 5 +++++ 6 files changed, 32 insertions(+), 1 deletion(-) diff --git a/generator/API.mli b/generator/API.mli index c5bba8c2..80633018 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -52,6 +52,7 @@ and | BytesPersistOut of string * string | Closure of closure (** function pointer + void *opaque *) | Enum of string * enum (** enum/union type, int in C *) +| Extent64 of string (** extent descriptor *) | Fd of string (** file descriptor *) | Flags of string * flags (** flags, uint32_t in C *) | Int of string (** small int *) diff --git a/generator/API.ml b/generator/API.ml index 72c81657..1fbf147b 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -42,6 +42,7 @@ | BytesPersistOut of string * string | Closure of closure | Enum of string * enum +| Extent64 of string | Fd of string | Flags of string * flags | Int of string @@ -157,6 +158,14 @@ let extent_closure = "nr_entries"); CBMutable (Int "error") ] } +let extent64_closure = { + cbname = "extent64"; + cbargs = [ CBString "metacontext"; + CBUInt64 "offset"; + CBArrayAndLen (Extent64 "entries", + "nr_entries"); + CBMutable (Int "error") ] +} let list_closure = { cbname = "list"; cbargs = [ CBString "name"; CBString "description" ] @@ -166,7 +175,8 @@ let context_closure = cbargs = [ CBString "name" ] } let all_closures = [ chunk_closure; completion_closure; - debug_closure; extent_closure; list_closure; + debug_closure; extent_closure; (* extent64_closure; *) + list_closure; context_closure ] (* Enums. *) diff --git a/generator/C.ml b/generator/C.ml index a2670f70..606ba1e0 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -94,6 +94,7 @@ let | Closure { cbname } -> [ sprintf "%s_callback" cbname; sprintf "%s_user_data" cbname ] | Enum (n, _) -> [n] + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd n -> [n] | Flags (n, _) -> [n] | Int n -> [n] @@ -132,6 +133,7 @@ let | Bool _ | Closure _ | Enum _ | Fd _ | Flags _ | Int _ | Int64 _ | SizeT _ | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ] + | Extent64 _ -> assert false (* only used in extent64_closure *) let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ] @@ -191,6 +193,7 @@ and | Enum (n, _) -> if types then pr "int "; pr "%s" n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags (n, _) -> if types then pr "uint32_t "; pr "%s" n @@ -751,6 +754,7 @@ let | Bool _ | Closure _ | Enum _ | Flags _ | Fd _ | Int _ | Int64 _ | SizeT _ | SockAddrAndLen _ | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> () + | Extent64 _ -> assert false (* only used in extent64_closure *) ) args; let indent = if may_set_error then ( @@ -774,6 +778,7 @@ let pr " %s=\\\"%%s\\\" %s=%%zu" n count | Closure { cbname } -> pr " %s=%%s" cbname | Enum (n, _) -> pr " %s=%%d" n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags (n, _) -> pr " %s=0x%%x" n | Fd n | Int n -> pr " %s=%%d" n | Int64 n -> pr " %s=%%\"PRIi64\"" n @@ -812,6 +817,7 @@ let pr "%s_printable ? %s_printable : \"\", %s" n n count | Closure _ -> pr "\"<fun>\"" | Enum (n, _) -> pr "%s" n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags (n, _) -> pr "%s" n | Fd n | Int n | Int64 n | SizeT n -> pr "%s" n | SockAddrAndLen (_, len) -> pr "(int) %s" len @@ -847,6 +853,7 @@ let | Bool _ | Closure _ | Enum _ | Flags _ | Fd _ | Int _ | Int64 _ | SizeT _ | SockAddrAndLen _ | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> () + | Extent64 _ -> assert false (* only used in extent64_closure *) ) args; pr " }\n" (* Print the trace when we leave a call with debugging enabled. *) diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 8a19f65a..73df5254 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -38,6 +38,7 @@ let | BytesPersistOut (n, len) -> n | Closure { cbname } -> cbname | Enum (n, _) -> n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd n -> n | Flags (n, _) -> n | Int n -> n @@ -60,6 +61,7 @@ let | BytesPersistOut _ -> "AioBuffer" | Closure { cbname } -> sprintf "%sCallback" (camel_case cbname) | Enum (_, { enum_prefix }) -> camel_case enum_prefix + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd _ -> "int" | Flags (_, { flag_prefix }) -> camel_case flag_prefix | Int _ -> "int" @@ -252,6 +254,7 @@ let pr " c_%s.user_data = C.alloc_cbid(C.long(%s_cbid))\n" cbname cbname | Enum (n, _) -> pr " c_%s := C.int(%s)\n" n n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd n -> pr " c_%s := C.int(%s)\n" n n | Flags (n, _) -> @@ -324,6 +327,7 @@ let | BytesPersistOut (n, len) -> pr ", c_%s, c_%s" n len | Closure { cbname } -> pr ", c_%s" cbname | Enum (n, _) -> pr ", c_%s" n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd n -> pr ", c_%s" n | Flags (n, _) -> pr ", c_%s" n | Int n -> pr ", c_%s" n diff --git a/generator/OCaml.ml b/generator/OCaml.ml index efc1af22..7971ac40 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -44,6 +44,7 @@ and | Closure { cbargs } -> sprintf "(%s)" (ocaml_closuredecl_to_string cbargs) | Enum (_, { enum_prefix }) -> sprintf "%s.t" enum_prefix + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd _ -> "Unix.file_descr" | Flags (_, { flag_prefix }) -> sprintf "%s.t list" flag_prefix | Int _ -> "int" @@ -102,6 +103,7 @@ let | BytesPersistOut (n, len) -> n | Closure { cbname } -> cbname | Enum (n, _) -> n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd n -> n | Flags (n, _) -> n | Int n -> n @@ -695,6 +697,7 @@ let pr " %s_callback.free = free_user_data;\n" cbname | Enum (n, { enum_prefix }) -> pr " int %s = %s_val (%sv);\n" n enum_prefix n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Fd n -> pr " /* OCaml Unix.file_descr is just an int, at least on Unix. */\n"; pr " int %s = Int_val (%sv);\n" n n @@ -792,6 +795,7 @@ let | UInt32 _ | UInt64 _ | UIntPtr _ -> () + | Extent64 _ -> assert false (* only used in extent64_closure *) ) args; pr " CAMLreturn (rv);\n"; diff --git a/generator/Python.ml b/generator/Python.ml index beaeaa4c..3a77b81f 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -307,6 +307,7 @@ let pr ".callback = %s_wrapper, .free = free_user_data" cbname); pr " };\n" | Enum (n, _) -> pr " int %s;\n" n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags (n, _) -> pr " uint32_t %s_u32;\n" n; pr " unsigned int %s; /* really uint32_t */\n" n @@ -363,6 +364,7 @@ let "O", sprintf "&%s" n, sprintf "py_%s->buf, py_%s->len" n n | Closure { cbname } -> "O", sprintf "&py_%s_fn" cbname, cbname | Enum (n, _) -> "i", sprintf "&%s" n, n + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags (n, _) -> "I", sprintf "&%s" n, sprintf "%s_u32" n | Fd n | Int n -> "i", sprintf "&%s" n, n | Int64 n -> "L", sprintf "&%s" n, sprintf "%s_i64" n @@ -454,6 +456,7 @@ let pr " if (!chunk_user_data->view) goto out;\n" ) | Enum _ -> () + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags (n, _) -> pr " %s_u32 = %s;\n" n n | Fd _ | Int _ -> () | Int64 n -> pr " %s_i64 = %s;\n" n n @@ -552,6 +555,7 @@ let | Closure { cbname } -> pr " free_user_data (%s_user_data);\n" cbname | Enum _ -> () + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags _ -> () | Fd _ | Int _ -> () | Int64 _ -> () @@ -882,6 +886,7 @@ let | BytesPersistOut (n, _) -> n, None | Closure { cbname } -> cbname, None | Enum (n, _) -> n, None + | Extent64 _ -> assert false (* only used in extent64_closure *) | Flags (n, _) -> n, None | Fd n | Int n -> n, None | Int64 n -> n, None -- 2.41.0

See the earlier commit "Add Extent64 arg type" for rationale in supporting a new generator arg type. This patch adds the C bindings for use of Extent64: it exposes a new 'struct nbd_extent' in <libnbd.h>, as well as the code to generate the C bindings for passing an extent64_closure once a new API uses the type in an upcoming patch. Note that 'struct nbd_block_descriptor_64' in lib/nbd-protocol.h is exactly the same as what we want to use in C. But it is easier to stick a new public type in <libnbd.h> than to figure out how to expose just part of a header we only want to use internally, and leaves us free to alter nbd-protocol.h (which is shared with nbdkit) without affecting public API. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: split out of larger patch [Laszlo] --- generator/API.mli | 2 +- generator/C.ml | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/generator/API.mli b/generator/API.mli index 80633018..96626c9b 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -52,7 +52,7 @@ and | BytesPersistOut of string * string | Closure of closure (** function pointer + void *opaque *) | Enum of string * enum (** enum/union type, int in C *) -| Extent64 of string (** extent descriptor *) +| Extent64 of string (** extent descriptor, struct nbd_extent in C *) | Fd of string (** file descriptor *) | Flags of string * flags (** flags, uint32_t in C *) | Int of string (** small int *) diff --git a/generator/C.ml b/generator/C.ml index 606ba1e0..e5a2879b 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -308,6 +308,11 @@ and pr "%s, " n; if types then pr "size_t "; pr "%s" len + | CBArrayAndLen (Extent64 n, len) -> + if types then pr "nbd_extent *"; + pr "%s, " n; + if types then pr "size_t "; + pr "%s" len | CBArrayAndLen _ -> assert false | CBBytesIn (n, len) -> if types then pr "const void *"; @@ -488,6 +493,13 @@ let pr "extern int nbd_get_errno (void);\n"; pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n"; pr "\n"; + pr "/* This is used in the callback for nbd_block_status_64.\n"; + pr " */\n"; + pr "typedef struct {\n"; + pr " uint64_t length;\n"; + pr " uint64_t flags;\n"; + pr "} nbd_extent;\n"; + pr "\n"; print_closure_structs (); List.iter ( fun (name, { args; optargs; ret }) -> -- 2.41.0

See the earlier commit "Add Extent64 arg type" for rationale in supporting a new generator arg type. This patch adds the Python bindings for use of Extent64: it is fairly easy to construct a list of 2-item tuples in place with Py_BuildValue's 'K' argument for unsigned 64-bit values, without needing to write any separate helper code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: split out of larger patch [Laszlo], use K instead of O to avoid memory issues while building list --- generator/Python.ml | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 3a77b81f..761f4511 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -169,7 +169,7 @@ let pr " PyObject *py_args, *py_ret;\n"; List.iter ( function - | CBArrayAndLen (UInt32 n, _) + | CBArrayAndLen ((UInt32 n | Extent64 n), _) | CBBytesIn (n, _) | CBMutable (Int n) -> pr " PyObject *py_%s = NULL;\n" n @@ -187,6 +187,18 @@ let pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n; pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" + | CBArrayAndLen (Extent64 n, len) -> + pr " py_%s = PyList_New (%s);\n" n len; + pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n; + pr " size_t i_%s;\n" n; + pr " for (i_%s = 0; i_%s < %s; ++i_%s) {\n" n n len n; + pr " PyObject *py_e_%s = Py_BuildValue (" n; + pr_wrap ',' (fun () -> + pr "\"KK\", %s[i_%s].length, %s[i_%s].flags" n n n n); + pr ");\n"; + pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n; + pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; + pr " }\n" | CBBytesIn (n, len) -> pr " py_%s = nbd_internal_py_get_subview (data->view, %s, %s);\n" n n len; pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n @@ -205,7 +217,7 @@ let let params = List.map ( function - | CBArrayAndLen (UInt32 n, _) -> "O", sprintf "py_%s" n + | CBArrayAndLen ((UInt32 n | Extent64 n), _) -> "O", sprintf "py_%s" n | CBBytesIn (n, _) -> "O", sprintf "py_%s" n | CBInt n -> "i", n | CBInt64 n -> "L", n @@ -254,7 +266,7 @@ let pr " out:\n"; List.iter ( function - | CBArrayAndLen (UInt32 n, _) -> + | CBArrayAndLen ((UInt32 n | Extent64 n), _) -> pr " Py_XDECREF (py_%s);\n" n | CBBytesIn (n, _) -> pr " Py_XDECREF (py_%s);\n" n -- 2.41.0

Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for accessing a C array as a Go slice in order to potentially benefit from any optimizations in Go's copy() for bulk transfer of memory over naive one-at-a-time iteration. But that commit also acknowledged that no benchmark timings were performed, which would have been useful to demonstrat an actual benefit for using hack in the first place. And since we are copying data anyways (rather than using the slice to avoid a copy), and network transmission costs have a higher impact to performance than in-memory copying speed, it's hard to justify keeping the hack without hard data. What's more, while using Go's copy() on an array of C uint32_t makes sense for 32-bit extents, our corresponding 64-bit code uses a struct which does not map as nicely to Go's copy(). Using a common style between both list copying helpers is beneficial to mainenance. Additionally, at face value, converting C.size_t to int may truncate; we could avoid that risk if we were to uniformly use uint64 instead of int. But we can equally just panic if the count is oversized: our state machine guarantees that the server's response fits within 64M bytes (count will be smaller than that, since it is multiple bytes per extent entry). Suggested-by: Laszlo Ersek <lersek@redhat.com> CC: Nir Soffer <nsoffer@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: new patch to the series, but previously posted as part of the golang cleanups. Since then: rework the commit message as it is no longer a true revert, and add a panic() if count exceeds expected bounds. --- generator/GoLang.ml | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 73df5254..cc7d78b6 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -516,11 +516,16 @@ let /* Closures. */ func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 { + if (uint64(count) > 64*1024*1024) { + panic(\"violation of state machine guarantee\") + } ret := make([]uint32, int(count)) - // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices - // TODO: Use unsafe.Slice() when we require Go 1.17. - s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count] - copy(ret, s) + addr := uintptr(unsafe.Pointer(entries)) + for i := 0; i < int(count); i++ { + ptr := (*C.uint32_t)(unsafe.Pointer(addr)) + ret[i] = uint32(*ptr) + addr += unsafe.Sizeof(*ptr) + } return ret } "; -- 2.41.0

Although we don't yet have an API that takes an extent64 callback, it's easier to add the code here separately from the API. We need to temporarily mark a couple of helper functions in language bindings with '__attribute__((unused))' to keep the compiler from warning about them. The biggest change in this patch is that the array to be passed to the callback is now populated based on which style API was in use, and demonstrates that the API is orthogonal from the server's reply size. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: new patch --- lib/internal.h | 10 +++++-- generator/API.ml | 2 +- generator/states-reply-chunk.c | 49 +++++++++++++++++++++++----------- generator/OCaml.ml | 1 + generator/Python.ml | 1 + lib/aio.c | 8 ++++-- lib/handle.c | 2 +- lib/rw.c | 10 ++++--- 8 files changed, 58 insertions(+), 25 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 9cad91e3..52a17128 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -80,12 +80,14 @@ struct export { struct command_cb { union { - nbd_extent_callback extent; + nbd_extent_callback extent32; + nbd_extent64_callback extent64; nbd_chunk_callback chunk; nbd_list_callback list; nbd_context_callback context; } fn; nbd_completion_callback completion; + bool wide; }; struct nbd_handle { @@ -323,7 +325,11 @@ struct nbd_handle { struct nbd_block_descriptor_32 *narrow; struct nbd_block_descriptor_64 *wide; } bs_raw; - uint32_t *bs_cooked; /* Note that this array has 2*bs_count entries */ + union { + char *storage; + uint32_t *narrow; /* Note that this array has 2*bs_count entries... */ + nbd_extent *wide; /* ...while this is just bs_count entries. */ + } bs_cooked; /* Commands which are waiting to be issued [meaning the request * packet is sent to the server]. This is used as a simple linked diff --git a/generator/API.ml b/generator/API.ml index 1fbf147b..858d86df 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -175,7 +175,7 @@ let context_closure = cbargs = [ CBString "name" ] } let all_closures = [ chunk_closure; completion_closure; - debug_closure; extent_closure; (* extent64_closure; *) + debug_closure; extent_closure; extent64_closure; list_closure; context_closure ] diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index 66771fa6..796262d2 100644 --- a/generator/states-reply-chunk.c +++ b/generator/states-reply-chunk.c @@ -147,7 +147,8 @@ REPLY.CHUNK_REPLY.START: !h->meta_valid || h->meta_contexts.len == 0 || !bs_reply_length_ok (type, length)) goto resync; - assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); + ASSERT_MEMBER_ALIAS (struct command_cb, fn.extent32, fn.extent64); + assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent32)); if (h->extended_headers != (type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT)) { debug (h, "wrong block status reply type detected, " "this is probably a server bug"); @@ -457,7 +458,7 @@ REPLY.CHUNK_REPLY.RECV_BS_HEADER: assert (cmd->type == NBD_CMD_BLOCK_STATUS); assert (bs_reply_length_ok (type, h->payload_left)); STATIC_ASSERT (sizeof (struct nbd_block_descriptor_32) == - 2 * sizeof *h->bs_cooked, + 2 * sizeof *h->bs_cooked.narrow, _block_desc_is_multiple_of_bs_entry); ASSERT_MEMBER_ALIAS (union chunk_payload, bs_hdr_32.context_id, bs_hdr_64.context_id); @@ -481,16 +482,20 @@ REPLY.CHUNK_REPLY.RECV_BS_HEADER: } free (h->bs_raw.storage); - free (h->bs_cooked); + free (h->bs_cooked.storage); h->bs_raw.storage = malloc (h->payload_left); - h->bs_cooked = malloc (2 * h->bs_count * sizeof *h->bs_cooked); - if (h->bs_raw.storage == NULL || h->bs_cooked == NULL) { + if (cmd->cb.wide) + h->bs_cooked.storage = malloc (h->bs_count * sizeof *h->bs_cooked.wide); + else + h->bs_cooked.storage = malloc (2 * h->bs_count * + sizeof *h->bs_cooked.narrow); + if (h->bs_raw.storage == NULL || h->bs_cooked.storage == NULL) { SET_NEXT_STATE (%.DEAD); set_error (errno, "malloc"); free (h->bs_raw.storage); - free (h->bs_cooked); + free (h->bs_cooked.storage); h->bs_raw.storage = NULL; - h->bs_cooked = NULL; + h->bs_cooked.storage = NULL; return 0; } @@ -511,6 +516,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: uint64_t orig_len, len, flags; uint64_t total, cap; bool stop; + int ret; switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return 0; @@ -523,7 +529,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: assert (cmd); /* guaranteed by CHECK */ assert (cmd->type == NBD_CMD_BLOCK_STATUS); - assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); + assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent32)); assert (h->bs_count && h->bs_raw.storage); assert (h->meta_valid); @@ -579,7 +585,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: cmd->error = cmd->error ? : EPROTO; len = h->exportsize; } - if (len > UINT32_MAX) { + if (len > UINT32_MAX && !cmd->cb.wide) { /* Pick an aligned value rather than overflowing 32-bit * callback; this does not require an error. */ @@ -587,7 +593,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: len = (uint32_t)-MAX_REQUEST_SIZE; } flags = be64toh (h->bs_raw.wide[i].status_flags); - if (flags > UINT32_MAX) { + if (flags > UINT32_MAX && !cmd->cb.wide) { stop = true; if (i > 0) break; /* Skip this and later extents; we already made progress */ @@ -610,8 +616,15 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: cmd->error = cmd->error ? : EPROTO; len -= total - cap; } - h->bs_cooked[i * 2] = len; - h->bs_cooked[i * 2 + 1] = flags; + if (cmd->cb.wide) { + h->bs_cooked.wide[i].length = len; + h->bs_cooked.wide[i].flags = flags; + } + else { + assert ((len | flags) <= UINT32_MAX); + h->bs_cooked.narrow[i * 2] = len; + h->bs_cooked.narrow[i * 2 + 1] = flags; + } } /* Call the caller's extent function. Yes, our 32-bit public API @@ -623,10 +636,14 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES: PRIu64 " and total %" PRIu64 " near extent %zu", orig_len, total, i); error = cmd->error; - if (CALL_CALLBACK (cmd->cb.fn.extent, name, cmd->offset, - h->bs_cooked, i * 2, &error) == -1) - if (cmd->error == 0) - cmd->error = error ? error : EPROTO; + if (cmd->cb.wide) + ret = CALL_CALLBACK (cmd->cb.fn.extent64, name, cmd->offset, + h->bs_cooked.wide, i, &error); + else + ret = CALL_CALLBACK (cmd->cb.fn.extent32, name, cmd->offset, + h->bs_cooked.narrow, i * 2, &error); + if (ret == -1 && cmd->error == 0) + cmd->error = error ? error : EPROTO; } return 0; diff --git a/generator/OCaml.ml b/generator/OCaml.ml index 621a4348..ba4c8403 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -592,6 +592,7 @@ let pr "}\n"; pr "\n"; pr "static int\n"; + pr "__attribute__ ((unused)) /* XXX temporary hack */\n"; pr "%s_wrapper " cbname; C.print_cbarg_list ~wrap:true cbargs; pr "\n"; diff --git a/generator/Python.ml b/generator/Python.ml index 761f4511..199d0265 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -158,6 +158,7 @@ let let print_python_closure_wrapper { cbname; cbargs } = pr "/* Wrapper for %s callback. */\n" cbname; pr "static int\n"; + pr "__attribute__ ((unused)) /* XXX temporary hack */\n"; pr "%s_wrapper " cbname; C.print_cbarg_list ~wrap:true cbargs; pr "\n"; diff --git a/lib/aio.c b/lib/aio.c index a419ac32..cf9421ec 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -32,8 +32,12 @@ void nbd_internal_retire_and_free_command (struct command *cmd) { /* Free the callbacks. */ - if (cmd->type == NBD_CMD_BLOCK_STATUS) - FREE_CALLBACK (cmd->cb.fn.extent); + if (cmd->type == NBD_CMD_BLOCK_STATUS) { + if (cmd->cb.wide) + FREE_CALLBACK (cmd->cb.fn.extent64); + else + FREE_CALLBACK (cmd->cb.fn.extent32); + } if (cmd->type == NBD_CMD_READ) FREE_CALLBACK (cmd->cb.fn.chunk); FREE_CALLBACK (cmd->cb.completion); diff --git a/lib/handle.c b/lib/handle.c index 1d3aae63..8adb9c73 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -134,7 +134,7 @@ nbd_close (struct nbd_handle *h) string_vector_empty (&h->querylist); free (h->bs_raw.storage); - free (h->bs_cooked); + free (h->bs_cooked.storage); nbd_internal_reset_size_and_flags (h); for (i = 0; i < h->meta_contexts.len; ++i) free (h->meta_contexts.ptr[i].name); diff --git a/lib/rw.c b/lib/rw.c index 8b2bd4cc..15b309ee 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -290,8 +290,12 @@ nbd_internal_command_common (struct nbd_handle *h, err: /* Since we did not queue the command, we must free the callbacks. */ if (cb) { - if (type == NBD_CMD_BLOCK_STATUS) - FREE_CALLBACK (cb->fn.extent); + if (type == NBD_CMD_BLOCK_STATUS) { + if (cb->wide) + FREE_CALLBACK (cb->fn.extent32); + else + FREE_CALLBACK (cb->fn.extent64); + } if (type == NBD_CMD_READ) FREE_CALLBACK (cb->fn.chunk); FREE_CALLBACK (cb->completion); @@ -487,7 +491,7 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, nbd_completion_callback *completion, uint32_t flags) { - struct command_cb cb = { .fn.extent = *extent, + struct command_cb cb = { .fn.extent32 = *extent, .wide = false, .completion = *completion }; if (h->strict & LIBNBD_STRICT_COMMANDS) { -- 2.41.0

Add unit test coverage to prove that the new API works in each of C, Python, OCaml, and Go bindings. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: new patch, split out from API addition itself --- python/t/465-block-status-64.py | 56 ++++++++++ ocaml/tests/Makefile.am | 1 + ocaml/tests/test_465_block_status_64.ml | 58 +++++++++++ tests/meta-base-allocation.c | 104 ++++++++++++++++--- golang/Makefile.am | 1 + golang/libnbd_465_block_status_64_test.go | 119 ++++++++++++++++++++++ 6 files changed, 326 insertions(+), 13 deletions(-) create mode 100644 python/t/465-block-status-64.py create mode 100644 ocaml/tests/test_465_block_status_64.ml create mode 100644 golang/libnbd_465_block_status_64_test.go diff --git a/python/t/465-block-status-64.py b/python/t/465-block-status-64.py new file mode 100644 index 00000000..a8a8eaea --- /dev/null +++ b/python/t/465-block-status-64.py @@ -0,0 +1,56 @@ +# libnbd Python bindings +# Copyright Red Hat +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program 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 General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import os + +import nbd + +script = "%s/../tests/meta-base-allocation.sh" % os.getenv("srcdir", ".") + +h = nbd.NBD() +h.add_meta_context("base:allocation") +h.connect_command(["nbdkit", "-s", "--exit-with-parent", "-v", "sh", script]) + +entries = [] + + +def f(user_data, metacontext, offset, e, err): + global entries + assert user_data == 42 + assert err.value == 0 + if metacontext != "base:allocation": + return + entries = e + + +h.block_status_64(65536, 0, lambda *args: f(42, *args)) +print("entries = %r" % entries) +assert entries == [(8192, 0), + (8192, 1), + (16384, 3), + (16384, 2), + (16384, 0)] + +h.block_status_64(1024, 32256, lambda *args: f(42, *args)) +print("entries = %r" % entries) +assert entries == [(512, 3), + (16384, 2)] + +h.block_status_64(1024, 32256, lambda *args: f(42, *args), + nbd.CMD_FLAG_REQ_ONE) +print("entries = %r" % entries) +assert entries == [(512, 3)] diff --git a/ocaml/tests/Makefile.am b/ocaml/tests/Makefile.am index 20546350..4ddd0f9f 100644 --- a/ocaml/tests/Makefile.am +++ b/ocaml/tests/Makefile.am @@ -40,6 +40,7 @@ ML_TESTS = \ test_405_pread_structured.ml \ test_410_pwrite.ml \ test_460_block_status.ml \ + test_465_block_status_64.ml \ test_500_aio_pread.ml \ test_505_aio_pread_structured_callback.ml \ test_510_aio_pwrite.ml \ diff --git a/ocaml/tests/test_465_block_status_64.ml b/ocaml/tests/test_465_block_status_64.ml new file mode 100644 index 00000000..0634a025 --- /dev/null +++ b/ocaml/tests/test_465_block_status_64.ml @@ -0,0 +1,58 @@ +(* hey emacs, this is OCaml code: -*- tuareg -*- *) +(* libnbd OCaml test case + * 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 + *) + +open Printf + +let script = + try + let srcdir = Sys.getenv "srcdir" in + sprintf "%s/../../tests/meta-base-allocation.sh" srcdir + with + Not_found -> failwith "error: srcdir is not defined" + +let entries = ref [||] +let f user_data metacontext offset e err = + assert (user_data = 42); + assert (!err = 0); + if metacontext = "base:allocation" then + entries := e; + 0 + +let () = + let nbd = NBD.create () in + NBD.add_meta_context nbd "base:allocation"; + NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "-v"; + "sh"; script]; + + NBD.block_status_64 nbd 65536_L 0_L (f 42); + assert (!entries = [| 8192_L, 0_L; + 8192_L, 1_L; + 16384_L, 3_L; + 16384_L, 2_L; + 16384_L, 0_L |]); + + NBD.block_status_64 nbd 1024_L 32256_L (f 42); + assert (!entries = [| 512_L, 3_L; + 16384_L, 2_L |]); + + let flags = let open NBD.CMD_FLAG in [REQ_ONE] in + NBD.block_status_64 nbd 1024_L 32256_L (f 42) ~flags; + assert (!entries = [| 512_L, 3_L |]) + +let () = Gc.compact () diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c index a7b3af09..7697b5da 100644 --- a/tests/meta-base-allocation.c +++ b/tests/meta-base-allocation.c @@ -32,10 +32,13 @@ #define BOGUS_CONTEXT "x-libnbd:nosuch" -static int check_extent (void *data, - const char *metacontext, - uint64_t offset, - uint32_t *entries, size_t nr_entries, int *error); +static int check_extent32 (void *data, const char *metacontext, + uint64_t offset, + uint32_t *entries, size_t nr_entries, int *error); + +static int check_extent64 (void *data, const char *metacontext, + uint64_t offset, + nbd_extent *entries, size_t nr_entries, int *error); int main (int argc, char *argv[]) @@ -43,8 +46,10 @@ main (int argc, char *argv[]) struct nbd_handle *nbd; char plugin_path[256]; int id; - nbd_extent_callback extent_callback = { .callback = check_extent, - .user_data = &id }; + nbd_extent_callback extent32_callback = { .callback = check_extent32, + .user_data = &id }; + nbd_extent64_callback extent64_callback = { .callback = check_extent64, + .user_data = &id }; int r; const char *s; char *tmp; @@ -150,23 +155,36 @@ main (int argc, char *argv[]) /* Read the block status. */ id = 1; - if (nbd_block_status (nbd, 65536, 0, extent_callback, 0) == -1) { + if (nbd_block_status (nbd, 65536, 0, extent32_callback, 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_block_status_64 (nbd, 65536, 0, extent64_callback, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } id = 2; - if (nbd_block_status (nbd, 1024, 32768-512, extent_callback, 0) == -1) { + if (nbd_block_status (nbd, 1024, 32768-512, extent32_callback, 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_block_status_64 (nbd, 1024, 32768-512, extent64_callback, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } id = 3; - if (nbd_block_status (nbd, 1024, 32768-512, extent_callback, + if (nbd_block_status (nbd, 1024, 32768-512, extent32_callback, LIBNBD_CMD_FLAG_REQ_ONE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } + if (nbd_block_status_64 (nbd, 1024, 32768-512, extent64_callback, + LIBNBD_CMD_FLAG_REQ_ONE) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_shutdown (nbd, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); @@ -178,10 +196,8 @@ main (int argc, char *argv[]) } static int -check_extent (void *data, - const char *metacontext, - uint64_t offset, - uint32_t *entries, size_t nr_entries, int *error) +check_extent32 (void *data, const char *metacontext, uint64_t offset, + uint32_t *entries, size_t nr_entries, int *error) { size_t i; int id; @@ -235,3 +251,65 @@ check_extent (void *data, return 0; } + +static int +check_extent64 (void *data, const char *metacontext, uint64_t offset, + nbd_extent *entries, size_t nr_entries, int *error) +{ + size_t i; + int id; + + id = * (int *)data; + + printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ", " + "nr_entries=%zu, error=%d\n", + id, metacontext, offset, nr_entries, *error); + + assert (*error == 0); + if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) { + for (i = 0; i < nr_entries; i++) { + printf ("\t%zu\tlength=%" PRIu64 ", status=%" PRIu64 "\n", + i, entries[i].length, entries[i].flags); + } + fflush (stdout); + + switch (id) { + case 1: + assert (nr_entries == 5); + assert (entries[0].length == 8192); + assert (entries[0].flags == 0); + assert (entries[1].length == 8192); + assert (entries[1].flags == LIBNBD_STATE_HOLE); + assert (entries[2].length == 16384); + assert (entries[2].flags == (LIBNBD_STATE_HOLE|LIBNBD_STATE_ZERO)); + assert (entries[3].length == 16384); + assert (entries[3].flags == LIBNBD_STATE_ZERO); + assert (entries[4].length == 16384); + assert (entries[4].flags == 0); + break; + + case 2: + assert (nr_entries == 2); + assert (entries[0].length == 512); + assert (entries[0].flags == (LIBNBD_STATE_HOLE|LIBNBD_STATE_ZERO)); + assert (entries[1].length == 16384); + assert (entries[1].flags == LIBNBD_STATE_ZERO); + break; + + case 3: + assert (nr_entries == 1); + assert (entries[0].length == 512); + assert (entries[0].flags == (LIBNBD_STATE_HOLE|LIBNBD_STATE_ZERO)); + break; + + default: + abort (); + } + + } + else + fprintf (stderr, "warning: ignored unexpected meta context %s\n", + metacontext); + + return 0; +} diff --git a/golang/Makefile.am b/golang/Makefile.am index fac65248..b4151250 100644 --- a/golang/Makefile.am +++ b/golang/Makefile.am @@ -43,6 +43,7 @@ source_files = \ libnbd_405_pread_structured_test.go \ libnbd_410_pwrite_test.go \ libnbd_460_block_status_test.go \ + libnbd_465_block_status_64_test.go \ libnbd_500_aio_pread_test.go \ libnbd_510_aio_pwrite_test.go \ libnbd_590_aio_copy_test.go \ diff --git a/golang/libnbd_465_block_status_64_test.go b/golang/libnbd_465_block_status_64_test.go new file mode 100644 index 00000000..7659a21d --- /dev/null +++ b/golang/libnbd_465_block_status_64_test.go @@ -0,0 +1,119 @@ +/* libnbd golang tests + * 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 + */ + +package libnbd + +import ( + "fmt" + "os" + "strings" + "testing" +) + +var entries64 []LibnbdExtent + +func mcf64(metacontext string, offset uint64, e []LibnbdExtent, error *int) int { + if *error != 0 { + panic("expected *error == 0") + } + if metacontext == "base:allocation" { + entries64 = e + } + return 0 +} + +// Seriously WTF? +func mc64_compare(a1 []LibnbdExtent, a2 []LibnbdExtent) bool { + if len(a1) != len(a2) { + return false + } + for i := 0; i < len(a1); i++ { + if a1[i] != a2[i] { + return false + } + } + return true +} + +func mc64_to_string(a []LibnbdExtent) string { + ss := make([]string, len(a)) + for i := 0; i < len(a); i++ { + ss[i] = fmt.Sprintf("%#v", a[i]) + } + return strings.Join(ss, ", ") +} + +func Test465BlockStatus64(t *testing.T) { + srcdir := os.Getenv("abs_top_srcdir") + script := srcdir + "/tests/meta-base-allocation.sh" + + h, err := Create() + if err != nil { + t.Fatalf("could not create handle: %s", err) + } + defer h.Close() + + err = h.AddMetaContext("base:allocation") + if err != nil { + t.Fatalf("%s", err) + } + err = h.ConnectCommand([]string{ + "nbdkit", "-s", "--exit-with-parent", "-v", + "sh", script, + }) + if err != nil { + t.Fatalf("%s", err) + } + + err = h.BlockStatus64(65536, 0, mcf64, nil) + if err != nil { + t.Fatalf("%s", err) + } + if !mc64_compare(entries64, []LibnbdExtent{ + {8192, 0}, + {8192, 1}, + {16384, 3}, + {16384, 2}, + {16384, 0}, + }) { + t.Fatalf("unexpected entries (1): %s", mc64_to_string(entries64)) + } + + err = h.BlockStatus64(1024, 32256, mcf64, nil) + if err != nil { + t.Fatalf("%s", err) + } + if !mc64_compare(entries64, []LibnbdExtent{ + {512, 3}, + {16384, 2}, + }) { + t.Fatalf("unexpected entries (2): %s", mc64_to_string(entries64)) + } + + var optargs BlockStatus64Optargs + optargs.FlagsSet = true + optargs.Flags = CMD_FLAG_REQ_ONE + err = h.BlockStatus64(1024, 32256, mcf64, &optargs) + if err != nil { + t.Fatalf("%s", err) + } + if !mc64_compare(entries64, []LibnbdExtent{{512, 3}}) { + t.Fatalf("unexpected entries (3): %s", mc64_to_string(entries64)) + } + +} -- 2.41.0

The new NBD_OPT_EXTENDED_HEADERS feature is worth using by default, but there may be cases where the user explicitly wants to stick with the older 32-bit headers. nbd_set_request_extended_headers() will let the client override the default, nbd_get_request_extended_headers() determines the current state of the request, and nbd_get_extended_headers_negotiated() determines what the client and server actually settled on. These use nbd_set_request_structured_replies() and friends as a template. Note that this patch just adds the API for controlling the defaults, but ignores the state variable; a later one will then tweak the state machine to actually request structured headers when the state variable is set, as well as add nbd_opt_extended_headers() for manual control. The intent is that because extended headers takes priority over structured replies, the functions will interact as follows during the automatic handshaking that takes place prior to nbd_opt_set_mode() relinquishing control to the client in negotiation mode: 1. client default: request_eh=1, request_sr=1 => try EXTENDED_HEADERS - server supports it: nothing further to do, use extended headers, but also report structured replies as active - server lacks it: behave like case 2 2. request_eh=0 (explicit client downgrade), request_sr=1 (left at default) => try STRUCTURED_REPLY - server supports it: expect structured replies - server lacks it: expect simple replies 3. request_sr=0 (explicit client downgrade), request_eh ignored => don't try either handshake - expect simple replies Client code that wants to manually force simple replies only has to disable structured replies; and only code that wants to disable extended headers but still use structured replies has to use the new nbd_set_request_extended_headers() API. Until a later patch adds an explicit API nbd_opt_extended_headers(), there is no way to request extended headers after structured replies are already negotiated. Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Richard W.M. Jones <rjones@redhat.com> --- v4: no code changes --- lib/internal.h | 1 + generator/API.ml | 115 ++++++++++++++++-- generator/states-newstyle-opt-starttls.c | 1 + .../states-newstyle-opt-structured-reply.c | 3 +- lib/handle.c | 23 ++++ python/t/110-defaults.py | 1 + python/t/120-set-non-defaults.py | 2 + ocaml/tests/test_110_defaults.ml | 2 + ocaml/tests/test_120_set_non_defaults.ml | 3 + golang/libnbd_110_defaults_test.go | 8 ++ golang/libnbd_120_set_non_defaults_test.go | 12 ++ 11 files changed, 160 insertions(+), 11 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 52a17128..efbb5abb 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -113,6 +113,7 @@ struct nbd_handle { char *tls_psk_file; /* PSK filename, NULL = no PSK */ /* Extended headers. */ + bool request_eh; /* Whether to request extended headers */ bool extended_headers; /* If we negotiated NBD_OPT_EXTENDED_HEADERS */ /* Desired metadata contexts. */ diff --git a/generator/API.ml b/generator/API.ml index 6843223f..36033817 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -808,6 +808,77 @@ "get_tls_psk_file", { }; *) + "set_request_extended_headers", { + default_call with + args = [Bool "request"]; ret = RErr; + permitted_states = [ Created ]; + shortdesc = "control use of extended headers"; + longdesc = "\ +By default, libnbd tries to negotiate extended headers with the +server, as this protocol extension permits the use of 64-bit +zero, trim, and block status actions. However, +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. + +For backwards compatibility, the setting of this knob is ignored +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 "set_handshake_flags"; Link "set_strict_mode"; + Link "get_extended_headers_negotiated"; + Link "zero"; Link "trim"; Link "cache"; + Link "block_status_64"; + Link "set_request_structured_replies"]; + }; + + "get_request_extended_headers", { + default_call with + args = []; ret = RBool; + may_set_error = false; + shortdesc = "see if extended headers are attempted"; + longdesc = "\ +Return the state of the request extended headers flag on this +handle. + +B<Note:> If you want to find out if extended headers were actually +negotiated on a particular connection use +L<nbd_get_extended_headers_negotiated(3)> instead."; + see_also = [Link "set_request_extended_headers"; + Link "get_extended_headers_negotiated"; + Link "get_request_extended_headers"]; + }; + + "get_extended_headers_negotiated", { + default_call with + args = []; ret = RBool; + permitted_states = [ Negotiating; Connected; Closed ]; + 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. + +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 +to express the length. But even when extended headers are +supported, the server may enforce other limits, visible through +L<nbd_get_block_size(3)>. + +Note that when extended headers are negotiated, you should +prefer the use of L<nbd_block_status_64(3)> instead of +L<nbd_block_status(3)> if any of the meta contexts you requested +via L<nbd_add_meta_context(3)> might return 64-bit status +values; however, all of the well-known meta contexts covered +by current C<LIBNBD_CONTEXT_*> constants only return 32-bit +status."; + see_also = [Link "set_request_extended_headers"; + Link "get_request_extended_headers"; + Link "zero"; Link "trim"; Link "cache"; + Link "block_status_64"; Link "get_block_size"; + Link "get_protocol"; + Link "get_structured_replies_negotiated"]; + }; + "set_request_structured_replies", { default_call with args = [Bool "request"]; ret = RErr; @@ -821,12 +892,16 @@ "set_request_structured_replies", { rather than find a way to alter the server to fail the negotiation request. It is also useful to set this to false prior to using L<nbd_set_opt_mode(3)> if it is desired to control when to send -L<nbd_opt_structured_reply(3)> during negotiation."; +L<nbd_opt_structured_reply(3)> during negotiation. + +Note that setting this knob to false also disables any automatic +request for extended headers."; see_also = [Link "get_request_structured_replies"; Link "set_handshake_flags"; Link "set_strict_mode"; Link "opt_structured_reply"; Link "get_structured_replies_negotiated"; - Link "can_meta_context"; Link "can_df"]; + Link "can_meta_context"; Link "can_df"; + Link "set_request_extended_headers"]; }; "get_request_structured_replies", { @@ -842,7 +917,8 @@ "get_request_structured_replies", { negotiated on a particular connection use L<nbd_get_structured_replies_negotiated(3)> instead."; see_also = [Link "set_request_structured_replies"; - Link "get_structured_replies_negotiated"]; + Link "get_structured_replies_negotiated"; + Link "get_request_extended_headers"]; }; "get_structured_replies_negotiated", { @@ -854,11 +930,17 @@ "get_structured_replies_negotiated", { After connecting you may call this to find out if the connection is using structured replies. Note that this setting is sticky; this can return true even after a second L<nbd_opt_structured_reply(3)> -returns false because the server detected a duplicate request."; +returns false because the server detected a duplicate request. + +Note that if the connection negotiates extended headers, this +function returns true (as extended headers imply structured +replies) even if no explicit request for structured replies was +attempted."; see_also = [Link "set_request_structured_replies"; Link "get_request_structured_replies"; Link "opt_structured_reply"; - Link "get_protocol"]; + Link "get_protocol"; + Link "get_extended_headers_negotiated"]; }; "set_request_meta_context", { @@ -2623,7 +2705,9 @@ "trim", { or there is an error. Note this will generally return an error if L<nbd_can_trim(3)> is false or L<nbd_is_read_only(3)> is true. -Note that not all servers can support a C<count> of 4GiB or larger. +Note that not all servers can support a C<count> of 4GiB or larger; +L<nbd_get_extended_headers_negotiated(3)> indicates which servers +will parse a request larger than 32 bits. The NBD protocol does not yet have a way for a client to learn if the server will enforce an even smaller maximum trim size, although a future extension may add a constraint visible in @@ -2654,7 +2738,9 @@ "cache", { this command. Note this will generally return an error if L<nbd_can_cache(3)> is false. -Note that not all servers can support a C<count> of 4GiB or larger. +Note that not all servers can support a C<count> of 4GiB or larger; +L<nbd_get_extended_headers_negotiated(3)> indicates which servers +will parse a request larger than 32 bits. The NBD protocol does not yet have a way for a client to learn if the server will enforce an even smaller maximum cache size, although a future extension may add a constraint visible in @@ -2683,7 +2769,9 @@ "zero", { or there is an error. Note this will generally return an error if L<nbd_can_zero(3)> is false or L<nbd_is_read_only(3)> is true. -Note that not all servers can support a C<count> of 4GiB or larger. +Note that not all servers can support a C<count> of 4GiB or larger; +L<nbd_get_extended_headers_negotiated(3)> indicates which servers +will parse a request larger than 32 bits. The NBD protocol does not yet have a way for a client to learn if the server will enforce an even smaller maximum zero size, although a future extension may add a constraint visible in @@ -2723,7 +2811,9 @@ "block_status", { are supported, the number of blocks and cumulative length of those blocks need not be identical between contexts. -Note that not all servers can support a C<count> of 4GiB or larger. +Note that not all servers can support a C<count> of 4GiB or larger; +L<nbd_get_extended_headers_negotiated(3)> indicates which servers +will parse a request larger than 32 bits. The NBD protocol does not yet have a way for a client to learn if the server will enforce an even smaller maximum block status size, although a future extension may add a constraint visible in @@ -2801,7 +2891,9 @@ "block_status_64", { are supported, the number of blocks and cumulative length of those blocks need not be identical between contexts. -Note that not all servers can support a C<count> of 4GiB or larger. +Note that not all servers can support a C<count> of 4GiB or larger; +L<nbd_get_extended_headers_negotiated(3)> indicates which servers +will parse a request larger than 32 bits. The NBD protocol does not yet have a way for a client to learn if the server will enforce an even smaller maximum block status size, although a future extension may add a constraint visible in @@ -4024,6 +4116,9 @@ let first_version = (* Added in 1.17.x development cycle, will be stable and supported in 1.18. *) "block_status_64", (1, 18); "aio_block_status_64", (1, 18); + "set_request_extended_headers", (1, 18); + "get_request_extended_headers", (1, 18); + "get_extended_headers_negotiated", (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-starttls.c b/generator/states-newstyle-opt-starttls.c index 700604dd..e497548c 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -86,6 +86,7 @@ NEWSTYLE.OPT_STARTTLS.CHECK_REPLY: } nbd_internal_reset_size_and_flags (h); h->structured_replies = false; + h->extended_headers = false; h->meta_valid = false; new_sock = nbd_internal_crypto_create_session (h, h->sock); if (new_sock == NULL) { diff --git a/generator/states-newstyle-opt-structured-reply.c b/generator/states-newstyle-opt-structured-reply.c index 18245054..bb94b70a 100644 --- a/generator/states-newstyle-opt-structured-reply.c +++ b/generator/states-newstyle-opt-structured-reply.c @@ -25,7 +25,7 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.START: assert (h->opt_mode); else { assert (CALLBACK_IS_NULL (h->opt_cb.completion)); - if (!h->request_sr) { + if (!h->request_sr || h->structured_replies) { if (h->opt_mode) SET_NEXT_STATE (%.NEGOTIATING); else @@ -84,6 +84,7 @@ NEWSTYLE.OPT_STRUCTURED_REPLY.CHECK_REPLY: err = 0; break; case NBD_REP_ERR_INVALID: + case NBD_REP_ERR_EXT_HEADER_REQD: err = EINVAL; /* fallthrough */ default: diff --git a/lib/handle.c b/lib/handle.c index 8adb9c73..802dcf53 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -67,6 +67,7 @@ nbd_create (void) h->unique = 1; h->tls_verify_peer = true; + h->request_eh = true; h->request_sr = true; h->request_meta = true; h->request_block_size = true; @@ -440,6 +441,28 @@ nbd_unlocked_clear_meta_contexts (struct nbd_handle *h) return 0; } + +int +nbd_unlocked_set_request_extended_headers (struct nbd_handle *h, + bool request) +{ + h->request_eh = request; + return 0; +} + +/* NB: may_set_error = false. */ +int +nbd_unlocked_get_request_extended_headers (struct nbd_handle *h) +{ + return h->request_eh; +} + +int +nbd_unlocked_get_extended_headers_negotiated (struct nbd_handle *h) +{ + return h->extended_headers; +} + int nbd_unlocked_set_request_structured_replies (struct nbd_handle *h, bool request) diff --git a/python/t/110-defaults.py b/python/t/110-defaults.py index ed60b786..2e59bf39 100644 --- a/python/t/110-defaults.py +++ b/python/t/110-defaults.py @@ -21,6 +21,7 @@ assert h.get_export_name() == "" assert h.get_full_info() is False assert h.get_tls() == nbd.TLS_DISABLE +assert h.get_request_extended_headers() is True assert h.get_request_structured_replies() is True assert h.get_request_meta_context() is True assert h.get_request_block_size() is True diff --git a/python/t/120-set-non-defaults.py b/python/t/120-set-non-defaults.py index a2fafe0c..3320f8cf 100644 --- a/python/t/120-set-non-defaults.py +++ b/python/t/120-set-non-defaults.py @@ -31,6 +31,8 @@ if h.supports_tls(): h.set_tls(nbd.TLS_ALLOW) assert h.get_tls() == nbd.TLS_ALLOW +h.set_request_extended_headers(False) +assert h.get_request_extended_headers() is False h.set_request_structured_replies(False) assert h.get_request_structured_replies() is False h.set_request_meta_context(False) diff --git a/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml index b95cd1f0..d7b0463f 100644 --- a/ocaml/tests/test_110_defaults.ml +++ b/ocaml/tests/test_110_defaults.ml @@ -26,6 +26,8 @@ let assert (not info); let tls = NBD.get_tls nbd in assert (tls = NBD.TLS.DISABLE); + let eh = NBD.get_request_extended_headers nbd in + assert (eh = true); let sr = NBD.get_request_structured_replies nbd in assert sr; let meta = NBD.get_request_meta_context nbd in diff --git a/ocaml/tests/test_120_set_non_defaults.ml b/ocaml/tests/test_120_set_non_defaults.ml index 478a88f3..4f5c2eb5 100644 --- a/ocaml/tests/test_120_set_non_defaults.ml +++ b/ocaml/tests/test_120_set_non_defaults.ml @@ -39,6 +39,9 @@ let let tls = NBD.get_tls nbd in assert (tls = NBD.TLS.ALLOW); ); + NBD.set_request_extended_headers nbd false; + let eh = NBD.get_request_extended_headers nbd in + assert (eh = false); NBD.set_request_structured_replies nbd false; let sr = NBD.get_request_structured_replies nbd in assert (not sr); diff --git a/golang/libnbd_110_defaults_test.go b/golang/libnbd_110_defaults_test.go index 119cb7ca..510baa39 100644 --- a/golang/libnbd_110_defaults_test.go +++ b/golang/libnbd_110_defaults_test.go @@ -51,6 +51,14 @@ func Test110Defaults(t *testing.T) { t.Fatalf("unexpected tls state") } + eh, err := h.GetRequestExtendedHeaders() + if err != nil { + t.Fatalf("could not get extended headers state: %s", err) + } + if eh != true { + t.Fatalf("unexpected extended headers state") + } + sr, err := h.GetRequestStructuredReplies() if err != nil { t.Fatalf("could not get structured replies state: %s", err) diff --git a/golang/libnbd_120_set_non_defaults_test.go b/golang/libnbd_120_set_non_defaults_test.go index b096c4f6..4c6bb3ce 100644 --- a/golang/libnbd_120_set_non_defaults_test.go +++ b/golang/libnbd_120_set_non_defaults_test.go @@ -81,6 +81,18 @@ func Test120SetNonDefaults(t *testing.T) { } } + err = h.SetRequestExtendedHeaders(false) + if err != nil { + t.Fatalf("could not set extended headers state: %s", err) + } + eh, err := h.GetRequestExtendedHeaders() + if err != nil { + t.Fatalf("could not get extended headers state: %s", err) + } + if eh != false { + t.Fatalf("unexpected extended headers state") + } + err = h.SetRequestStructuredReplies(false) if err != nil { t.Fatalf("could not set structured replies state: %s", err) -- 2.41.0

Add another bit of overall server information, as well as a '--has extended-headers' silent query. For now, the testsuite is written assuming that when nbdkit finally adds extended headers support, it will also add a --no-eh kill switch comparable to its existing --no-sr switch. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: s/can/has/ [Rich] --- info/nbdinfo.pod | 9 +++++++++ info/can.c | 9 +++++++++ info/info-can.sh | 27 +++++++++++++++++++++++++++ info/info-packets.sh | 17 ++++++++++++++++- info/main.c | 7 ++++++- 5 files changed, 67 insertions(+), 2 deletions(-) diff --git a/info/nbdinfo.pod b/info/nbdinfo.pod index 72193c24..2bdd573f 100644 --- a/info/nbdinfo.pod +++ b/info/nbdinfo.pod @@ -86,6 +86,7 @@ the I<--json> parameter: "protocol": "newstyle-fixed", "TLS": false, "structured": true, + "extended": false, "exports": [ { "export-name": "", @@ -165,6 +166,11 @@ Test if the NBD URI connection is using TLS. Test if server has support for structured replies (a prerequisite for supporting block status commands). +=item nbdinfo --has extended-headers URI + +Test if server supports extended headers (a prerequisite for +supporting 64-bit commands; implies structured replies as well). + =item nbdinfo --is rotational URI Test if the server export is backed by something which behaves like a @@ -367,6 +373,8 @@ When using I<--list>, the default is I<--no-content> (since downloading from each export is expensive). To enable content probing use I<--list --content>. +=item B<--has extended-headers> + =item B<--has structured-reply> Test properties of the NBD server connection. The command does not @@ -377,6 +385,7 @@ indicate an error querying the flag). For further information see the L<NBD protocol|https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md> and the following libnbd functions: +L<nbd_get_extended_headers_negotiated(3)>, L<nbd_get_structured_replies_negotiated(3)>. =item B<--is read-only> diff --git a/info/can.c b/info/can.c index 8514fd5a..8547ef04 100644 --- a/info/can.c +++ b/info/can.c @@ -50,6 +50,15 @@ do_can (void) strcasecmp (can, "structured_replies") == 0) feature = nbd_get_structured_replies_negotiated (nbd); + else if (strcasecmp (can, "eh") == 0 || + strcasecmp (can, "extended header") == 0 || + strcasecmp (can, "extended-header") == 0 || + strcasecmp (can, "extended_header") == 0 || + strcasecmp (can, "extended headers") == 0 || + strcasecmp (can, "extended-headers") == 0 || + strcasecmp (can, "extended_headers") == 0) + feature = nbd_get_extended_headers_negotiated (nbd); + else if (strcasecmp (can, "readonly") == 0 || strcasecmp (can, "read-only") == 0 || strcasecmp (can, "read_only") == 0) diff --git a/info/info-can.sh b/info/info-can.sh index 13ef1032..5d9f2e89 100755 --- a/info/info-can.sh +++ b/info/info-can.sh @@ -61,6 +61,33 @@ esac EOF test $st = 2 +# --has extended-headers cannot be positively tested until nbdkit gains +# --no-eh support. Otherwise, it is similar to --has structured-reply. + +no_eh= +if nbdkit --no-eh --help >/dev/null 2>/dev/null; then + no_eh=--no-eh + nbdkit -v -U - sh - \ + --run '$VG nbdinfo --has extended-headers "nbd+unix:///?socket=$unixsocket"' <<'EOF' +case "$1" in + get_size) echo 1024 ;; + pread) ;; + *) exit 2 ;; +esac +EOF +fi + +st=0 +nbdkit -v -U - $no_eh sh - \ + --run '$VG nbdinfo --has extended-headers "nbd+unix:///?socket=$unixsocket"' <<'EOF' || st=$? +case "$1" in + get_size) echo 1024 ;; + pread) ;; + *) exit 2 ;; +esac +EOF +test $st = 2 + # --can cache and --can fua require special handling because in # nbdkit-sh-plugin we must print "native" or "none". Also the can_fua # flag is only sent if the export is writable (hence can_write below). diff --git a/info/info-packets.sh b/info/info-packets.sh index 2460052e..410faef8 100755 --- a/info/info-packets.sh +++ b/info/info-packets.sh @@ -27,12 +27,27 @@ requires nbdkit --no-sr memory --version out=info-packets.out cleanup_fn rm -f $out +# Older nbdkit does not support extended headers; --no-eh is a reliable +# witness of whether nbdkit is new enough. + +no_eh= +if nbdkit --no-eh --help >/dev/null 2>/dev/null; then + no_eh=--no-eh +fi + nbdkit --no-sr -U - memory size=1M \ --run '$VG nbdinfo "nbd+unix:///?socket=$unixsocket"' > $out cat $out grep "protocol: .*using simple packets" $out -nbdkit -U - memory size=1M \ +nbdkit $no_eh -U - memory size=1M \ --run '$VG nbdinfo "nbd+unix:///?socket=$unixsocket"' > $out cat $out grep "protocol: .*using structured packets" $out + +if test x != "x$no_eh"; then + nbdkit -U - memory size=1M \ + --run '$VG nbdinfo "nbd+unix:///?socket=$unixsocket"' > $out + cat $out + grep "protocol: .*using extended packets" $out +fi diff --git a/info/main.c b/info/main.c index dbcc5a14..572dd536 100644 --- a/info/main.c +++ b/info/main.c @@ -306,11 +306,13 @@ main (int argc, char *argv[]) const char *protocol; int tls_negotiated; int sr_negotiated; + int eh_negotiated; /* Print per-connection fields. */ protocol = nbd_get_protocol (nbd); tls_negotiated = nbd_get_tls_negotiated (nbd); sr_negotiated = nbd_get_structured_replies_negotiated (nbd); + eh_negotiated = nbd_get_extended_headers_negotiated (nbd); if (!json_output) { if (protocol) { @@ -318,8 +320,9 @@ main (int argc, char *argv[]) fprintf (fp, "protocol: %s", protocol); if (tls_negotiated >= 0) fprintf (fp, " %s TLS", tls_negotiated ? "with" : "without"); - if (sr_negotiated >= 0) + if (eh_negotiated >= 0 && sr_negotiated >= 0) fprintf (fp, ", using %s packets", + eh_negotiated ? "extended" : sr_negotiated ? "structured" : "simple"); fprintf (fp, "\n"); ansi_restore (fp); @@ -337,6 +340,8 @@ main (int argc, char *argv[]) fprintf (fp, "\"TLS\": %s,\n", tls_negotiated ? "true" : "false"); if (sr_negotiated >= 0) fprintf (fp, "\"structured\": %s,\n", sr_negotiated ? "true" : "false"); + if (eh_negotiated >= 0) + fprintf (fp, "\"extended\": %s,\n", eh_negotiated ? "true" : "false"); } if (!list_all) -- 2.41.0

Although our use of "base:allocation" doesn't require the use of the 64-bit API for flags, we might perform slightly faster for a server that does give us 64-bit extent lengths and honors larger nbd_zero lengths. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- v4: no change --- examples/copy-libev.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/examples/copy-libev.c b/examples/copy-libev.c index 32cb46b3..9346faf7 100644 --- a/examples/copy-libev.c +++ b/examples/copy-libev.c @@ -96,7 +96,7 @@ struct request { }; struct extent { - uint32_t length; + uint64_t length; bool zero; }; @@ -184,7 +184,7 @@ get_events (struct connection *c) static int extent_callback (void *user_data, const char *metacontext, uint64_t offset, - uint32_t *entries, size_t nr_entries, int *error) + nbd_extent *entries, size_t nr_entries, int *error) { struct request *r = user_data; @@ -199,22 +199,21 @@ extent_callback (void *user_data, const char *metacontext, uint64_t offset, return 1; } - /* Libnbd returns uint32_t pair (length, flags) for each extent. */ - extents_len = nr_entries / 2; + extents_len = nr_entries; extents = malloc (extents_len * sizeof *extents); if (extents == NULL) FAIL ("Cannot allocated extents: %s", strerror (errno)); /* Copy libnbd entries to extents array. */ - for (int i = 0, j = 0; i < extents_len; i++, j=i*2) { - extents[i].length = entries[j]; + for (int i = 0; i < extents_len; i++) { + extents[i].length = entries[i].length; /* Libnbd exposes both ZERO and HOLE flags. We care only about * ZERO status, meaning we can copy this extent using efficinet * zero method. */ - extents[i].zero = (entries[j + 1] & LIBNBD_STATE_ZERO) != 0; + extents[i].zero = (entries[i].flags & LIBNBD_STATE_ZERO) != 0; } DEBUG ("r%zu: received %zu extents for %s", @@ -286,10 +285,10 @@ start_extents (struct request *r) DEBUG ("r%zu: start extents offset=%" PRIi64 " count=%zu", r->index, offset, count); - cookie = nbd_aio_block_status ( + cookie = nbd_aio_block_status_64 ( src.nbd, count, offset, - (nbd_extent_callback) { .callback=extent_callback, - .user_data=r }, + (nbd_extent64_callback) { .callback=extent_callback, + .user_data=r }, (nbd_completion_callback) { .callback=extents_completed, .user_data=r }, 0); @@ -324,7 +323,7 @@ next_extent (struct request *r) limit = MIN (REQUEST_SIZE, size - offset); while (length < limit) { - DEBUG ("e%zu: offset=%" PRIi64 " len=%" PRIu32 " zero=%d", + DEBUG ("e%zu: offset=%" PRIi64 " len=%" PRIu64 " zero=%d", extents_pos, offset, extents[extents_pos].length, is_zero); /* If this extent is too large, steal some data from it to -- 2.41.0

Since our example program for 32-bit extents is inherently limited to 32-bit lengths, it is also worth demonstrating the 64-bit extent API, including the difference in the array indexing being saner. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- v4: no change --- ocaml/examples/Makefile.am | 1 + ocaml/examples/extents64.ml | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 ocaml/examples/extents64.ml diff --git a/ocaml/examples/Makefile.am b/ocaml/examples/Makefile.am index 28b4ab94..a4eb47a5 100644 --- a/ocaml/examples/Makefile.am +++ b/ocaml/examples/Makefile.am @@ -20,6 +20,7 @@ include $(top_srcdir)/subdir-rules.mk ml_examples = \ asynch_copy.ml \ extents.ml \ + extents64.ml \ get_size.ml \ open_qcow2.ml \ server_flags.ml \ diff --git a/ocaml/examples/extents64.ml b/ocaml/examples/extents64.ml new file mode 100644 index 00000000..8ee7e218 --- /dev/null +++ b/ocaml/examples/extents64.ml @@ -0,0 +1,42 @@ +open Printf + +let () = + NBD.with_handle ( + fun nbd -> + NBD.add_meta_context nbd "base:allocation"; + NBD.connect_command nbd + ["nbdkit"; "-s"; "--exit-with-parent"; "-r"; + "sparse-random"; "8G"]; + + (* Read the extents and print them. *) + let size = NBD.get_size nbd in + let cap = + match NBD.get_extended_headers_negotiated nbd with + | true -> size + | false -> 0x8000_0000_L in + let fetch_offset = ref 0_L in + while !fetch_offset < size do + let remaining = Int64.sub size !fetch_offset in + let fetch_size = min remaining cap in + NBD.block_status_64 nbd fetch_size !fetch_offset ( + fun meta _ entries err -> + printf "nbd_block_status callback: meta=%s err=%d\n" meta !err; + if meta = "base:allocation" then ( + printf "index\t%16s %16s %s\n" "offset" "length" "flags"; + for i = 0 to Array.length entries - 1 do + let len = fst entries.(i) + and flags = + match snd entries.(i) with + | 0_L -> "data" + | 1_L -> "hole" + | 2_L -> "zero" + | 3_L -> "hole+zero" + | unknown -> sprintf "unknown (%Ld)" unknown in + printf "%d:\t%16Ld %16Ld %s\n" i !fetch_offset len flags; + fetch_offset := Int64.add !fetch_offset len + done; + ); + 0 + ) (* NBD.block_status *) + done + ) -- 2.41.0

This is the culmination of the previous patches' preparation work for using extended headers when possible. The new states in the state machine are copied extensively from our handling of OPT_STRUCTURED_REPLY. The next patch will then expose a new API nbd_opt_extended_headers() for manual control. At the same time I posted this patch, I had patches for qemu-nbd to support extended headers as server (nbdkit is a bit tougher). The next patches will add some interop tests that pass when using a new enough qemu-nbd, showing that we have cross-project interoperability and therefore an extension worth standardizing. NOTE: This patch causes an observable (albeit uncommon) change in behavior to a specific corner case involving a meta-context with 64-bit flags (so far, no such meta-context exists, but I'll use "x-context:big" as a placeholder for such a meta-context). An application compiled and run with libnbd 1.16 that requests nbd_add_meta_context(h, "x-context:big") will fail to negotiate that context, but can still succeed at negotiating "base:allocation". What's more, that application was compiled at a time when the nbd_block_status_64() API did not exist, so it will necessarily be using the older 32-bit nbd_block_status() API. With the approach done in this patch (that is, the same client now linking against libnbd 1.18 defaults to unconditionally requesting extended headers), the negotiation for "x-context:big" will now succeed, but calls to nbd_block_status() will encounter an EOVERFLOW error when the server's 64-bit flags cannot be passed back to the caller's 32-bit callback. The caller will still see the "base:allocation" results, but the overall API will fail. Thus, we have taken a case that used to pass into something that now fails. Still, it is easy to make the following mitigating arguments: 1) most applications _don't_ blindly request "x-context:big" while insisting on a 32-bit API; since interpreting the context bits is context-specific, any app that knows what those bits actually mean (or which want to support arbitrary user input for meta contexts, like 'nbdinfo --map') will want to be compiled against libnbd 1.18 in the first place, to take advantage of nbd_block_status_64(). 2) If the application REALLY needs to preserve 1.16 behavior, even when compiled against 1.18 or newer, it is easy enough to add the following escape hatch: #if LIBNBD_HAVE_NBD_SET_REQUEST_EXTENDED_HEADERS nbd_set_request_extended_headers(h, 0); #endif And since such an escape hatch is only needed in the finite (possibly empty?) set of programs that need to preserve the older behavior, it does not introduce scaling problems. Because my approach is a subtle change, I also evaluated two other potential solutions, documented here, and why I did not go with them: Approach 2) No change to the default. ALL applications that want to use extended headers have to explicitly opt-in (possibly taking advantage of LIBNBD_HAVE_... witness macros to allow successful compilation to fallback APIs when compiling against older libnbd). This avoids the subtle change, but comes at an open-ended cost: all future libnbd clients that WANT to benefit from extended headers will have to add boilerplate to their connection setup, which does not scale well, just to benefit the few applications that depended on the older behavior. It becomes highly likely that someone will write a new client but does not use the new boilerplate, and thereby inadvertently suffers from worse performace because of the older defaults being set in stone. Approach 3) Resort to versioned symbols, to make the default of whether to request extended headers (in the absence of an explicit nbd_set_request_extended_headers() call) depend on which version of libnbd one compiles against. That is, teach our linker script to produce aliased symbols, where nbd_create becomes an alias to either __nbd_create@@1_16 (default off) for backwards compatibility to apps compiled against older libnbd but run against the new .so, or to __nbd_create@@1_18 (default on) with preprocessor magic in <libnbd.h> that you get this alias by default when compiling against newer libnbd. This scales well for clients (old apps continue to run with identical behavior, new apps automatically get new features unless they request preprocessor magic to specifically favor the older aliasing styles), but is dreadful to maintain (look at glibc for the hoops they have to jump through for such versioned-symbol shenanigans needed to provide transparent multi-standard support). I don't see libnbd as such a critical or high-user-base project that we need to take on this extra level of work. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- v4: expand commit message to document other alternatives I did not code up --- generator/API.ml | 87 ++++++++--------- generator/state_machine.ml | 41 ++++++++ .../states-newstyle-opt-extended-headers.c | 94 +++++++++++++++++++ generator/states-newstyle-opt-starttls.c | 6 +- generator/Makefile.am | 1 + 5 files changed, 184 insertions(+), 45 deletions(-) create mode 100644 generator/states-newstyle-opt-extended-headers.c diff --git a/generator/API.ml b/generator/API.ml index 36033817..d1849710 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -953,23 +953,24 @@ "set_request_meta_context", { (all C<nbd_connect_*> calls when L<nbd_set_opt_mode(3)> is false, or L<nbd_opt_go(3)> and L<nbd_opt_info(3)> when option mode is enabled) will also try to issue NBD_OPT_SET_META_CONTEXT when -the server supports structured replies and any contexts were -registered by L<nbd_add_meta_context(3)>. The default setting -is true; however the extra step of negotiating meta contexts is -not always desirable: performing both info and go on the same -export works without needing to re-negotiate contexts on the -second call; integration testing of other servers may benefit -from manual invocation of L<nbd_opt_set_meta_context(3)> at -other times in the negotiation sequence; and even when using just -L<nbd_opt_info(3)>, it can be faster to collect the server's +the server supports structured replies or extended headers and +any contexts were registered by L<nbd_add_meta_context(3)>. The +default setting is true; however the extra step of negotiating +meta contexts is not always desirable: performing both info and +go on the same export works without needing to re-negotiate +contexts on the second call; integration testing of other servers +may benefit from manual invocation of L<nbd_opt_set_meta_context(3)> +at other times in the negotiation sequence; and even when using +just L<nbd_opt_info(3)>, it can be faster to collect the server's results by relying on the callback function passed to L<nbd_opt_list_meta_context(3)> than a series of post-process calls to L<nbd_can_meta_context(3)>. Note that this control has no effect if the server does not -negotiate structured replies, or if the client did not request -any contexts via L<nbd_add_meta_context(3)>. Setting this -control to false may cause L<nbd_block_status(3)> to fail."; +negotiate structured replies or extended headers, or if the +client did not request any contexts via L<nbd_add_meta_context(3)>. +Setting this control to false may cause L<nbd_block_status(3)> +to fail."; see_also = [Link "set_opt_mode"; Link "opt_go"; Link "opt_info"; Link "opt_list_meta_context"; Link "opt_set_meta_context"; Link "get_structured_replies_negotiated"; @@ -1404,11 +1405,11 @@ "opt_info", { If successful, functions like L<nbd_is_read_only(3)> and L<nbd_get_size(3)> will report details about that export. If L<nbd_set_request_meta_context(3)> is set (the default) and -structured replies were negotiated, it is also valid to use -L<nbd_can_meta_context(3)> after this call. However, it may be -more efficient to clear that setting and manually utilize -L<nbd_opt_list_meta_context(3)> with its callback approach, for -learning which contexts an export supports. In general, if +structured replies or extended headers were negotiated, it is also +valid to use L<nbd_can_meta_context(3)> after this call. However, +it may be more efficient to clear that setting and manually +utilize L<nbd_opt_list_meta_context(3)> with its callback approach, +for learning which contexts an export supports. In general, if L<nbd_opt_go(3)> is called next, that call will likely succeed with the details remaining the same, although this is not guaranteed by all servers. @@ -1538,12 +1539,12 @@ "opt_set_meta_context", { recent L<nbd_set_export_name(3)> or L<nbd_connect_uri(3)>. This can only be used if L<nbd_set_opt_mode(3)> enabled option mode. Normally, this function is redundant, as L<nbd_opt_go(3)> -automatically does the same task if structured replies have -already been negotiated. But manual control over meta context -requests can be useful for fine-grained testing of how a server -handles unusual negotiation sequences. Often, use of this -function is coupled with L<nbd_set_request_meta_context(3)> to -bypass the automatic context request normally performed by +automatically does the same task if structured replies or extended +headers have already been negotiated. But manual control over +meta context requests can be useful for fine-grained testing of +how a server handles unusual negotiation sequences. Often, use +of this function is coupled with L<nbd_set_request_meta_context(3)> +to bypass the automatic context request normally performed by L<nbd_opt_go(3)>. The NBD protocol allows a client to decide how many queries to ask @@ -1597,12 +1598,13 @@ "opt_set_meta_context_queries", { or L<nbd_connect_uri(3)>. This can only be used if L<nbd_set_opt_mode(3)> enabled option mode. Normally, this function is redundant, as L<nbd_opt_go(3)> automatically does -the same task if structured replies have already been -negotiated. But manual control over meta context requests can -be useful for fine-grained testing of how a server handles -unusual negotiation sequences. Often, use of this function is -coupled with L<nbd_set_request_meta_context(3)> to bypass the -automatic context request normally performed by L<nbd_opt_go(3)>. +the same task if structured replies or extended headers have +already been negotiated. But manual control over meta context +requests can be useful for fine-grained testing of how a server +handles unusual negotiation sequences. Often, use of this +function is coupled with L<nbd_set_request_meta_context(3)> to +bypass the automatic context request normally performed by +L<nbd_opt_go(3)>. The NBD protocol allows a client to decide how many queries to ask the server. This function takes an explicit list of queries; to @@ -3281,13 +3283,13 @@ "aio_opt_set_meta_context", { recent L<nbd_set_export_name(3)> or L<nbd_connect_uri(3)>. This can only be used if L<nbd_set_opt_mode(3)> enabled option mode. Normally, this function is redundant, as L<nbd_opt_go(3)> -automatically does the same task if structured replies have -already been negotiated. But manual control over meta context -requests can be useful for fine-grained testing of how a server -handles unusual negotiation sequences. Often, use of this -function is coupled with L<nbd_set_request_meta_context(3)> to -bypass the automatic context request normally performed by -L<nbd_opt_go(3)>. +automatically does the same task if structured replies or +extended headers have already been negotiated. But manual +control over meta context requests can be useful for fine-grained +testing of how a server handles unusual negotiation sequences. +Often, use of this function is coupled with +L<nbd_set_request_meta_context(3)> to bypass the automatic +context request normally performed by L<nbd_opt_go(3)>. To determine when the request completes, wait for L<nbd_aio_is_connecting(3)> to return false. Or supply the optional @@ -3314,12 +3316,13 @@ "aio_opt_set_meta_context_queries", { or L<nbd_connect_uri(3)>. This can only be used if L<nbd_set_opt_mode(3)> enabled option mode. Normally, this function is redundant, as L<nbd_opt_go(3)> automatically does -the same task if structured replies have already been -negotiated. But manual control over meta context requests can -be useful for fine-grained testing of how a server handles -unusual negotiation sequences. Often, use of this function is -coupled with L<nbd_set_request_meta_context(3)> to bypass the -automatic context request normally performed by L<nbd_opt_go(3)>. +the same task if structured replies or extended headers have +already been negotiated. But manual control over meta context +requests can be useful for fine-grained testing of how a server +handles unusual negotiation sequences. Often, use of this +function is coupled with L<nbd_set_request_meta_context(3)> to +bypass the automatic context request normally performed by +L<nbd_opt_go(3)>. To determine when the request completes, wait for L<nbd_aio_is_connecting(3)> to return false. Or supply the optional diff --git a/generator/state_machine.ml b/generator/state_machine.ml index 7a5bc59b..2d912ef8 100644 --- a/generator/state_machine.ml +++ b/generator/state_machine.ml @@ -297,6 +297,7 @@ and * NEGOTIATING after OPT_STRUCTURED_REPLY or any failed OPT_GO. *) Group ("OPT_STARTTLS", newstyle_opt_starttls_state_machine); + Group ("OPT_EXTENDED_HEADERS", newstyle_opt_extended_headers_state_machine); Group ("OPT_STRUCTURED_REPLY", newstyle_opt_structured_reply_state_machine); Group ("OPT_META_CONTEXT", newstyle_opt_meta_context_state_machine); Group ("OPT_GO", newstyle_opt_go_state_machine); @@ -441,6 +442,46 @@ and }; ] +(* Fixed newstyle NBD_OPT_EXTENDED_HEADERS option. + * Implementation: generator/states-newstyle-opt-extended-headers.c + *) +and newstyle_opt_extended_headers_state_machine = [ + State { + default_state with + name = "START"; + comment = "Try to negotiate newstyle NBD_OPT_EXTENDED_HEADERS"; + external_events = []; + }; + + State { + default_state with + name = "SEND"; + comment = "Send newstyle NBD_OPT_EXTENDED_HEADERS negotiation request"; + external_events = [ NotifyWrite, "" ]; + }; + + State { + default_state with + name = "RECV_REPLY"; + comment = "Receive newstyle NBD_OPT_EXTENDED_HEADERS option reply"; + external_events = [ NotifyRead, "" ]; + }; + + State { + default_state with + name = "RECV_REPLY_PAYLOAD"; + comment = "Receive any newstyle NBD_OPT_EXTENDED_HEADERS reply payload"; + external_events = [ NotifyRead, "" ]; + }; + + State { + default_state with + name = "CHECK_REPLY"; + comment = "Check newstyle NBD_OPT_EXTENDED_HEADERS option reply"; + external_events = []; + }; +] + (* Fixed newstyle NBD_OPT_STRUCTURED_REPLY option. * Implementation: generator/states-newstyle-opt-structured-reply.c *) diff --git a/generator/states-newstyle-opt-extended-headers.c b/generator/states-newstyle-opt-extended-headers.c new file mode 100644 index 00000000..1ec25e97 --- /dev/null +++ b/generator/states-newstyle-opt-extended-headers.c @@ -0,0 +1,94 @@ +/* nbd client library in userspace: state machine + * 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 + */ + +/* State machine for negotiating NBD_OPT_EXTENDED_HEADERS. */ + +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; + } + + h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); + h->sbuf.option.option = htobe32 (NBD_OPT_EXTENDED_HEADERS); + h->sbuf.option.optlen = htobe32 (0); + h->chunks_sent++; + h->wbuf = &h->sbuf; + h->wlen = sizeof h->sbuf.option; + SET_NEXT_STATE (%SEND); + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.SEND: + switch (send_from_wbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 0: + h->rbuf = &h->sbuf; + h->rlen = sizeof h->sbuf.or.option_reply; + SET_NEXT_STATE (%RECV_REPLY); + } + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.RECV_REPLY: + switch (recv_into_rbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 0: + if (prepare_for_reply_payload (h, NBD_OPT_EXTENDED_HEADERS) == -1) { + SET_NEXT_STATE (%.DEAD); + return 0; + } + SET_NEXT_STATE (%RECV_REPLY_PAYLOAD); + } + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.RECV_REPLY_PAYLOAD: + switch (recv_into_rbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 0: SET_NEXT_STATE (%CHECK_REPLY); + } + return 0; + + NEWSTYLE.OPT_EXTENDED_HEADERS.CHECK_REPLY: + uint32_t reply; + + reply = be32toh (h->sbuf.or.option_reply.reply); + switch (reply) { + case NBD_REP_ACK: + debug (h, "negotiated extended headers on this connection"); + h->extended_headers = true; + /* Extended headers trump structured replies, so skip ahead. */ + h->structured_replies = true; + break; + default: + if (handle_reply_error (h) == -1) { + SET_NEXT_STATE (%.DEAD); + return 0; + } + + debug (h, "extended headers are not supported by this server"); + break; + } + + /* Next option. */ + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + return 0; + +} /* END STATE MACHINE */ diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c index e497548c..1e2997a3 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -26,7 +26,7 @@ NEWSTYLE.OPT_STARTTLS.START: else { /* If TLS was not requested we skip this option and go to the next one. */ if (h->tls == LIBNBD_TLS_DISABLE) { - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + SET_NEXT_STATE (%^OPT_EXTENDED_HEADERS.START); return 0; } assert (CALLBACK_IS_NULL (h->opt_cb.completion)); @@ -128,7 +128,7 @@ NEWSTYLE.OPT_STARTTLS.CHECK_REPLY: SET_NEXT_STATE (%.NEGOTIATING); else { debug (h, "continuing with unencrypted connection"); - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + SET_NEXT_STATE (%^OPT_EXTENDED_HEADERS.START); } return 0; } @@ -185,7 +185,7 @@ NEWSTYLE.OPT_STARTTLS.TLS_HANDSHAKE_DONE: if (h->opt_current == NBD_OPT_STARTTLS) SET_NEXT_STATE (%.NEGOTIATING); else - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + SET_NEXT_STATE (%^OPT_EXTENDED_HEADERS.START); return 0; } /* END STATE MACHINE */ diff --git a/generator/Makefile.am b/generator/Makefile.am index c3d53b26..c8477842 100644 --- a/generator/Makefile.am +++ b/generator/Makefile.am @@ -30,6 +30,7 @@ states_code = \ states-issue-command.c \ states-magic.c \ states-newstyle-opt-export-name.c \ + states-newstyle-opt-extended-headers.c \ states-newstyle-opt-list.c \ states-newstyle-opt-go.c \ states-newstyle-opt-meta-context.c \ -- 2.41.0

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> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- v4: no change --- generator/API.ml | 79 +++++++-- generator/states-newstyle.c | 3 + .../states-newstyle-opt-extended-headers.c | 30 +++- 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 d1849710..a02a8a2d 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.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/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/lib/opt.c b/lib/opt.c index 1446eef3..dd0bed96 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) @@ -388,6 +413,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 e5304f16..981b3bda 100644 --- a/.gitignore +++ b/.gitignore @@ -119,6 +119,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 -- 2.41.0

In the recent NBD protocol extensions to add 64-bit commands [1], an additional option was added to allow NBD_CMD_BLOCK_STATUS to pass a client payload instructing the server to filter its answers in nbd.git commit e6f3b94a (mainly useful when the client requests more than one meta context with NBD_OPT_SET_META_CONTEXT). This patch lays the groundwork by exposing servers that advertise this capability, although libnbd does not yet actually utilize it until the next patch. At the time this patch was written, qemu-nbd was also patched to provide such support; hence, an interop/ test shows the API in action. [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/ Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> --- v4: rebase to earlier changes, s/can/has/ [Rich] --- info/nbdinfo.pod | 8 ++ lib/nbd-protocol.h | 7 ++ generator/API.ml | 18 +++++ lib/flags.c | 12 +++ examples/server-flags.c | 7 +- interop/Makefile.am | 6 ++ interop/block-status-payload.c | 126 ++++++++++++++++++++++++++++++++ interop/block-status-payload.sh | 68 +++++++++++++++++ .gitignore | 1 + info/can.c | 5 ++ info/show.c | 9 ++- 11 files changed, 265 insertions(+), 2 deletions(-) create mode 100644 interop/block-status-payload.c create mode 100755 interop/block-status-payload.sh diff --git a/info/nbdinfo.pod b/info/nbdinfo.pod index 2bdd573f..9b1962da 100644 --- a/info/nbdinfo.pod +++ b/info/nbdinfo.pod @@ -171,6 +171,11 @@ for supporting block status commands). Test if server supports extended headers (a prerequisite for supporting 64-bit commands; implies structured replies as well). +=item nbdinfo --has block-status-payload URI + +Test if server has support for passing a client payload to limit the +response to a block status command. + =item nbdinfo --is rotational URI Test if the server export is backed by something which behaves like a @@ -373,6 +378,8 @@ When using I<--list>, the default is I<--no-content> (since downloading from each export is expensive). To enable content probing use I<--list --content>. +=item B<--has block-status-payload> + =item B<--has extended-headers> =item B<--has structured-reply> @@ -385,6 +392,7 @@ indicate an error querying the flag). For further information see the L<NBD protocol|https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md> and the following libnbd functions: +L<nbd_can_block_status_payload(3)>, L<nbd_get_extended_headers_negotiated(3)>, L<nbd_get_structured_replies_negotiated(3)>. diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h index b5a28ae4..e5288a17 100644 --- a/lib/nbd-protocol.h +++ b/lib/nbd-protocol.h @@ -113,6 +113,7 @@ struct nbd_fixed_new_option_reply { #define NBD_FLAG_CAN_MULTI_CONN (1U << 8) #define NBD_FLAG_SEND_CACHE (1U << 10) #define NBD_FLAG_SEND_FAST_ZERO (1U << 11) +#define NBD_FLAG_BLOCK_STATUS_PAYLOAD (1U << 12) /* NBD options (new style handshake only). */ #define NBD_OPT_EXPORT_NAME 1 @@ -204,6 +205,12 @@ struct nbd_request_ext { uint64_t count; /* Request effect or payload length. */ } NBD_ATTRIBUTE_PACKED; +/* Extended request payload for NBD_CMD_BLOCK_STATUS, when supported. */ +struct nbd_block_status_payload { + uint64_t length; /* Effective length of client request */ + /* followed by array of uint32_t ids */ +} NBD_ATTRIBUTE_PACKED; + /* Simple reply (server -> client). */ struct nbd_simple_reply { uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC. */ diff --git a/generator/API.ml b/generator/API.ml index a02a8a2d..6228a4e5 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -2327,6 +2327,23 @@ "can_fast_zero", { example = Some "examples/server-flags.c"; }; + "can_block_status_payload", { + default_call with + args = []; ret = RBool; + permitted_states = [ Negotiating; Connected; Closed ]; + shortdesc = "does the server support the block status payload flag?"; + longdesc = "\ +Returns true if the server supports the use of the +C<LIBNBD_CMD_FLAG_PAYLOAD_LEN> flag to allow filtering of the +block status command. Returns +false if the server does not. Note that this will never return +true if L<nbd_get_extended_headers_negotiated(3)> is false." +^ non_blocking_test_call_description; + see_also = [SectionLink "Flag calls"; Link "opt_info"; + Link "get_extended_headers_negotiated"]; + example = Some "examples/server-flags.c"; + }; + "can_df", { default_call with args = []; ret = RBool; @@ -4183,6 +4200,7 @@ let first_version = "get_extended_headers_negotiated", (1, 18); "opt_extended_headers", (1, 18); "aio_opt_extended_headers", (1, 18); + "can_block_status_payload", (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/lib/flags.c b/lib/flags.c index be880acf..7e6ddedd 100644 --- a/lib/flags.c +++ b/lib/flags.c @@ -66,6 +66,12 @@ nbd_internal_set_size_and_flags (struct nbd_handle *h, eflags &= ~NBD_FLAG_SEND_DF; } + if (eflags & NBD_FLAG_BLOCK_STATUS_PAYLOAD && !h->extended_headers) { + debug (h, "server lacks extended headers, ignoring claim " + "of block status payload"); + eflags &= ~NBD_FLAG_BLOCK_STATUS_PAYLOAD; + } + if (eflags & NBD_FLAG_SEND_FAST_ZERO && !(eflags & NBD_FLAG_SEND_WRITE_ZEROES)) { debug (h, "server lacks write zeroes, ignoring claim of fast zero"); @@ -213,6 +219,12 @@ nbd_unlocked_can_cache (struct nbd_handle *h) return get_flag (h, NBD_FLAG_SEND_CACHE); } +int +nbd_unlocked_can_block_status_payload (struct nbd_handle *h) +{ + return get_flag (h, NBD_FLAG_BLOCK_STATUS_PAYLOAD); +} + int nbd_unlocked_can_meta_context (struct nbd_handle *h, const char *name) { diff --git a/examples/server-flags.c b/examples/server-flags.c index d156aced..f53b86ed 100644 --- a/examples/server-flags.c +++ b/examples/server-flags.c @@ -78,8 +78,13 @@ main (int argc, char *argv[]) PRINT_FLAG (nbd_can_multi_conn); PRINT_FLAG (nbd_can_trim); PRINT_FLAG (nbd_can_zero); -#if LIBNBD_HAVE_NBD_CAN_FAST_ZERO /* Added in 1.2 */ +#if LIBNBD_HAVE_NBD_CAN_FAST_ZERO + /* Added in 1.2 */ PRINT_FLAG (nbd_can_fast_zero); +#endif +#if LIBNBD_HAVE_NBD_CAN_BLOCK_STATUS_PAYLOAD + /* Added in 1.18 */ + PRINT_FLAG (nbd_can_block_status_payload); #endif PRINT_FLAG (nbd_is_read_only); PRINT_FLAG (nbd_is_rotational); diff --git a/interop/Makefile.am b/interop/Makefile.am index 3947ce3d..d6485adf 100644 --- a/interop/Makefile.am +++ b/interop/Makefile.am @@ -27,6 +27,7 @@ EXTRA_DIST = \ list-exports-test-dir/disk2 \ structured-read.sh \ opt-extended-headers.sh \ + block-status-payload.sh \ $(NULL) TESTS_ENVIRONMENT = \ @@ -138,6 +139,7 @@ check_PROGRAMS += \ block-status-64 \ structured-read \ opt-extended-headers \ + block-status-payload \ $(NULL) TESTS += \ interop-qemu-nbd \ @@ -150,6 +152,7 @@ TESTS += \ structured-read.sh \ interop-qemu-block-size.sh \ opt-extended-headers.sh \ + block-status-payload.sh \ $(NULL) interop_qemu_nbd_SOURCES = \ @@ -247,6 +250,9 @@ 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 +block_status_payload_SOURCES = block-status-payload.c +block_status_payload_LDADD = $(top_builddir)/lib/libnbd.la + endif HAVE_QEMU_NBD #---------------------------------------------------------------------- diff --git a/interop/block-status-payload.c b/interop/block-status-payload.c new file mode 100644 index 00000000..9603dfe5 --- /dev/null +++ b/interop/block-status-payload.c @@ -0,0 +1,126 @@ +/* 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 interaction with qemu using block status payload filtering. */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <assert.h> +#include <stdbool.h> +#include <errno.h> + +#include <libnbd.h> + +#include "array-size.h" + +static const char *contexts[] = { + "base:allocation", + "qemu:allocation-depth", + "qemu:dirty-bitmap:bitmap0", + "qemu:dirty-bitmap:bitmap1", +}; + +static int +cb (void *opaque, const char *metacontext, uint64_t offset, + nbd_extent *entries, size_t len, int *error) +{ + /* Adjust seen according to which context was visited */ + unsigned int *seen = opaque; + size_t i; + + for (i = 0; i < ARRAY_SIZE (contexts); i++) + if (strcmp (contexts[i], metacontext) == 0) + break; + *seen |= 1 << i; + return 0; +} + +int +main (int argc, char *argv[]) +{ + struct nbd_handle *nbd; + int64_t exportsize; + unsigned int seen; + size_t i; + int r; + + if (argc < 2) { + fprintf (stderr, "%s qemu-nbd [args ...]\n", argv[0]); + exit (EXIT_FAILURE); + } + + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + assert (ARRAY_SIZE (contexts) == 4); + for (i = 0; i < ARRAY_SIZE (contexts); i++) { + if (nbd_add_meta_context (nbd, contexts[i]) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + } + + if (nbd_connect_systemd_socket_activation (nbd, &argv[1]) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + r = nbd_can_block_status_payload (nbd); + if (r == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (r != 1) { + fprintf (stderr, "expecting block status payload support from qemu\n"); + exit (EXIT_FAILURE); + } + + exportsize = nbd_get_size (nbd); + if (exportsize == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* An unfiltered call should see all four contexts */ + seen = 0; + if (nbd_block_status_64 (nbd, exportsize, 0, + (nbd_extent64_callback) { .callback = cb, + .user_data = &seen }, + 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + assert (seen == 0xf); + + /* FIXME: Test filtered calls once the API is added */ + if (nbd_shutdown (nbd, 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + nbd_close (nbd); + + exit (EXIT_SUCCESS); +} diff --git a/interop/block-status-payload.sh b/interop/block-status-payload.sh new file mode 100755 index 00000000..a12cfc8a --- /dev/null +++ b/interop/block-status-payload.sh @@ -0,0 +1,68 @@ +#!/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 use of block status payload for server filtering + +source ../tests/functions.sh +set -e +set -x + +requires qemu-img bitmap --help +# This test uses the qemu-nbd -A and -B options. +requires qemu-nbd -A -BA --version + +file="block-status-payload.qcow2" +rm -f $file +cleanup_fn rm -f $file + +# Create sparse file with two bitmaps. +qemu-img create -f qcow2 $file 1M +qemu-img bitmap --add --enable -f qcow2 $file bitmap0 +qemu-img bitmap --add --enable -f qcow2 $file bitmap1 + +# Unconditional part of test: qemu should not advertise block status payload +# support if extended headers are not in use +nbdsh -c ' +h.set_request_extended_headers(False) +h.add_meta_context("base:allocation") +h.add_meta_context("qemu:allocation-depth") +h.add_meta_context("qemu:dirty-bitmap:bitmap0") +h.add_meta_context("qemu:dirty-bitmap:bitmap1") +h.set_opt_mode(True) +args = ["qemu-nbd", "-f", "qcow2", "-A", "-B", "bitmap0", "-B", "bitmap1", + "'"$file"'"] +h.connect_systemd_socket_activation(args) +assert h.aio_is_negotiating() is True +assert h.get_extended_headers_negotiated() is False +# Flag not available until info or go +try: + h.can_block_status_payload() + assert False +except nbd.Error: + pass +h.opt_info() +assert h.can_block_status_payload() is False +assert h.can_meta_context("base:allocation") is True +h.opt_abort() +' + +# Conditional part of test: if qemu is new enough to support extended +# headers, we assume it can also support block status payload. +requires nbdinfo --can extended-headers -- [ qemu-nbd -r -f qcow2 "$file" ] +$VG ./block-status-payload \ + qemu-nbd -f qcow2 -A -B bitmap0 -B bitmap1 $file diff --git a/.gitignore b/.gitignore index c0761b25..cc45d61b 100644 --- a/.gitignore +++ b/.gitignore @@ -103,6 +103,7 @@ Makefile.in /info/nbdinfo.1 /install-sh /interop/block-status-64 +/interop/block-status-payload /interop/dirty-bitmap /interop/interop-nbd-server /interop/interop-nbd-server-tls diff --git a/info/can.c b/info/can.c index 8547ef04..6bb36fb1 100644 --- a/info/can.c +++ b/info/can.c @@ -72,6 +72,11 @@ do_can (void) else if (strcasecmp (can, "rotational") == 0) feature = nbd_is_rotational (nbd); + else if (strcasecmp (can, "block status payload") == 0 || + strcasecmp (can, "block-status-payload") == 0 || + strcasecmp (can, "block_status_payload") == 0) + feature = nbd_can_block_status_payload (nbd); + else if (strcasecmp (can, "cache") == 0) feature = nbd_can_cache (nbd); diff --git a/info/show.c b/info/show.c index 920bbb0a..8914f927 100644 --- a/info/show.c +++ b/info/show.c @@ -54,7 +54,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc, char *uri = NULL; int is_rotational, is_read_only; int can_cache, can_df, can_fast_zero, can_flush, can_fua, - can_multi_conn, can_trim, can_zero; + can_multi_conn, can_trim, can_zero, can_block_status_payload; int64_t block_minimum, block_preferred, block_maximum; string_vector contexts = empty_vector; bool show_context = false; @@ -120,6 +120,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc, can_multi_conn = nbd_can_multi_conn (nbd); can_trim = nbd_can_trim (nbd); can_zero = nbd_can_zero (nbd); + can_block_status_payload = nbd_can_block_status_payload (nbd); block_minimum = nbd_get_block_size (nbd, LIBNBD_SIZE_MINIMUM); block_preferred = nbd_get_block_size (nbd, LIBNBD_SIZE_PREFERRED); block_maximum = nbd_get_block_size (nbd, LIBNBD_SIZE_MAXIMUM); @@ -161,6 +162,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc, if (is_read_only >= 0) fprintf (fp, "\t%s: %s\n", "is_read_only", is_read_only ? "true" : "false"); + if (can_block_status_payload >= 0) + show_boolean ("can_block_status_payload", can_block_status_payload); if (can_cache >= 0) show_boolean ("can_cache", can_cache); if (can_df >= 0) @@ -230,6 +233,10 @@ show_one_export (struct nbd_handle *nbd, const char *desc, if (is_read_only >= 0) fprintf (fp, "\t\"%s\": %s,\n", "is_read_only", is_read_only ? "true" : "false"); + if (can_block_status_payload >= 0) + fprintf (fp, "\t\"%s\": %s,\n", + "can_block_status_payload", + can_block_status_payload ? "true" : "false"); if (can_cache >= 0) fprintf (fp, "\t\"%s\": %s,\n", "can_cache", can_cache ? "true" : "false"); -- 2.41.0

As part of extending NBD to support 64-bit lengths, the protocol also added an option for servers to allow clients to request filtered responses to NBD_CMD_BLOCK_STATUS when more than one meta-context is negotiated (see NBD commit e6f3b94a). At the same time as this patch, qemu-nbd was taught to support and advertise this feature as a server, but does not utilize it as a client (qemu doesn't yet need to connect to multiple contexts at once). Thus, addding generic client support and enhancing the interop/ test in libnbd is needed to prove that the feature is viable and worth standardizing. Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Richard W.M. Jones <rjones@redhat.com> --- v4: rebase to earlier changes --- lib/internal.h | 5 +- generator/API.ml | 71 ++++++++++++++++-- generator/states-issue-command.c | 4 +- lib/aio.c | 4 + lib/rw.c | 124 ++++++++++++++++++++++++++++++- interop/block-status-payload.c | 117 ++++++++++++++++++++++++++++- interop/block-status-payload.sh | 14 +++- info/info-can.sh | 3 + 8 files changed, 332 insertions(+), 10 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index efbb5abb..64579bc0 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -73,6 +73,8 @@ struct meta_context { }; DEFINE_VECTOR_TYPE (meta_vector, struct meta_context); +DEFINE_VECTOR_TYPE(uint32_vector, uint32_t); + struct export { char *name; char *description; @@ -399,7 +401,8 @@ struct command { uint64_t cookie; uint64_t offset; uint64_t count; - void *data; /* Buffer for read/write */ + void *data; /* Buffer for read/write, uint32_vector* for status payload */ + uint32_vector *ids; /* For block status with payload */ struct command_cb cb; bool initialized; /* For read, true if getting a hole may skip memset */ uint32_t data_seen; /* For read, cumulative size of data chunks seen */ diff --git a/generator/API.ml b/generator/API.ml index 6228a4e5..2e8a33bd 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -2335,12 +2335,13 @@ "can_block_status_payload", { longdesc = "\ Returns true if the server supports the use of the C<LIBNBD_CMD_FLAG_PAYLOAD_LEN> flag to allow filtering of the -block status command. Returns +block status command (see L<nbd_block_status_filter(3)>). Returns false if the server does not. Note that this will never return true if L<nbd_get_extended_headers_negotiated(3)> is false." ^ non_blocking_test_call_description; see_also = [SectionLink "Flag calls"; Link "opt_info"; - Link "get_extended_headers_negotiated"]; + Link "get_extended_headers_negotiated"; + Link "block_status_filter"]; example = Some "examples/server-flags.c"; }; @@ -2409,6 +2410,10 @@ "can_meta_context", { meta contexts were requested but there is a missing or failed attempt at NBD_OPT_SET_META_CONTEXT during option negotiation. +If the server supports block status filtering (see +L<nbd_can_block_status_payload(3)>, this function must return +true for any filter name passed to L<nbd_block_status_filter(3)>. + The single parameter is the name of the metadata context, for example C<LIBNBD_CONTEXT_BASE_ALLOCATION>. B<E<lt>libnbd.hE<gt>> includes defined constants for well-known @@ -2941,9 +2946,12 @@ "block_status_64", { information about blocks beginning from the specified offset to be returned. The C<count> parameter is a hint: the server may choose to return less status, or the final block -may extend beyond the requested range. If multiple contexts +may extend beyond the requested range. When multiple contexts are supported, the number of blocks and cumulative length -of those blocks need not be identical between contexts. +of those blocks need not be identical between contexts; this +command generally returns the status of all negotiated contexts, +while some servers also support a filtered request (see +L<nbd_can_block_status_payload(3)>, L<nbd_block_status_filter(3)>). Note that not all servers can support a C<count> of 4GiB or larger; L<nbd_get_extended_headers_negotiated(3)> indicates which servers @@ -2993,11 +3001,38 @@ "block_status_64", { does not exceed C<count> bytes; however, libnbd does not validate that the server obeyed the flag." ^ strict_call_description; - see_also = [Link "block_status"; + see_also = [Link "block_status"; Link "block_status_filter"; Link "add_meta_context"; Link "can_meta_context"; Link "aio_block_status_64"; Link "set_strict_mode"]; }; + "block_status_filter", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; StringList "contexts"; + Closure extent64_closure ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["REQ_ONE"; "PAYLOAD_LEN"]) ]; + ret = RErr; + permitted_states = [ Connected ]; + shortdesc = "send filtered block status command, with 64-bit callback"; + longdesc = "\ +Issue a filtered block status command to the NBD server. If +supported by the server (see L<nbd_can_block_status_payload(3)>), +this causes metadata context information about blocks beginning +from the specified offset to be returned, and with the result +limited to just the contexts specified in C<filter>. Note that +all strings in C<filter> must be supported by +L<nbd_can_meta_context(3)>. + +All other parameters to this function have the same semantics +as in L<nbd_block_status_64(3)>; except that for convenience, +the C<flags> parameter may additionally contain or omit +C<LIBNBD_CMD_FLAG_PAYLOAD_LEN>." +^ strict_call_description; + see_also = [Link "block_status_64"; + Link "can_block_status_payload"; Link "can_meta_context"; + Link "aio_block_status_filter"; Link "set_strict_mode"]; + }; + "poll", { default_call with args = [ Int "timeout" ]; ret = RInt; @@ -3667,6 +3702,30 @@ "aio_block_status_64", { Link "set_strict_mode"]; }; + "aio_block_status_filter", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; StringList "contexts"; + Closure extent64_closure ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["REQ_ONE"; "PAYLOAD_LEN"]) ]; + ret = RCookie; + permitted_states = [ Connected ]; + shortdesc = "send filtered block status command to the NBD server"; + longdesc = "\ +Send a filtered block status command to the NBD server. + +To check if the command completed, call L<nbd_aio_command_completed(3)>. +Or supply the optional C<completion_callback> which will be invoked +as described in L<libnbd(3)/Completion callbacks>. + +Other parameters behave as documented in L<nbd_block_status_filter(3)>." +^ strict_call_description; + see_also = [SectionLink "Issuing asynchronous commands"; + Link "aio_block_status_64"; Link "block_status_filter"; + Link "can_meta_context"; Link "can_block_status_payload"; + Link "set_strict_mode"]; + }; + "aio_get_fd", { default_call with args = []; ret = RFd; @@ -4201,6 +4260,8 @@ let first_version = "opt_extended_headers", (1, 18); "aio_opt_extended_headers", (1, 18); "can_block_status_payload", (1, 18); + "block_status_filter", (1, 18); + "aio_block_status_filter", (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-issue-command.c b/generator/states-issue-command.c index 59b05835..d825fbb7 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -84,7 +84,9 @@ ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD: assert (h->cmds_to_issue != NULL); cmd = h->cmds_to_issue; assert (cmd->cookie == be64toh (h->req.compact.cookie)); - if (cmd->type == NBD_CMD_WRITE) { + if (cmd->type == NBD_CMD_WRITE || + (h->extended_headers && cmd->type == NBD_CMD_BLOCK_STATUS && + cmd->flags & NBD_CMD_FLAG_PAYLOAD_LEN)) { h->wbuf = cmd->data; h->wlen = cmd->count; if (cmd->next && cmd->count < 64 * 1024) diff --git a/lib/aio.c b/lib/aio.c index cf9421ec..30505a36 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -37,6 +37,10 @@ nbd_internal_retire_and_free_command (struct command *cmd) FREE_CALLBACK (cmd->cb.fn.extent64); else FREE_CALLBACK (cmd->cb.fn.extent32); + if (cmd->ids) { + uint32_vector_reset (cmd->ids); + free (cmd->ids); + } } if (cmd->type == NBD_CMD_READ) FREE_CALLBACK (cmd->cb.fn.chunk); diff --git a/lib/rw.c b/lib/rw.c index 8e8c024c..87bcb1b6 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -187,6 +187,26 @@ nbd_unlocked_block_status_64 (struct nbd_handle *h, return wait_for_command (h, cookie); } +/* Issue a filtered block status command and wait for the reply. */ +int +nbd_unlocked_block_status_filter (struct nbd_handle *h, + uint64_t count, uint64_t offset, + char **filter, + nbd_extent64_callback *extent64, + uint32_t flags) +{ + int64_t cookie; + nbd_completion_callback c = NBD_NULL_COMPLETION; + + cookie = nbd_unlocked_aio_block_status_filter (h, count, offset, filter, + extent64, &c, flags); + if (cookie == -1) + return -1; + + assert (CALLBACK_IS_NULL (*extent64)); + return wait_for_command (h, cookie); +} + /* count_err represents the errno to return if bounds check fail */ int64_t nbd_internal_command_common (struct nbd_handle *h, @@ -195,6 +215,7 @@ nbd_internal_command_common (struct nbd_handle *h, void *data, struct command_cb *cb) { struct command *cmd; + uint32_vector *ids = NULL; if (h->disconnect_request) { set_error (EINVAL, "cannot request more commands after NBD_CMD_DISC"); @@ -242,10 +263,23 @@ nbd_internal_command_common (struct nbd_handle *h, } break; + case NBD_CMD_BLOCK_STATUS: + if (data) { + ids = data; + count = ids->len * sizeof (uint32_t); + data = ids->ptr; + if (count > MAX_REQUEST_SIZE || + (h->strict & LIBNBD_STRICT_PAYLOAD && count > h->payload_maximum)) { + set_error (ERANGE, "filter set too large"); + goto err; + } + break; + } + /* fallthrough */ + default: /* Other commands are limited by the 32 bit field in the command * structure on the wire, unless extended headers were negotiated. */ - default: if (!h->extended_headers && count > UINT32_MAX) { set_error (ERANGE, "request too large: maximum request size is %" PRIu32, UINT32_MAX); @@ -265,6 +299,7 @@ nbd_internal_command_common (struct nbd_handle *h, cmd->offset = offset; cmd->count = count; cmd->data = data; + cmd->ids = ids; if (cb) cmd->cb = *cb; @@ -314,6 +349,10 @@ nbd_internal_command_common (struct nbd_handle *h, FREE_CALLBACK (cb->fn.extent32); else FREE_CALLBACK (cb->fn.extent64); + if (ids) { + uint32_vector_reset (ids); + free (ids); + } } if (type == NBD_CMD_READ) FREE_CALLBACK (cb->fn.chunk); @@ -561,3 +600,86 @@ nbd_unlocked_aio_block_status_64 (struct nbd_handle *h, return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS, offset, count, EINVAL, NULL, &cb); } + +int64_t +nbd_unlocked_aio_block_status_filter (struct nbd_handle *h, + uint64_t count, uint64_t offset, + char **filter, + nbd_extent64_callback *extent64, + nbd_completion_callback *completion, + uint32_t flags) +{ + struct command_cb cb = { .fn.extent64 = *extent64, .wide = true, + .completion = *completion }; + uint32_vector *ids; + char *name; + size_t i; + + /* Because this affects wire format, it is more convenient to manage + * PAYLOAD_LEN by what was negotiated than to require the user to + * have to set it correctly. + */ + if (!h->extended_headers) { + set_error (ENOTSUP, "server does not support extended headers"); + return -1; + } + flags |= LIBNBD_CMD_FLAG_PAYLOAD_LEN; + + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_can_block_status_payload (h) != 1) { + set_error (EINVAL, + "server does not support the block status payload flag"); + return -1; + } + + if (!h->meta_valid || h->meta_contexts.len == 0) { + set_error (ENOTSUP, "did not negotiate any metadata contexts, " + "either you did not call nbd_add_meta_context before " + "connecting or the server does not support it"); + return -1; + } + } + + ids = calloc (1, sizeof *ids); + if (ids == NULL) { + set_error (errno, "calloc"); + return -1; + } + if (uint32_vector_append (ids, htobe32 (count >> 32)) == -1 || + uint32_vector_append (ids, htobe32 (count)) == -1) { + set_error (errno, "realloc"); + goto fail; + } + + /* O(n^2) search - hopefully filter and negotiated contexts are both small */ + for ( ; (name = *filter) != NULL; filter++) { + if (!h->meta_valid) { + set_error (EINVAL, "context %s not negotiated", name); + goto fail; + } + for (i = 0; i < h->meta_contexts.len; i++) { + struct meta_context *meta = &h->meta_contexts.ptr[i]; + if (strcmp (name, meta->name) == 0) { + if (uint32_vector_append (ids, htobe32 (meta->context_id)) == -1) { + set_error (errno, "realloc"); + goto fail; + } + break; + } + } + if (i == h->meta_contexts.len) { + set_error (EINVAL, "context %s not negotiated", name); + goto fail; + } + } + + SET_CALLBACK_TO_NULL (*extent64); + SET_CALLBACK_TO_NULL (*completion); + return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS, offset, + count, EINVAL, ids, &cb); + + fail: + uint32_vector_reset (ids); + free (ids); + return -1; +} diff --git a/interop/block-status-payload.c b/interop/block-status-payload.c index 9603dfe5..704b25aa 100644 --- a/interop/block-status-payload.c +++ b/interop/block-status-payload.c @@ -54,11 +54,26 @@ cb (void *opaque, const char *metacontext, uint64_t offset, return 0; } +static char ** +list (unsigned int use) +{ + static const char *array[ARRAY_SIZE (contexts) + 1]; + size_t i, j; + + assert (use < 1 << ARRAY_SIZE (contexts)); + for (i = j = 0; i < ARRAY_SIZE (contexts); i++) + if (use & (1 << i)) + array[j++] = contexts[i]; + array[j] = NULL; + return (char **) array; +} + int main (int argc, char *argv[]) { struct nbd_handle *nbd; int64_t exportsize; + uint64_t bytes_sent; unsigned int seen; size_t i; int r; @@ -114,7 +129,107 @@ main (int argc, char *argv[]) } assert (seen == 0xf); - /* FIXME: Test filtered calls once the API is added */ + /* Filtering with all contexts listed, same effect as unfilitered call */ + seen = 0; + if (nbd_block_status_filter (nbd, exportsize, 0, list (0xf), + (nbd_extent64_callback) { .callback = cb, + .user_data = &seen }, + 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + assert (seen == 0xf); + + /* Filtering with just two out of four contexts; test optional flag */ + seen = 0; + if (nbd_block_status_filter (nbd, exportsize, 0, list (0x5), + (nbd_extent64_callback) { .callback = cb, + .user_data = &seen }, + LIBNBD_CMD_FLAG_PAYLOAD_LEN) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + assert (seen == 0x5); + + /* Filtering with one context, near end of file (to make sure the + * payload length isn't confused with the effect length) + */ + seen = 0; + if (nbd_block_status_filter (nbd, 1, exportsize - 1, list (0x2), + (nbd_extent64_callback) { .callback = cb, + .user_data = &seen }, + 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + assert (seen == 0x2); + + /* Filtering with no contexts - pointless, so qemu rejects it */ + bytes_sent = nbd_stats_bytes_sent (nbd); + seen = 0; + if (nbd_block_status_filter (nbd, exportsize, 0, list (0x0), + (nbd_extent64_callback) { .callback = cb, + .user_data = &seen }, + 0) != -1) { + fprintf (stderr, "expecting block status failure\n"); + exit (EXIT_FAILURE); + } + assert (seen == 0x0); + if (nbd_get_errno () != EINVAL) { + fprintf (stderr, "expecting EINVAL after block status failure\n"); + exit (EXIT_FAILURE); + } + if (nbd_stats_bytes_sent (nbd) <= bytes_sent) { + fprintf (stderr, "expecting server-side rejection of bad request\n"); + exit (EXIT_FAILURE); + } + + /* Giving unknown string triggers EINVAL from libnbd */ + bytes_sent = nbd_stats_bytes_sent (nbd); + seen = 0; + { + const char *bogus[] = { "qemu:dirty-bitmap:bitmap2", NULL }; + if (nbd_block_status_filter (nbd, exportsize, 0, (char **) bogus, + (nbd_extent64_callback) { .callback = cb, + .user_data = &seen }, + 0) != -1) { + fprintf (stderr, "expecting block status failure\n"); + exit (EXIT_FAILURE); + } + } + if (nbd_get_errno () != EINVAL) { + fprintf (stderr, "expecting EINVAL after block status failure\n"); + exit (EXIT_FAILURE); + } + assert (seen == 0x0); + if (nbd_stats_bytes_sent (nbd) != bytes_sent) { + fprintf (stderr, "expecting client-side rejection of bad request\n"); + exit (EXIT_FAILURE); + } + + /* Giving same string twice triggers EINVAL from qemu */ + seen = 0; + { + const char *dupes[] = { "base:allocation", "base:allocation", NULL }; + if (nbd_block_status_filter (nbd, exportsize, 0, (char **) dupes, + (nbd_extent64_callback) { .callback = cb, + .user_data = &seen }, + 0) != -1) { + fprintf (stderr, "expecting block status failure\n"); + exit (EXIT_FAILURE); + } + } + if (nbd_get_errno () != EINVAL) { + fprintf (stderr, "expecting EINVAL after block status failure\n"); + exit (EXIT_FAILURE); + } + assert (seen == 0x0); + if (nbd_stats_bytes_sent (nbd) <= bytes_sent) { + fprintf (stderr, "expecting server-side rejection of bad request\n"); + exit (EXIT_FAILURE); + } + + /* Done */ if (nbd_shutdown (nbd, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); diff --git a/interop/block-status-payload.sh b/interop/block-status-payload.sh index a12cfc8a..0e6681b6 100755 --- a/interop/block-status-payload.sh +++ b/interop/block-status-payload.sh @@ -49,6 +49,7 @@ args = ["qemu-nbd", "-f", "qcow2", "-A", "-B", "bitmap0", "-B", "bitmap1", h.connect_systemd_socket_activation(args) assert h.aio_is_negotiating() is True assert h.get_extended_headers_negotiated() is False + # Flag not available until info or go try: h.can_block_status_payload() @@ -58,7 +59,18 @@ except nbd.Error: h.opt_info() assert h.can_block_status_payload() is False assert h.can_meta_context("base:allocation") is True -h.opt_abort() + +# Filter request not allowed if not advertised +def f(): + assert False +h.opt_go() +assert h.can_block_status_payload() is False +try: + h.block_status_filter(0, 512, ["base:allocation"], f) + assert False +except nbd.Error: + pass +h.shutdown() ' # Conditional part of test: if qemu is new enough to support extended diff --git a/info/info-can.sh b/info/info-can.sh index 5d9f2e89..391d2c12 100755 --- a/info/info-can.sh +++ b/info/info-can.sh @@ -38,6 +38,9 @@ requires bash -c "nbdkit sh --dump-plugin | grep has_can_cache=1" # and oldstyle never, but that feels like depending a bit too much on # the implementation. +# --has block-status-payload is not supported by nbdkit yet. Testing +# is done during interop with new-enough qemu. + # --has structured-reply is not a per-export setting, but rather # something set on the server as a whole. -- 2.41.0