Eric Blake
2022-Feb-17 23:04 UTC
[Libguestfs] [libnbd PATCH] RFC: api: Add set_request_block_size
WIP: I need to finish writing the actual interop-qemu-block-size.sh to
demonstrate scenarios where qemu-nbd advertises a block size > 1 to
clients that request it, but sticks to 1 otherwise (see
https://gitlab.com/qemu-project/qemu/-/blob/master/nbd/server.c#L660).
Our testsuite coverage of nbd_get_block_size() is pretty sparse (a
single line in tests/errors.c, which was skipping until patches to
nbdkit finally made it possible to utilize). But in the process of
adding an interop test with qemu-nbd, I also noticed that qemu-nbd (at
least version 6.2) exposes different block sizes to older clients that
don't request block size than it does to newer clients which promise
to obey block sizes. We still want to request by default, but now we
need a knob, similar to the existing set_full_info(), for overriding
that default for testing purposes.
---
lib/internal.h | 5 +--
generator/API.ml | 57 +++++++++++++++++++++++++++---
generator/states-newstyle-opt-go.c | 20 +++++++----
lib/handle.c | 13 +++++++
interop/Makefile.am | 4 ++-
5 files changed, 85 insertions(+), 14 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 525499a9..3868a7f1 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -120,8 +120,9 @@ struct nbd_handle {
uint8_t opt_current; /* 0 or one of NBD_OPT_* */
struct command_cb opt_cb;
- /* Full info mode. */
- bool full_info;
+ /* Tweak what OPT_INFO/GO requests. */
+ bool request_block_size; /* default true, for INFO_BLOCK_SIZE */
+ bool full_info; /* default false, for INFO_NAME, INFO_DESCRIPTION */
/* Sanitization for pread. */
bool pread_initialize;
diff --git a/generator/API.ml b/generator/API.ml
index e63a10ee..414c6b61 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -395,6 +395,45 @@ "get_export_name", {
Link "get_canonical_export_name"];
};
+ "set_request_block_size", {
+ default_call with
+ args = [Bool "request"]; ret = RErr;
+ permitted_states = [ Created; Negotiating ];
+ shortdesc = "control whether NBD_OPT_GO requests block size";
+ longdesc = "\
+By default, when connecting to an export, libnbd requests that the
+server report any block size restrictions. The NBD protocol states
+that a server may supply block sizes regardless of whether the client
+requests them, and libnbd will report those block sizes (see
+L<nbd_get_block_size(3)>). However, it also states that unless a
+client requested block sizes, the server must also be prepared for the
+client to send requests that do not match the server's constraints;
+thus, requesting block sizes puts a burden on the client to be more
+careful in the requests it makes (libnbd enforces block sizes by
+default, although L<nbd_set_strict_mode(3)> can relax things to
+bypass the NBD protocol requirements). Therefore, there are some
+NBD servers which will report a different block size to clients
+that did not request any than to the clients that promise to abide
+by block sizes. This function makes it possible to disable the
+client block size request, to observe server behavior when there
+is no promise of abiding by block size constraints.
+
+Note that even when block size is requested, the server is not
+obligated to provide any.";
+ see_also = [Link "get_request_block_size"; Link
"set_full_info";
+ Link "get_block_size"; Link
"set_strict_mode"];
+ };
+
+ "get_request_block_size", {
+ default_call with
+ args = []; ret = RBool;
+ permitted_states = [];
+ shortdesc = "see if NBD_OPT_GO requests block size";
+ longdesc = "\
+Return the state of the block size request flag on this handle.";
+ see_also = [Link "set_request_block_size"];
+ };
+
"set_full_info", {
default_call with
args = [Bool "request"]; ret = RErr;
@@ -413,10 +452,11 @@ "set_full_info", {
Note that even when full info is requested, the server is not
obligated to reply with all information that libnbd requested.
Similarly, libnbd will ignore any optional server information that
-libnbd has not yet been taught to recognize.";
- example = Some "examples/server-flags.c";
+libnbd has not yet been taught to recognize. Furthermore, the
+hint to request block sizes is independently controlled via
+L<nbd_set_request_block_size(3)>.";
see_also = [Link "get_full_info"; Link
"get_canonical_export_name";
- Link "get_export_description"];
+ Link "get_export_description"; Link
"set_request_block_size"];
};
"get_full_info", {
@@ -1839,9 +1879,13 @@ "get_block_size", {
=back
Future NBD extensions may result in additional C<size_type> values.
+Note that by default, libnbd requests all available block sizes,
+but that a server may differ in what sizes it chooses to report
+if L<nbd_set_request_block_size(3)> alters whether the client
+requests sizes.
"
^ non_blocking_test_call_description;
- see_also = [Link "get_protocol";
+ see_also = [Link "get_protocol"; Link
"set_request_block_size";
Link "get_size"; Link "opt_info"]
};
@@ -1976,7 +2020,8 @@ "pread_structured", {
^ strict_call_description;
see_also = [Link "can_df"; Link "pread";
Link "aio_pread_structured"; Link
"get_block_size";
- Link "set_strict_mode"; Link
"set_pread_initialize"];
+ Link "set_strict_mode"; Link
"set_pread_initialize";
+ Link "set_request_block_size"];
};
"pwrite", {
@@ -3201,6 +3246,8 @@ let first_version (* Added in 1.11.x development cycle,
will be stable and supported in 1.12. *)
"set_pread_initialize", (1, 12);
"get_pread_initialize", (1, 12);
+ "set_request_block_size", (1, 12);
+ "get_request_block_size", (1, 12);
(* 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-go.c
b/generator/states-newstyle-opt-go.c
index eed769b3..b60c9eaa 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -1,5 +1,5 @@
/* nbd client library in userspace: state machine
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -67,7 +67,12 @@ STATE_MACHINE {
return 0;
NEWSTYLE.OPT_GO.SEND_EXPORT:
- uint16_t nrinfos = h->full_info ? 3 : 1;
+ uint16_t nrinfos = 0;
+
+ if (h->request_block_size)
+ nrinfos++;
+ if (h->full_info)
+ nrinfos += 2;
switch (send_from_wbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
@@ -80,14 +85,17 @@ STATE_MACHINE {
return 0;
NEWSTYLE.OPT_GO.SEND_NRINFOS:
- uint16_t nrinfos = h->full_info ? 3 : 1;
+ uint16_t nrinfos = 0;
switch (send_from_wbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
case 0:
- h->sbuf.info[0] = htobe16 (NBD_INFO_BLOCK_SIZE);
- h->sbuf.info[1] = htobe16 (NBD_INFO_NAME);
- h->sbuf.info[2] = htobe16 (NBD_INFO_DESCRIPTION);
+ if (h->request_block_size)
+ h->sbuf.info[nrinfos++] = htobe16 (NBD_INFO_BLOCK_SIZE);
+ if (h->full_info) {
+ h->sbuf.info[nrinfos++] = htobe16 (NBD_INFO_NAME);
+ h->sbuf.info[nrinfos++] = htobe16 (NBD_INFO_DESCRIPTION);
+ }
h->wbuf = &h->sbuf;
h->wlen = sizeof h->sbuf.info[0] * nrinfos;
SET_NEXT_STATE (%SEND_INFO);
diff --git a/lib/handle.c b/lib/handle.c
index 4f00c059..ea2ac3d8 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -242,6 +242,19 @@ nbd_unlocked_get_export_name (struct nbd_handle *h)
return copy;
}
+int
+nbd_unlocked_set_request_block_size (struct nbd_handle *h, bool request)
+{
+ h->request_block_size = request;
+ return 0;
+}
+
+int
+nbd_unlocked_get_request_block_size (struct nbd_handle *h)
+{
+ return h->request_block_size;
+}
+
int
nbd_unlocked_set_full_info (struct nbd_handle *h, bool request)
{
diff --git a/interop/Makefile.am b/interop/Makefile.am
index 56571660..cb7b3234 100644
--- a/interop/Makefile.am
+++ b/interop/Makefile.am
@@ -1,5 +1,5 @@
# nbd client library in userspace
-# Copyright (C) 2013-2021 Red Hat Inc.
+# Copyright (C) 2013-2022 Red Hat Inc.
#
# This library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
@@ -20,6 +20,7 @@ include $(top_srcdir)/subdir-rules.mk
EXTRA_DIST = \
dirty-bitmap.sh \
interop-qemu-storage-daemon.sh \
+ interop-qemu-block-size.sh \
list-exports-nbd-config \
list-exports-test-dir/disk1 \
list-exports-test-dir/disk2 \
@@ -141,6 +142,7 @@ TESTS += \
socket-activation-qemu-nbd \
dirty-bitmap.sh \
structured-read.sh \
+ interop-qemu-block-size.sh \
$(NULL)
interop_qemu_nbd_SOURCES = \
--
2.35.1
Richard W.M. Jones
2022-Feb-20 19:14 UTC
[Libguestfs] [libnbd PATCH] RFC: api: Add set_request_block_size
On Thu, Feb 17, 2022 at 05:04:13PM -0600, Eric Blake wrote:> WIP: I need to finish writing the actual interop-qemu-block-size.sh to > demonstrate scenarios where qemu-nbd advertises a block size > 1 to > clients that request it, but sticks to 1 otherwise (see > https://gitlab.com/qemu-project/qemu/-/blob/master/nbd/server.c#L660). > > Our testsuite coverage of nbd_get_block_size() is pretty sparse (a > single line in tests/errors.c, which was skipping until patches to > nbdkit finally made it possible to utilize). But in the process of > adding an interop test with qemu-nbd, I also noticed that qemu-nbd (at > least version 6.2) exposes different block sizes to older clients that > don't request block size than it does to newer clients which promise > to obey block sizes. We still want to request by default, but now we > need a knob, similar to the existing set_full_info(), for overriding > that default for testing purposes.The idea seems sensible.> @@ -20,6 +20,7 @@ include $(top_srcdir)/subdir-rules.mk > EXTRA_DIST = \ > dirty-bitmap.sh \ > interop-qemu-storage-daemon.sh \ > + interop-qemu-block-size.sh \ > list-exports-nbd-config \ > list-exports-test-dir/disk1 \ > list-exports-test-dir/disk2 \ > @@ -141,6 +142,7 @@ TESTS += \ > socket-activation-qemu-nbd \ > dirty-bitmap.sh \ > structured-read.sh \ > + interop-qemu-block-size.sh \ > $(NULL)Is there an issue with the whitespace on these lines? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v