Eric Blake
2022-Feb-24 17:26 UTC
[Libguestfs] [libnbd PATCH v2] api: Add set_request_block_size
Our testsuite coverage of nbd_get_block_size() is pretty sparse (the
recent commit 6f5fec2ea uses them in errors-server-unaligned.c for
debug purposes, and even that requires recent patches in nbdkit). But
in the process of adding an interop test with qemu-nbd, I also noticed
that qemu-nbd (at least version 6.2) fails NBD_OPT_INFO for older
clients that don't request block size, and fudges the value to 1 for
NBD_OPT_GO for back-compat reasons. We still want to request by
default, but now we need a knob, similar to the existing
set_full_info(), for overriding our defaults for testing purposes.
---
In v2:
- drop RFC
- fix bugs in implementation (the RFC version passed a wrong size over
the wire, causing 'nbdkit nbd' to deadlock)
- actually implement interop-qemu-block-size.sh
- add test coverage of language bindings
lib/internal.h | 5 +-
generator/API.ml | 52 ++++++++++++--
generator/states-newstyle-opt-go.c | 27 ++++++--
lib/handle.c | 14 ++++
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 +
interop/Makefile.am | 4 +-
interop/interop-qemu-block-size.sh | 80 ++++++++++++++++++++++
golang/libnbd_110_defaults_test.go | 8 +++
golang/libnbd_120_set_non_defaults_test.go | 14 +++-
12 files changed, 196 insertions(+), 16 deletions(-)
create mode 100755 interop/interop-qemu-block-size.sh
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..3e948aa2 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -395,6 +395,40 @@ "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)>); conversely, if a client does not request
+block sizes, the server may reject the connection instead of dealing
+with a client sending unaligned requests. This function makes it
+possible to test server behavior by emulating older clients.
+
+Note that even when block size is requested, the server is not
+obligated to provide any. Furthermore, if block sizes are provided
+(whether or not the client requested them), libnbd enforces alignment
+to those sizes unless L<nbd_set_strict_mode(3)> is used to bypass
+client-side safety checks.";
+ 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 +447,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 +1874,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 +2015,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 +3241,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..b7354aed 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
@@ -20,7 +20,12 @@
STATE_MACHINE {
NEWSTYLE.OPT_GO.START:
- uint16_t nrinfos = h->full_info ? 3 : 1;
+ uint16_t nrinfos = 0;
+
+ if (h->request_block_size)
+ nrinfos++;
+ if (h->full_info)
+ nrinfos += 2;
assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
if (h->opt_current == NBD_OPT_INFO)
@@ -67,7 +72,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 +90,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..8713e184 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -64,6 +64,7 @@ nbd_create (void)
h->unique = 1;
h->tls_verify_peer = true;
h->request_sr = true;
+ h->request_block_size = true;
h->pread_initialize = true;
h->uri_allow_transports = LIBNBD_ALLOW_TRANSPORT_MASK;
@@ -242,6 +243,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/python/t/110-defaults.py b/python/t/110-defaults.py
index a4262dae..749c94f4 100644
--- a/python/t/110-defaults.py
+++ b/python/t/110-defaults.py
@@ -22,6 +22,7 @@ assert h.get_export_name() == ""
assert h.get_full_info() is False
assert h.get_tls() == nbd.TLS_DISABLE
assert h.get_request_structured_replies() is True
+assert h.get_request_block_size() is True
assert h.get_pread_initialize() is True
assert h.get_handshake_flags() == nbd.HANDSHAKE_FLAG_MASK
assert h.get_opt_mode() is False
diff --git a/python/t/120-set-non-defaults.py b/python/t/120-set-non-defaults.py
index e71c6ad0..61a4160c 100644
--- a/python/t/120-set-non-defaults.py
+++ b/python/t/120-set-non-defaults.py
@@ -33,6 +33,8 @@ if h.supports_tls():
assert h.get_tls() == nbd.TLS_ALLOW
h.set_request_structured_replies(False)
assert h.get_request_structured_replies() is False
+h.set_request_block_size(False)
+assert h.get_request_block_size() is False
h.set_pread_initialize(False)
assert h.get_pread_initialize() is False
try:
diff --git a/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml
index 04aa744a..0fdd42e4 100644
--- a/ocaml/tests/test_110_defaults.ml
+++ b/ocaml/tests/test_110_defaults.ml
@@ -28,6 +28,8 @@ let
assert (tls = NBD.TLS.DISABLE);
let sr = NBD.get_request_structured_replies nbd in
assert (sr = true);
+ let bs = NBD.get_request_block_size nbd in
+ assert (bs = true);
let init = NBD.get_pread_initialize nbd in
assert (init = true);
let flags = NBD.get_handshake_flags nbd in
diff --git a/ocaml/tests/test_120_set_non_defaults.ml
b/ocaml/tests/test_120_set_non_defaults.ml
index f9498076..28e6b9fc 100644
--- a/ocaml/tests/test_120_set_non_defaults.ml
+++ b/ocaml/tests/test_120_set_non_defaults.ml
@@ -42,6 +42,9 @@ let
NBD.set_request_structured_replies nbd false;
let sr = NBD.get_request_structured_replies nbd in
assert (sr = false);
+ NBD.set_request_block_size nbd false;
+ let bs = NBD.get_request_block_size nbd in
+ assert (bs = false);
NBD.set_pread_initialize nbd false;
let init = NBD.get_pread_initialize nbd in
assert (init = false);
diff --git a/interop/Makefile.am b/interop/Makefile.am
index 56571660..27b1064a 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 = \
diff --git a/interop/interop-qemu-block-size.sh
b/interop/interop-qemu-block-size.sh
new file mode 100755
index 00000000..4ce287fd
--- /dev/null
+++ b/interop/interop-qemu-block-size.sh
@@ -0,0 +1,80 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2019-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
+# 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 interop with qemu-nbd block sizes.
+
+source ../tests/functions.sh
+set -e
+set -x
+
+requires qemu-nbd --version
+requires nbdsh --version
+requires qemu-img --version
+requires truncate --version
+
+f="qemu-block-size.raw"
+sock=$(mktemp -u /tmp/interop-qemu.XXXXXX)
+rm -f $f $sock
+cleanup_fn rm -f $f $sock
+
+truncate --size=10M $f
+export sock
+fail=0
+
+# run_test FMT REQUEST EXP_INFO EXP_GO
+run_test() {
+ # No -t or -e, so qemu-nbd should exit once nbdsh disconnects
+ qemu-nbd -k $sock $1 $f &
+ pid=$!
+ $VG nbdsh -c - <<EOF || fail=1
+import os
+
+sock = os.environ["sock"]
+
+h.set_opt_mode(True)
+assert h.get_request_block_size()
+h.set_request_block_size($2)
+assert h.get_request_block_size() is $2
+h.connect_unix(sock)
+
+try:
+ h.opt_info()
+ assert h.get_block_size(nbd.SIZE_MINIMUM) == $3
+except nbd.Error:
+ assert $3 == 0
+
+h.opt_go()
+assert h.get_block_size(nbd.SIZE_MINIMUM) == $4
+
+h.shutdown()
+EOF
+ wait $pid || fail=1
+}
+
+# Without '-f raw', qemu-nbd forces sector granularity to prevent
writing
+# to sector 0 from changing the disk type. However, if the client does
+# not request block sizes, it reports a size then errors out for NBD_OPT_INFO,
+# while fudging size for NBD_OPT_GO.
+run_test '' True 512 512
+run_test '' False 0 1
+
+# With '-f raw', qemu-nbd always exposes byte-level granularity for
files.
+run_test '-f raw' True 1 1
+run_test '-f raw' False 1 1
+
+exit $fail
diff --git a/golang/libnbd_110_defaults_test.go
b/golang/libnbd_110_defaults_test.go
index ca7c1c41..f56c9656 100644
--- a/golang/libnbd_110_defaults_test.go
+++ b/golang/libnbd_110_defaults_test.go
@@ -59,6 +59,14 @@ func Test110Defaults(t *testing.T) {
t.Fatalf("unexpected structured replies state")
}
+ bs, err := h.GetRequestBlockSize()
+ if err != nil {
+ t.Fatalf("could not get block size state: %s", err)
+ }
+ if bs != true {
+ t.Fatalf("unexpected block size state")
+ }
+
init, err := h.GetPreadInitialize()
if err != nil {
t.Fatalf("could not get pread initialize state: %s", err)
diff --git a/golang/libnbd_120_set_non_defaults_test.go
b/golang/libnbd_120_set_non_defaults_test.go
index 029f0db3..a4c411d0 100644
--- a/golang/libnbd_120_set_non_defaults_test.go
+++ b/golang/libnbd_120_set_non_defaults_test.go
@@ -1,5 +1,5 @@
/* libnbd golang tests
- * 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
@@ -93,6 +93,18 @@ func Test120SetNonDefaults(t *testing.T) {
t.Fatalf("unexpected structured replies state")
}
+ err = h.SetRequestBlockSize(false)
+ if err != nil {
+ t.Fatalf("could not set block size state: %s", err)
+ }
+ bs, err := h.GetRequestBlockSize()
+ if err != nil {
+ t.Fatalf("could not get block size state: %s", err)
+ }
+ if bs != false {
+ t.Fatalf("unexpected block size state")
+ }
+
err = h.SetPreadInitialize(false)
if err != nil {
t.Fatalf("could not set pread initialize state: %s", err)
--
2.35.1
Richard W.M. Jones
2022-Feb-24 17:34 UTC
[Libguestfs] [libnbd PATCH v2] api: Add set_request_block_size
On Thu, Feb 24, 2022 at 11:26:20AM -0600, Eric Blake wrote:> Our testsuite coverage of nbd_get_block_size() is pretty sparse (the > recent commit 6f5fec2ea uses them in errors-server-unaligned.c for > debug purposes, and even that requires recent patches in nbdkit). But > in the process of adding an interop test with qemu-nbd, I also noticed > that qemu-nbd (at least version 6.2) fails NBD_OPT_INFO for older > clients that don't request block size, and fudges the value to 1 for > NBD_OPT_GO for back-compat reasons. We still want to request by > default, but now we need a knob, similar to the existing > set_full_info(), for overriding our defaults for testing purposes. > --- > > In v2: > - drop RFC > - fix bugs in implementation (the RFC version passed a wrong size over > the wire, causing 'nbdkit nbd' to deadlock) > - actually implement interop-qemu-block-size.sh > - add test coverage of language bindings >[...] This version looks good, ACK Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org