[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