Document and make it easier for applications to determine which bitmask values were supported at compile time. Also, generating bitmask values in hex tends to be more legible than in decimal, as it makes it obvious that the values are intended for bitwise operations. And it doesn't hurt that this reduces churn in some of the tests if new bits are added. --- docs/libnbd.pod | 7 +++++++ generator/API.ml | 13 +++++++++++-- generator/C.ml | 13 ++++++++----- generator/GoLang.ml | 8 ++++++-- generator/OCaml.ml | 9 +++++++++ generator/Python.ml | 5 ++++- lib/handle.c | 5 ++--- python/t/110-defaults.py | 3 +-- python/t/120-set-non-defaults.py | 5 ++--- ocaml/tests/test_110_defaults.ml | 3 +-- ocaml/tests/test_120_set_non_defaults.ml | 3 +-- tests/errors.c | 9 +++------ .../libnbd/libnbd_110_defaults_test.go | 2 +- .../libnbd/libnbd_120_set_non_defaults_test.go | 4 ++-- 14 files changed, 58 insertions(+), 31 deletions(-) diff --git a/docs/libnbd.pod b/docs/libnbd.pod index f2ba3bb..d8dffea 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -520,6 +520,13 @@ To get the size of the export in bytes, use L<nbd_get_size(3)>: You can read and write data from the NBD server using L<nbd_pread(3)> and L<nbd_pwrite(3)> or their asynchronous equivalents. +All data commands support a C<flags> argument (mandatory in C, but +optional in languages where it can default to 0). For convenience, +the constant C<LIBNBD_CMD_FLAG_MASK> is defined with the set of flags +currently recognized by libnbd, where future NBD protocol extensions +may result in additional flags being supported; but in general, +specific data commands only accept a subset of known flags. + Some servers also support: =over 4 diff --git a/generator/API.ml b/generator/API.ml index cc1a7ba..42eeac0 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -679,6 +679,8 @@ fewer all-zero padding bytes over the connection. =back +For convenience, the constant C<LIBNBD_HANDSHAKE_FLAG_MASK> is +available to describe all flags supported by this build of libnbd. Future NBD extensions may add further flags, which in turn may be enabled by default in newer libnbd. As such, when attempting to disable only one specific bit, it is wiser to first call @@ -895,7 +897,11 @@ ORed together: =item C<LIBNBD_ALLOW_TRANSPORT_VSOCK> -=back"; +=back + +For convenience, the constant C<LIBNBD_ALLOW_TRANSPORT_MASK> is +available to describe all transports recognized by this build of +libnbd. A future version of the library may add new flags."; see_also = [Link "connect_uri"; Link "set_uri_allow_tls"]; }; @@ -1629,7 +1635,10 @@ issuing those commands before informing the server of the intent to disconnect. =back -"; + +For convenience, the constant C<LIBNBD_SHUTDOWN_MASK> is available +to describe all shutdown flags recognized by this build of libnbd. +A future version of the library may add new flags."; see_also = [Link "close"; Link "aio_disconnect"]; example = Some "examples/reads-and-writes.c"; }; diff --git a/generator/C.ml b/generator/C.ml index c9f0ff4..4d4958d 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -349,11 +349,15 @@ let generate_include_libnbd_h () = ) all_enums; List.iter ( fun { flag_prefix; flags } -> + let mask = ref 0 in List.iter ( fun (flag, i) -> let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in - pr "#define %-40s %d\n" flag i + pr "#define %-40s 0x%02x\n" flag i; + mask := !mask lor i ) flags; + let flag = sprintf "LIBNBD_%s_MASK" flag_prefix in + pr "#define %-40s 0x%02x\n" flag !mask; pr "\n" ) all_flags; List.iter ( @@ -490,13 +494,12 @@ let generate_lib_api_c () = ); (* Check parameters are valid. *) - let print_flags_check n { flag_prefix; flags } = + let print_flags_check n { flag_prefix } = let value = match errcode with | Some value -> value | None -> assert false in - let mask = List.fold_left (lor) 0 (List.map snd flags) in - pr " if (unlikely ((%s & ~%d) != 0)) {\n" n mask; - pr " set_error (EINVAL, \"%%s: invalid value for flag: %%d\",\n"; + pr " if (unlikely ((%s & ~LIBNBD_%s_MASK) != 0)) {\n" n flag_prefix; + pr " set_error (EINVAL, \"%%s: invalid value for flag: 0x%%x\",\n"; pr " \"%s\", %s);\n" n n; pr " ret = %s;\n" value; pr " goto out;\n"; diff --git a/generator/GoLang.ml b/generator/GoLang.ml index 359c7d0..f07b074 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -447,12 +447,16 @@ import ( "; List.iter ( fun { flag_prefix; flags } -> - pr "type %s uint32\n" (camel_case flag_prefix); + let flag_type = camel_case flag_prefix in + let mask = ref 0 in + pr "type %s uint32\n" flag_type; pr "const (\n"; List.iter ( fun (flag, v) -> - pr " %s_%s = %s(%d)\n" flag_prefix flag (camel_case flag_prefix) v + pr " %s_%s = %s(0x%02x)\n" flag_prefix flag flag_type v; + mask := !mask lor v ) flags; + pr " %s_MASK = %s(0x%02x)\n" flag_prefix flag_type !mask; pr ")\n"; pr "\n" ) all_flags; diff --git a/generator/OCaml.ml b/generator/OCaml.ml index 4bcd450..43b3679 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -164,6 +164,8 @@ type cookie = int64 pr " | %s\n" flag ) flags; pr " | UNKNOWN of int\n"; + pr "\n"; + pr " val mask : t list\n"; pr "end\n"; pr "\n" ) all_flags; @@ -269,6 +271,13 @@ let () = pr " | %s\n" flag ) flags; pr " | UNKNOWN of int\n"; + pr "\n"; + pr " let mask = [\n"; + List.iter ( + fun (flag, _) -> + pr " %s;\n" flag + ) flags; + pr " ]\n"; pr "end\n"; pr "\n" ) all_flags; diff --git a/generator/Python.ml b/generator/Python.ml index 71368c6..fd09eae 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -696,11 +696,14 @@ Error.__str__ = _str ) all_enums; List.iter ( fun { flag_prefix; flags } -> + let mask = ref 0 in List.iter ( fun (flag, i) -> let flag = sprintf "%s_%s" flag_prefix flag in - pr "%s = %d\n" flag i + pr "%s = 0x%02x\n" flag i; + mask := !mask lor i ) flags; + pr "%s_MASK = 0x%02x\n" flag_prefix !mask; pr "\n" ) all_flags; List.iter (fun (n, i) -> pr "%s = %d\n" n i) constants; diff --git a/lib/handle.c b/lib/handle.c index 7987d59..4d26842 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -65,12 +65,11 @@ nbd_create (void) h->tls_verify_peer = true; h->request_sr = true; - h->uri_allow_transports = (uint32_t) -1; + h->uri_allow_transports = LIBNBD_ALLOW_TRANSPORT_MASK; h->uri_allow_tls = LIBNBD_TLS_ALLOW; h->uri_allow_local_file = false; - h->gflags = (LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE | - LIBNBD_HANDSHAKE_FLAG_NO_ZEROES); + h->gflags = LIBNBD_HANDSHAKE_FLAG_MASK; s = getenv ("LIBNBD_DEBUG"); h->debug = s && strcmp (s, "1") == 0; diff --git a/python/t/110-defaults.py b/python/t/110-defaults.py index 47c1d02..fb961cf 100644 --- a/python/t/110-defaults.py +++ b/python/t/110-defaults.py @@ -22,6 +22,5 @@ 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_handshake_flags() == (nbd.HANDSHAKE_FLAG_FIXED_NEWSTYLE | - nbd.HANDSHAKE_FLAG_NO_ZEROES) +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 3963021..3da0c23 100644 --- a/python/t/120-set-non-defaults.py +++ b/python/t/120-set-non-defaults.py @@ -34,11 +34,10 @@ if h.supports_tls(): h.set_request_structured_replies(False) assert h.get_request_structured_replies() is False try: - h.set_handshake_flags(nbd.HANDSHAKE_FLAG_NO_ZEROES << 1) + h.set_handshake_flags(nbd.HANDSHAKE_FLAG_MASK + 1) assert False except nbd.Error: - assert h.get_handshake_flags() == (nbd.HANDSHAKE_FLAG_NO_ZEROES | - nbd.HANDSHAKE_FLAG_FIXED_NEWSTYLE) + assert h.get_handshake_flags() == nbd.HANDSHAKE_FLAG_MASK h.set_handshake_flags(0) assert h.get_handshake_flags() == 0 h.set_opt_mode(True) diff --git a/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml index 54f2cbc..f5886fc 100644 --- a/ocaml/tests/test_110_defaults.ml +++ b/ocaml/tests/test_110_defaults.ml @@ -28,8 +28,7 @@ let () = let sr = NBD.get_request_structured_replies nbd in assert (sr = true); let flags = NBD.get_handshake_flags nbd in - assert (flags = [ NBD.HANDSHAKE_FLAG.FIXED_NEWSTYLE; - NBD.HANDSHAKE_FLAG.NO_ZEROES ]); + assert (flags = NBD.HANDSHAKE_FLAG.mask); let opt = NBD.get_opt_mode nbd in assert (opt = false) diff --git a/ocaml/tests/test_120_set_non_defaults.ml b/ocaml/tests/test_120_set_non_defaults.ml index 79fe184..b660e5d 100644 --- a/ocaml/tests/test_120_set_non_defaults.ml +++ b/ocaml/tests/test_120_set_non_defaults.ml @@ -46,8 +46,7 @@ let () = with NBD.Error _ -> (); let flags = NBD.get_handshake_flags nbd in - assert (flags = [ NBD.HANDSHAKE_FLAG.FIXED_NEWSTYLE; - NBD.HANDSHAKE_FLAG.NO_ZEROES ]); + assert (flags = NBD.HANDSHAKE_FLAG.mask); NBD.set_handshake_flags nbd []; let flags = NBD.get_handshake_flags nbd in assert (flags = []); diff --git a/tests/errors.c b/tests/errors.c index 906c7da..8166429 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -86,7 +86,6 @@ main (int argc, char *argv[]) */ const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", "sh", script, NULL }; - int i; progname = argv[0]; @@ -147,15 +146,13 @@ main (int argc, char *argv[]) } /* Attempt to set a bitmask with an unknown bit. */ - i = LIBNBD_HANDSHAKE_FLAG_NO_ZEROES << 1; - if (nbd_set_handshake_flags (nbd, i) != -1) { + if (nbd_set_handshake_flags (nbd, LIBNBD_HANDSHAKE_FLAG_MASK + 1) != -1) { fprintf (stderr, "%s: test failed: " "nbd_set_handshake_flags did not reject invalid bitmask\n", argv[0]); exit (EXIT_FAILURE); } - i = LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE | LIBNBD_HANDSHAKE_FLAG_NO_ZEROES; - if (nbd_get_handshake_flags (nbd) != i) { + if (nbd_get_handshake_flags (nbd) != LIBNBD_HANDSHAKE_FLAG_MASK) { fprintf (stderr, "%s: test failed: " "nbd_get_handshake_flags not left at default value\n", argv[0]); @@ -247,7 +244,7 @@ main (int argc, char *argv[]) check (EINVAL, "nbd_pread: "); /* Use unknown command flags */ - if (nbd_pread (nbd, buf, 1, 0, -1) != -1) { + if (nbd_pread (nbd, buf, 1, 0, LIBNBD_CMD_FLAG_MASK + 1) != -1) { fprintf (stderr, "%s: test failed: " "nbd_pread did not fail with bogus flags\n", argv[0]); diff --git a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go index 9af0ed8..9e782ac 100644 --- a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go +++ b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go @@ -63,7 +63,7 @@ func Test110Defaults(t *testing.T) { if err != nil { t.Fatalf("could not get handshake flags: %s", err) } - if flags != HANDSHAKE_FLAG_FIXED_NEWSTYLE | HANDSHAKE_FLAG_NO_ZEROES { + if flags != HANDSHAKE_FLAG_MASK { t.Fatalf("unexpected handshake flags") } diff --git a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go index 5d8cce7..b0b06bd 100644 --- a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go +++ b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go @@ -93,7 +93,7 @@ func Test120SetNonDefaults(t *testing.T) { t.Fatalf("unexpected structured replies state") } - err = h.SetHandshakeFlags(HANDSHAKE_FLAG_NO_ZEROES << 1) + err = h.SetHandshakeFlags(HANDSHAKE_FLAG_MASK + 1) if err == nil { t.Fatalf("expect failure for out-of-range flags") } @@ -101,7 +101,7 @@ func Test120SetNonDefaults(t *testing.T) { if err != nil { t.Fatalf("could not get handshake flags: %s", err) } - if flags != HANDSHAKE_FLAG_FIXED_NEWSTYLE | HANDSHAKE_FLAG_NO_ZEROES { + if flags != HANDSHAKE_FLAG_MASK { t.Fatalf("unexpected handshake flags") } -- 2.28.0

Rather than having to open-code the list of accepted command flags in the unlocked version of each command, we can store that information in the generator to produce the check directly in the public API. --- generator/API.ml | 53 +++++++++++++++++++++++++++++---------------- generator/API.mli | 3 ++- generator/C.ml | 26 +++++++++++++++------- generator/GoLang.ml | 10 ++++----- generator/OCaml.ml | 6 ++--- generator/Python.ml | 10 ++++----- lib/disconnect.c | 10 --------- lib/rw.c | 45 -------------------------------------- 8 files changed, 67 insertions(+), 96 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index 42eeac0..b212e95 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -55,7 +55,7 @@ and arg = | UInt64 of string and optarg = | OClosure of closure -| OFlags of string * flags +| OFlags of string * flags * string list option and ret = | RBool | RStaticString @@ -1485,7 +1485,11 @@ Future NBD extensions may result in additional C<size_type> values. "pread", { default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + (* We could silently accept flag DF, but it really only makes sense + * with callbacks, because otherwise there is no observable change + * except that the server may fail where it would otherwise succeed. + *) + optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1509,7 +1513,7 @@ protocol extensions)."; default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset"; Closure chunk_closure ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["DF"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1585,7 +1589,7 @@ actually obeys the flag."; "pwrite", { default_call with args = [ BytesIn ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; @@ -1608,7 +1612,8 @@ L<nbd_can_fua(3)>)."; "shutdown", { default_call with - args = []; optargs = [ OFlags ("flags", shutdown_flags) ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", shutdown_flags, None) ]; + ret = RErr; permitted_states = [ Connected ]; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -1645,7 +1650,7 @@ A future version of the library may add new flags."; "flush", { default_call with - args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send flush command to the NBD server"; longdesc = "\ @@ -1662,7 +1667,7 @@ protocol extensions)."; "trim", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send trim command to the NBD server"; @@ -1685,7 +1690,7 @@ L<nbd_can_fua(3)>)."; "cache", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send cache (prefetch) command to the NBD server"; @@ -1705,7 +1710,8 @@ protocol extensions)."; "zero", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, + Some ["FUA"; "NO_HOLE"; "FAST_ZERO"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send write zeroes command to the NBD server"; @@ -1733,7 +1739,7 @@ cannot do this, see L<nbd_can_fast_zero(3)>)."; "block_status", { default_call with args = [ UInt64 "count"; UInt64 "offset"; Closure extent_closure ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send block status command to the NBD server"; @@ -2026,7 +2032,8 @@ callback."; "aio_pread", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some []) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -2048,7 +2055,8 @@ completed. Other parameters behave as documented in L<nbd_pread(3)>."; default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; Closure chunk_closure ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["DF"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -2067,7 +2075,8 @@ Other parameters behave as documented in L<nbd_pread_structured(3)>."; "aio_pwrite", { default_call with args = [ BytesPersistIn ("buf", "count"); UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; @@ -2086,7 +2095,7 @@ completed. Other parameters behave as documented in L<nbd_pwrite(3)>."; "aio_disconnect", { default_call with - args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -2111,7 +2120,8 @@ however, L<nbd_shutdown(3)> will call this function if appropriate."; "aio_flush", { default_call with args = []; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some []) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send flush command to the NBD server"; @@ -2130,7 +2140,8 @@ Other parameters behave as documented in L<nbd_flush(3)>."; "aio_trim", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send trim command to the NBD server"; @@ -2149,7 +2160,8 @@ Other parameters behave as documented in L<nbd_trim(3)>."; "aio_cache", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some []) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send cache (prefetch) command to the NBD server"; @@ -2168,7 +2180,9 @@ Other parameters behave as documented in L<nbd_cache(3)>."; "aio_zero", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, + Some ["FUA"; "NO_HOLE"; "FAST_ZERO"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send write zeroes command to the NBD server"; @@ -2188,7 +2202,8 @@ Other parameters behave as documented in L<nbd_zero(3)>."; "aio_block_status", { default_call with args = [ UInt64 "count"; UInt64 "offset"; Closure extent_closure ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send block status command to the NBD server"; diff --git a/generator/API.mli b/generator/API.mli index e45b5c0..db978ca 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -65,7 +65,8 @@ and arg = | UInt64 of string (** 64 bit unsigned int *) and optarg = | OClosure of closure (** optional closure *) -| OFlags of string * flags (** optional flags, uint32_t in C *) +| OFlags of string * flags * string list option (** optional flags, uint32_t + in C, and valid subset *) and ret = | RBool (** return a boolean, or error *) | RStaticString (** return a static string (must be located in diff --git a/generator/C.ml b/generator/C.ml index 4d4958d..86d9c5c 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -191,7 +191,7 @@ and print_arg_list' ?(handle = false) ?(types = true) ?(closure_style = Direct) | AddressOf -> "&" | Pointer -> "*" in pr "%s%s_callback" mark cbname - | OFlags (n, _) -> + | OFlags (n, _, _) -> if types then pr "uint32_t "; pr "%s" n ) optargs @@ -494,11 +494,21 @@ let generate_lib_api_c () = ); (* Check parameters are valid. *) - let print_flags_check n { flag_prefix } = + let print_flags_check n { flag_prefix; flags } subset = let value = match errcode with | Some value -> value | None -> assert false in - pr " if (unlikely ((%s & ~LIBNBD_%s_MASK) != 0)) {\n" n flag_prefix; + let mask = match subset with + | Some [] -> "0" + | Some subset -> + let v = ref 0 in + List.iter ( + fun (flag, i) -> + if List.mem flag subset then v := !v lor i + ) flags; + sprintf "0x%x" !v + | None -> "LIBNBD_" ^ flag_prefix ^ "_MASK" in + pr " if (unlikely ((%s & ~%s) != 0)) {\n" n mask; pr " set_error (EINVAL, \"%%s: invalid value for flag: 0x%%x\",\n"; pr " \"%s\", %s);\n" n n; pr " ret = %s;\n" value; @@ -536,7 +546,7 @@ let generate_lib_api_c () = pr " }\n"; need_out_label := true | Flags (n, flags) -> - print_flags_check n flags + print_flags_check n flags None | String n -> let value = match errcode with | Some value -> value @@ -552,8 +562,8 @@ let generate_lib_api_c () = List.iter ( function | OClosure _ -> () - | OFlags (n, flags) -> - print_flags_check n flags + | OFlags (n, flags, subset) -> + print_flags_check n flags subset ) optargs; (* Make the call. *) @@ -635,7 +645,7 @@ let generate_lib_api_c () = List.iter ( function | OClosure { cbname } -> pr " %s=%%s" cbname - | OFlags (n, _) -> pr " %s=0x%%x" n + | OFlags (n, _, _) -> pr " %s=0x%%x" n ) optargs; pr "\""; List.iter ( @@ -660,7 +670,7 @@ let generate_lib_api_c () = function | OClosure { cbname } -> pr ", CALLBACK_IS_NULL (%s_callback) ? \"<fun>\" : \"NULL\"" cbname - | OFlags (n, _) -> pr ", %s" n + | OFlags (n, _, _) -> pr ", %s" n ) optargs; pr ");\n"; List.iter ( diff --git a/generator/GoLang.ml b/generator/GoLang.ml index f07b074..81446a6 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -83,7 +83,7 @@ let go_arg_type = function let go_name_of_optarg = function | OClosure { cbname } -> sprintf "%sCallback" (camel_case cbname) - | OFlags (n, _) -> String.capitalize_ascii n + | OFlags (n, _, _) -> String.capitalize_ascii n let go_ret_type = function (* RErr returns only the error, with no return value. *) @@ -202,7 +202,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = pr " %s " fname; (match optarg with | OClosure { cbname } -> pr "%sCallback" (camel_case cbname) - | OFlags (_, {flag_prefix}) -> pr "%s" (camel_case flag_prefix) + | OFlags (_, {flag_prefix}, _) -> pr "%s" (camel_case flag_prefix) ); pr "\n" ) optargs; @@ -298,7 +298,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = function | OClosure { cbname } -> pr " var c_%s C.nbd_%s_callback\n" cbname cbname - | OFlags (n, _) -> pr " var c_%s C.uint32_t\n" n + | OFlags (n, _, _) -> pr " var c_%s C.uint32_t\n" n ) optargs; pr " if optargs != nil {\n"; List.iter ( @@ -312,7 +312,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = cbname cbname; pr " c_%s.user_data = unsafe.Pointer (C.long_to_vp (C.long (registerCallbackId (optargs.%s))))\n" cbname (go_name_of_optarg optarg) - | OFlags (n, _) -> + | OFlags (n, _, _) -> pr " c_%s = C.uint32_t (optargs.%s)\n" n (go_name_of_optarg optarg); ); @@ -346,7 +346,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = List.iter ( function | OClosure { cbname} -> pr ", c_%s" cbname - | OFlags (n, _) -> pr ", c_%s" n + | OFlags (n, _, _) -> pr ", c_%s" n ) optargs; pr ")\n"; diff --git a/generator/OCaml.ml b/generator/OCaml.ml index 43b3679..28acb50 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -72,7 +72,7 @@ and ocaml_ret_to_string = function and ocaml_optarg_to_string = function | OClosure { cbname; cbargs } -> sprintf "?%s:(%s)" cbname (ocaml_closuredecl_to_string cbargs) - | OFlags (n, { flag_prefix }) -> sprintf "?%s:%s.t list" n flag_prefix + | OFlags (n, { flag_prefix }, _) -> sprintf "?%s:%s.t list" n flag_prefix and ocaml_closuredecl_to_string cbargs = let cbargs = List.map ocaml_cbarg_to_string cbargs in @@ -112,7 +112,7 @@ let ocaml_name_of_arg = function let ocaml_name_of_optarg = function | OClosure { cbname } -> cbname - | OFlags (n, _) -> n + | OFlags (n, _, _) -> n let num_params args optargs = List.length optargs + 1 (* handle *) + List.length args @@ -614,7 +614,7 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " }\n"; pr " %s_callback.user_data = %s_user_data;\n" cbname cbname; pr " %s_callback.free = free_user_data;\n" cbname; - | OFlags (n, { flag_prefix }) -> + | OFlags (n, { flag_prefix }, _) -> pr " uint32_t %s;\n" n; pr " if (%sv != Val_int (0)) /* Some [ list of %s.t ] */\n" n flag_prefix; diff --git a/generator/Python.ml b/generator/Python.ml index fd09eae..1705ad9 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -320,7 +320,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " nbd_%s_callback %s = { .callback = %s_wrapper,\n" cbname cbname cbname; pr " .free = free_user_data };\n" - | OFlags (n, _) -> + | OFlags (n, _, _) -> pr " uint32_t %s_u32;\n" n; pr " unsigned int %s; /* really uint32_t */\n" n ) optargs; @@ -378,7 +378,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = List.iter ( function | OClosure { cbname } -> pr ", &py_%s_fn" cbname - | OFlags (n, _) -> pr ", &%s" n + | OFlags (n, _, _) -> pr ", &%s" n ) optargs; pr "))\n"; pr " goto out;\n"; @@ -402,7 +402,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " }\n"; pr " else\n"; pr " %s.callback = NULL; /* we're not going to call it */\n" cbname - | OFlags (n, _) -> pr " %s_u32 = %s;\n" n n + | OFlags (n, _, _) -> pr " %s_u32 = %s;\n" n n ) optargs; List.iter ( function @@ -472,7 +472,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = List.iter ( function | OClosure { cbname } -> pr ", %s" cbname - | OFlags (n, _) -> pr ", %s_u32" n + | OFlags (n, _, _) -> pr ", %s_u32" n ) optargs; pr ");\n"; List.iter ( @@ -802,7 +802,7 @@ class NBD(object): List.map ( function | OClosure { cbname } -> cbname, Some "None", None - | OFlags (n, _) -> n, Some "0", None + | OFlags (n, _, _) -> n, Some "0", None ) optargs in let args = args @ optargs in pr " def %s(" name; diff --git a/lib/disconnect.c b/lib/disconnect.c index b8356b7..9de1e34 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -30,11 +30,6 @@ int nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) { - if ((flags & ~LIBNBD_SHUTDOWN_IMMEDIATE) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - /* If IMMEDIATE, abort any commands that have not yet had any bytes * sent to the server, so that NBD_CMD_DISC will be first in line. */ @@ -69,11 +64,6 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); if (id == -1) return -1; diff --git a/lib/rw.c b/lib/rw.c index f3adb71..95c002c 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -280,15 +280,6 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, { struct command_cb cb = { .completion = *completion }; - /* We could silently accept flag DF, but it really only makes sense - * with callbacks, because otherwise there is no observable change - * except that the server may fail where it would otherwise succeed. - */ - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, buf, &cb); @@ -304,11 +295,6 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, struct command_cb cb = { .fn.chunk = *chunk, .completion = *completion }; - if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_DF) != 0 && nbd_unlocked_can_df (h) != 1) { set_error (EINVAL, "server does not support the DF flag"); @@ -334,11 +320,6 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, return -1; } - if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && nbd_unlocked_can_fua (h) != 1) { set_error (EINVAL, "server does not support the FUA flag"); @@ -362,11 +343,6 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, return -1; } - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0, NULL, &cb); @@ -389,11 +365,6 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, return -1; } - if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && nbd_unlocked_can_fua (h) != 1) { set_error (EINVAL, "server does not support the FUA flag"); @@ -427,11 +398,6 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, return -1; } - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count, NULL, &cb); @@ -454,12 +420,6 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, return -1; } - if ((flags & ~(LIBNBD_CMD_FLAG_FUA | LIBNBD_CMD_FLAG_NO_HOLE | - LIBNBD_CMD_FLAG_FAST_ZERO)) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && nbd_unlocked_can_fua (h) != 1) { set_error (EINVAL, "server does not support the FUA flag"); @@ -504,11 +464,6 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, return -1; } - if ((flags & ~LIBNBD_CMD_FLAG_REQ_ONE) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if (count == 0) { /* NBD protocol forbids this. */ set_error (EINVAL, "count cannot be 0"); return -1; -- 2.28.0

Right now, libnbd has refused to issue a command not advertised as supported by a server, mainly because the NBD protocol does not guarantee what the server will do, and libnbd would rather stay in sync with the server than drop the connection. However, for integration purposes, it can be handy to coerce libnbd into sending something to see how the server will react (whether it be an extension libnbd has not yet learned, or an intentional bad request to test server error handling). Time to make this something the user can control, by adding a new strictness mode. Later patches will add more knobs to the mode. --- lib/internal.h | 3 + generator/API.ml | 165 ++++++++++++++++++++++++++++++++++++----------- lib/handle.c | 16 +++++ lib/rw.c | 134 +++++++++++++++++++++----------------- tests/errors.c | 21 +++++- 5 files changed, 240 insertions(+), 99 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 96699b5..2a5147f 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -148,6 +148,9 @@ struct nbd_handle { bool debug; nbd_debug_callback debug_callback; + /* How strict to be. */ + uint32_t strict; + /* State machine. * * The actual current state is ‘state’. ‘public_state’ is updated diff --git a/generator/API.ml b/generator/API.ml index b212e95..aa970e6 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -102,6 +102,13 @@ and link = | ExternalLink of string * int | URLLink of string +let strict_call_description = "\n +By default, libnbd will reject attempts to use this function with +parameters that are likely to result in server failure, such as +requesting an unknown command flag. The L<nbd_set_strict_mode(3)> +function can be used to alter which scenarios should await a server +reply rather than failing fast." + let non_blocking_test_call_description = "\n This call does not block, because it returns data that is saved in the handle from the NBD protocol handshake." @@ -171,7 +178,13 @@ let handshake_flags = { flags = [ "FIXED_NEWSTYLE", 1 lsl 0; "NO_ZEROES", 1 lsl 1; - ] + ] +} +let strict_flags = { + flag_prefix = "STRICT"; + flags = [ + "COMMANDS", 1 lsl 0; + ] } let allow_transport_flags = { flag_prefix = "ALLOW_TRANSPORT"; @@ -187,8 +200,8 @@ let shutdown_flags = { "IMMEDIATE", 1 lsl 1; ] } -let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags; - shutdown_flags ] +let all_flags = [ cmd_flags; handshake_flags; strict_flags; + allow_transport_flags; shutdown_flags ] let default_call = { args = []; optargs = []; ret = RErr; shortdesc = ""; longdesc = ""; example = None; @@ -451,7 +464,7 @@ test whether this is the case with L<nbd_supports_tls(3)>."; "get_tls", { default_call with - args = []; ret = REnum (tls_enum); + args = []; ret = REnum tls_enum; may_set_error = false; shortdesc = "get the TLS request setting"; longdesc = "\ @@ -610,7 +623,7 @@ for integration testing, it can be useful to clear this flag rather than find a way to alter the server to fail the negotiation request."; see_also = [Link "get_request_structured_replies"; - Link "set_handshake_flags"; + Link "set_handshake_flags"; Link "set_strict_mode"; Link "get_structured_replies_negotiated"; Link "can_meta_context"; Link "can_df"]; }; @@ -692,7 +705,7 @@ blindly setting a constant value."; "get_handshake_flags", { default_call with - args = []; ret = RFlags (handshake_flags); + args = []; ret = RFlags handshake_flags; may_set_error = false; shortdesc = "see which handshake flags are supported"; longdesc = "\ @@ -706,10 +719,62 @@ protocol defines new handshake flags, then the return value from a newer library version may include bits that were undefined at the time of compilation."; see_also = [Link "set_handshake_flags"; - Link "get_protocol"; + Link "get_protocol"; Link "set_strict_mode"; Link "aio_is_created"; Link "aio_is_ready"]; }; + "set_strict_mode", { + default_call with + args = [ Flags ("flags", strict_flags) ]; ret = RErr; + shortdesc = "control how strictly to follow NBD protocol"; + longdesc = "\ +By default, libnbd tries to detect requests that would trigger +undefined behavior in the NBD protocol, and rejects them client +side without causing any network traffic, rather than risking +undefined server behavior. However, for integration testing, it +can be handy to relax the strictness of libnbd, to coerce it into +sending such requests over the network for testing the robustness +of the server in dealing with such traffic. + +The C<flags> argument is a bitmask, including zero or more of the +following strictness flags: + +=over 4 + +=item C<LIBNBD_STRICT_COMMANDS> = 1 + +If set, this flag rejects client requests that do not comply with the +set of advertised server flags (for example, attempting a write on +a read-only server, or attempting to use C<LIBNBD_CMD_FLAG_FUA> when +L<nbd_can_fua(3)> returned false). If clear, this flag relies on the +server to reject unexpected commands. + +=back + +For convenience, the constant C<LIBNBD_STRICT_MASK> is available to +describe all strictness flags supported by this build of libnbd. +Future versions of libnbd may add further flags, which are likely +to be enabled by default for additional client-side filtering. As +such, when attempting to relax only one specific bit while keeping +remaining checks at the client side, it is wiser to first call +L<nbd_get_strict_mode(3)> and modify that value, rather than +blindly setting a constant value."; + see_also = [Link "get_strict_mode"; Link "set_handshake_flags"]; + }; + + "get_strict_mode", { + default_call with + args = []; ret = RFlags strict_flags; + may_set_error = false; + shortdesc = "see which strictness flags are in effect"; + longdesc = "\ +Return flags indicating which protocol strictness items are being +enforced locally by libnbd rather than the server. The return value +from a newer library version may include bits that were undefined at +the time of compilation."; + see_also = [Link "set_strict_mode"]; + }; + "set_opt_mode", { default_call with args = [Bool "enable"]; ret = RErr; @@ -1598,15 +1663,18 @@ Issue a write command to the NBD server, writing the data in C<buf> to the range starting at C<offset> and ending at C<offset> + C<count> - 1. NBD can only write all or nothing using this call. The call returns when the command has been -acknowledged by the server, or there is an error. +acknowledged by the server, or there is an error. Note this will +generally return an error if L<nbd_is_read_only(3)> is true. The C<flags> parameter may be C<0> for no flags, or may contain C<LIBNBD_CMD_FLAG_FUA> meaning that the server should not return until the data has been committed to permanent storage (if that is supported - some servers cannot do this, see -L<nbd_can_fua(3)>)."; +L<nbd_can_fua(3)>)." +^ strict_call_description; see_also = [Link "can_fua"; Link "is_read_only"; - Link "aio_pwrite"; Link "get_block_size"]; + Link "aio_pwrite"; Link "get_block_size"; + Link "set_strict_mode"]; example = Some "examples/reads-and-writes.c"; }; @@ -1657,11 +1725,13 @@ A future version of the library may add new flags."; Issue the flush command to the NBD server. The function should return when all write commands which have completed have been committed to permanent storage on the server. Note this will -return an error if L<nbd_can_flush(3)> is false. +generally return an error if L<nbd_can_flush(3)> is false. The C<flags> parameter must be C<0> for now (it exists for future NBD -protocol extensions)."; - see_also = [Link "can_flush"; Link "aio_flush"]; +protocol extensions)." +^ strict_call_description; + see_also = [Link "can_flush"; Link "aio_flush"; + Link "set_strict_mode"]; }; "trim", { @@ -1676,15 +1746,17 @@ Issue a trim command to the NBD server, which if supported by the server causes a hole to be punched in the backing store starting at C<offset> and ending at C<offset> + C<count> - 1. The call returns when the command has been acknowledged by the server, -or there is an error. +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. The C<flags> parameter may be C<0> for no flags, or may contain C<LIBNBD_CMD_FLAG_FUA> meaning that the server should not return until the data has been committed to permanent storage (if that is supported - some servers cannot do this, see -L<nbd_can_fua(3)>)."; - see_also = [Link "can_fua"; Link "can_trim"; - Link "aio_trim"]; +L<nbd_can_fua(3)>)." +^ strict_call_description; + see_also = [Link "can_fua"; Link "can_trim"; Link "is_read_only"; + Link "aio_trim"; Link "set_strict_mode"]; }; "cache", { @@ -1699,12 +1771,14 @@ Issue the cache (prefetch) command to the NBD server, which if supported by the server causes data to be prefetched into faster storage by the server, speeding up a subsequent L<nbd_pread(3)> call. The server can also silently ignore -this command. Note this will return an error if +this command. Note this will generally return an error if L<nbd_can_cache(3)> is false. The C<flags> parameter must be C<0> for now (it exists for future NBD -protocol extensions)."; - see_also = [Link "can_cache"; Link "aio_cache"]; +protocol extensions)." +^ strict_call_description; + see_also = [Link "can_cache"; Link "aio_cache"; + Link "set_strict_mode"]; }; "zero", { @@ -1720,7 +1794,8 @@ Issue a write zeroes command to the NBD server, which if supported by the server causes a zeroes to be written efficiently starting at C<offset> and ending at C<offset> + C<count> - 1. The call returns when the command has been acknowledged by the server, -or there is an error. +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. The C<flags> parameter may be C<0> for no flags, or may contain C<LIBNBD_CMD_FLAG_FUA> meaning that the server should not @@ -1731,9 +1806,11 @@ the server should favor writing actual allocated zeroes over punching a hole, and/or C<LIBNBD_CMD_FLAG_FAST_ZERO> meaning that the server must fail quickly if writing zeroes is no faster than a normal write (if that is supported - some servers -cannot do this, see L<nbd_can_fast_zero(3)>)."; - see_also = [Link "can_fua"; Link "can_zero"; - Link "can_fast_zero"; Link "aio_zero"]; +cannot do this, see L<nbd_can_fast_zero(3)>)." +^ strict_call_description; + see_also = [Link "can_fua"; Link "can_zero"; Link "is_read_only"; + Link "can_fast_zero"; Link "aio_zero"; + Link "set_strict_mode"]; }; "block_status", { @@ -1792,9 +1869,10 @@ The C<flags> parameter may be C<0> for no flags, or may contain C<LIBNBD_CMD_FLAG_REQ_ONE> meaning that the server should return only one extent per metadata context where that extent does not exceed C<count> bytes; however, libnbd does not -validate that the server obeyed the flag."; +validate that the server obeyed the flag." +^ strict_call_description; see_also = [Link "add_meta_context"; Link "can_meta_context"; - Link "aio_block_status"]; + Link "aio_block_status"; Link "set_strict_mode"]; }; "poll", { @@ -2088,9 +2166,10 @@ Or supply the optional C<completion_callback> which will be invoked as described in L<libnbd(3)/Completion callbacks>. Note that you must ensure C<buf> is valid until the command has -completed. Other parameters behave as documented in L<nbd_pwrite(3)>."; +completed. Other parameters behave as documented in L<nbd_pwrite(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; - Link "is_read_only"; Link "pwrite"]; + Link "is_read_only"; Link "pwrite"; Link "set_strict_mode"]; }; "aio_disconnect", { @@ -2132,9 +2211,10 @@ 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_flush(3)>."; +Other parameters behave as documented in L<nbd_flush(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; - Link "can_flush"; Link "flush"]; + Link "can_flush"; Link "flush"; Link "set_strict_mode"]; }; "aio_trim", { @@ -2152,9 +2232,10 @@ 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_trim(3)>."; +Other parameters behave as documented in L<nbd_trim(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; - Link "can_trim"; Link "trim"]; + Link "can_trim"; Link "trim"; Link "set_strict_mode"]; }; "aio_cache", { @@ -2172,9 +2253,10 @@ 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_cache(3)>."; +Other parameters behave as documented in L<nbd_cache(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; - Link "can_cache"; Link "cache"]; + Link "can_cache"; Link "cache"; Link "set_strict_mode"]; }; "aio_zero", { @@ -2193,10 +2275,11 @@ 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_zero(3)>."; +Other parameters behave as documented in L<nbd_zero(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; Link "can_zero"; Link "can_fast_zero"; - Link "zero"]; + Link "zero"; Link "set_strict_mode"]; }; "aio_block_status", { @@ -2214,9 +2297,11 @@ 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(3)>."; +Other parameters behave as documented in L<nbd_block_status(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; - Link "can_meta_context"; Link "block_status"]; + Link "can_meta_context"; Link "block_status"; + Link "set_strict_mode"]; }; "aio_get_fd", { @@ -2663,6 +2748,10 @@ let first_version = [ "aio_opt_list", (1, 4); "aio_opt_info", (1, 4); + (* Added in 1.5.x development cycle, will be stable and supported in 1.6. *) + "set_strict_mode", (1, 6); + "get_strict_mode", (1, 6); + (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. "get_tls_certificates", (1, ??); diff --git a/lib/handle.c b/lib/handle.c index 4d26842..2dc1fb5 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -74,6 +74,8 @@ nbd_create (void) s = getenv ("LIBNBD_DEBUG"); h->debug = s && strcmp (s, "1") == 0; + h->strict = LIBNBD_STRICT_MASK; + h->public_state = STATE_START; h->state = STATE_START; h->pid = -1; @@ -349,6 +351,20 @@ nbd_unlocked_get_handshake_flags (struct nbd_handle *h) return h->gflags; } +int +nbd_unlocked_set_strict_mode (struct nbd_handle *h, uint32_t flags) +{ + h->strict = flags; + return 0; +} + +/* NB: may_set_error = false. */ +uint32_t +nbd_unlocked_get_strict_mode (struct nbd_handle *h) +{ + return h->strict; +} + const char * nbd_unlocked_get_package_name (struct nbd_handle *h) { diff --git a/lib/rw.c b/lib/rw.c index 95c002c..f49fe25 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -295,10 +295,12 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, struct command_cb cb = { .fn.chunk = *chunk, .completion = *completion }; - if ((flags & LIBNBD_CMD_FLAG_DF) != 0 && - nbd_unlocked_can_df (h) != 1) { - set_error (EINVAL, "server does not support the DF flag"); - return -1; + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if ((flags & LIBNBD_CMD_FLAG_DF) != 0 && + nbd_unlocked_can_df (h) != 1) { + set_error (EINVAL, "server does not support the DF flag"); + return -1; + } } SET_CALLBACK_TO_NULL (*chunk); @@ -315,15 +317,17 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, { struct command_cb cb = { .completion = *completion }; - if (nbd_unlocked_is_read_only (h) == 1) { - set_error (EPERM, "server does not support write operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_is_read_only (h) == 1) { + set_error (EPERM, "server does not support write operations"); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && - nbd_unlocked_can_fua (h) != 1) { - set_error (EINVAL, "server does not support the FUA flag"); - return -1; + if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && + nbd_unlocked_can_fua (h) != 1) { + set_error (EINVAL, "server does not support the FUA flag"); + return -1; + } } SET_CALLBACK_TO_NULL (*completion); @@ -338,9 +342,11 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, { struct command_cb cb = { .completion = *completion }; - if (nbd_unlocked_can_flush (h) != 1) { - set_error (EINVAL, "server does not support flush operations"); - return -1; + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_can_flush (h) != 1) { + set_error (EINVAL, "server does not support flush operations"); + return -1; + } } SET_CALLBACK_TO_NULL (*completion); @@ -356,19 +362,21 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, { struct command_cb cb = { .completion = *completion }; - if (nbd_unlocked_can_trim (h) != 1) { - set_error (EINVAL, "server does not support trim operations"); - return -1; - } - if (nbd_unlocked_is_read_only (h) == 1) { - set_error (EPERM, "server does not support write operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_can_trim (h) != 1) { + set_error (EINVAL, "server does not support trim operations"); + return -1; + } + if (nbd_unlocked_is_read_only (h) == 1) { + set_error (EPERM, "server does not support write operations"); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && - nbd_unlocked_can_fua (h) != 1) { - set_error (EINVAL, "server does not support the FUA flag"); - return -1; + if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && + nbd_unlocked_can_fua (h) != 1) { + set_error (EINVAL, "server does not support the FUA flag"); + return -1; + } } if (count == 0) { /* NBD protocol forbids this. */ @@ -389,13 +397,15 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, { struct command_cb cb = { .completion = *completion }; - /* Actually according to the NBD protocol document, servers do exist - * that support NBD_CMD_CACHE but don't advertise the - * NBD_FLAG_SEND_CACHE bit, but we ignore those. - */ - if (nbd_unlocked_can_cache (h) != 1) { - set_error (EINVAL, "server does not support cache operations"); - return -1; + if (h->strict & LIBNBD_STRICT_COMMANDS) { + /* Actually according to the NBD protocol document, servers do exist + * that support NBD_CMD_CACHE but don't advertise the + * NBD_FLAG_SEND_CACHE bit, but we ignore those. + */ + if (nbd_unlocked_can_cache (h) != 1) { + set_error (EINVAL, "server does not support cache operations"); + return -1; + } } SET_CALLBACK_TO_NULL (*completion); @@ -411,25 +421,27 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, { struct command_cb cb = { .completion = *completion }; - if (nbd_unlocked_can_zero (h) != 1) { - set_error (EINVAL, "server does not support zero operations"); - return -1; - } - if (nbd_unlocked_is_read_only (h) == 1) { - set_error (EPERM, "server does not support write operations"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (nbd_unlocked_can_zero (h) != 1) { + set_error (EINVAL, "server does not support zero operations"); + return -1; + } + if (nbd_unlocked_is_read_only (h) == 1) { + set_error (EPERM, "server does not support write operations"); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && - nbd_unlocked_can_fua (h) != 1) { - set_error (EINVAL, "server does not support the FUA flag"); - return -1; - } + if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && + nbd_unlocked_can_fua (h) != 1) { + set_error (EINVAL, "server does not support the FUA flag"); + return -1; + } - if ((flags & LIBNBD_CMD_FLAG_FAST_ZERO) != 0 && - nbd_unlocked_can_fast_zero (h) != 1) { - set_error (EINVAL, "server does not support the fast zero flag"); - return -1; + if ((flags & LIBNBD_CMD_FLAG_FAST_ZERO) != 0 && + nbd_unlocked_can_fast_zero (h) != 1) { + set_error (EINVAL, "server does not support the fast zero flag"); + return -1; + } } if (count == 0) { /* NBD protocol forbids this. */ @@ -452,16 +464,18 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, struct command_cb cb = { .fn.extent = *extent, .completion = *completion }; - if (!h->structured_replies) { - set_error (ENOTSUP, "server does not support structured replies"); - return -1; - } + if (h->strict & LIBNBD_STRICT_COMMANDS) { + if (!h->structured_replies) { + set_error (ENOTSUP, "server does not support structured replies"); + return -1; + } - if (h->meta_contexts == NULL) { - 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; + if (h->meta_contexts == NULL) { + 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; + } } if (count == 0) { /* NBD protocol forbids this. */ diff --git a/tests/errors.c b/tests/errors.c index 8166429..0c4151a 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -86,6 +86,7 @@ main (int argc, char *argv[]) */ const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", "sh", script, NULL }; + uint32_t strict; progname = argv[0]; @@ -269,7 +270,25 @@ main (int argc, char *argv[]) } check (ERANGE, "nbd_aio_pwrite: "); - /* Use unadvertised command */ + /* Use unadvertised command, client-side */ + strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_COMMANDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_trim (nbd, 512, 0, 0) != -1) { + fprintf (stderr, "%s: test failed: " + "unpermitted nbd_trim did not fail\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_trim: "); + /* Use unadvertised command, server-side */ + strict &= ~LIBNBD_STRICT_COMMANDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_trim (nbd, 512, 0, 0) != -1) { fprintf (stderr, "%s: test failed: " "unpermitted nbd_trim did not fail\n", -- 2.28.0

The next strict knob: allow the user to pass unknown flags across the wire (this is different than passing a known flag at the wrong time). It is interesting to note that NBD only permits 16 bits of flags, but we have a signature that takes uint32_t; if we wanted, we could pack libnbd-specific flags in the upper bits that the NBD protocol would never see. --- generator/API.ml | 46 ++++++++++++++++++++++++++++++++++++++-------- generator/API.mli | 1 + generator/C.ml | 7 +++++-- lib/disconnect.c | 2 +- lib/rw.c | 6 +++--- tests/errors.c | 22 ++++++++++++++++++++-- 6 files changed, 68 insertions(+), 16 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index aa970e6..4cd425b 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -87,6 +87,7 @@ and enum = { } and flags = { flag_prefix : string; + guard : string option; flags : (string * int) list } and permitted_state = @@ -165,6 +166,7 @@ let all_enums = [ tls_enum; block_size_enum ] (* Flags. See also Constants below. *) let cmd_flags = { flag_prefix = "CMD_FLAG"; + guard = Some "((h->strict & LIBNBD_STRICT_FLAGS) || flags > UINT16_MAX)"; flags = [ "FUA", 1 lsl 0; "NO_HOLE", 1 lsl 1; @@ -175,6 +177,7 @@ let cmd_flags = { } let handshake_flags = { flag_prefix = "HANDSHAKE_FLAG"; + guard = None; flags = [ "FIXED_NEWSTYLE", 1 lsl 0; "NO_ZEROES", 1 lsl 1; @@ -182,12 +185,15 @@ let handshake_flags = { } let strict_flags = { flag_prefix = "STRICT"; + guard = None; flags = [ "COMMANDS", 1 lsl 0; + "FLAGS", 1 lsl 1; ] } let allow_transport_flags = { flag_prefix = "ALLOW_TRANSPORT"; + guard = None; flags = [ "TCP", 1 lsl 0; "UNIX", 1 lsl 1; @@ -196,6 +202,7 @@ let allow_transport_flags = { } let shutdown_flags = { flag_prefix = "SHUTDOWN"; + guard = None; flags = [ "IMMEDIATE", 1 lsl 1; ] @@ -749,6 +756,22 @@ a read-only server, or attempting to use C<LIBNBD_CMD_FLAG_FUA> when L<nbd_can_fua(3)> returned false). If clear, this flag relies on the server to reject unexpected commands. +=item C<LIBNBD_STRICT_FLAGS> = 2 + +If set, this flag rejects client requests that attempt to set a +command flag not recognized by libnbd (those outside of +C<LIBNBD_CMD_FLAG_MASK>). If clear, this flag passes on unknown +flags to the server. One possible reason to relax this strictness +knob is to send C<LIBNBD_CMD_FLAG_FUA> on a read command (libnbd +normally prevents that, but the NBD protocol allows it to succeed); +however, it is also possible that sending an unknown flag may +cause the server to change its reply in a manner that confuses +libnbd, perhaps causing deadlock or ending the connection. + +Note that the NBD protocol only supports 16 bits of command flags, +even though the libnbd API uses C<uint32_t>; bits outside of the +range permitted by the protocol are always a client-side error. + =back For convenience, the constant C<LIBNBD_STRICT_MASK> is available to @@ -1568,9 +1591,10 @@ required into the server's replies, or if you want to use C<LIBNBD_CMD_FLAG_DF>. The C<flags> parameter must be C<0> for now (it exists for future NBD -protocol extensions)."; +protocol extensions)." +^ strict_call_description; see_also = [Link "aio_pread"; Link "pread_structured"; - Link "get_block_size"]; + Link "get_block_size"; Link "set_strict_mode"]; example = Some "examples/fetch-first-sector.c"; }; @@ -1646,9 +1670,11 @@ The C<flags> parameter may be C<0> for no flags, or may contain C<LIBNBD_CMD_FLAG_DF> meaning that the server should not reply with more than one fragment (if that is supported - some servers cannot do this, see L<nbd_can_df(3)>). Libnbd does not validate that the server -actually obeys the flag."; +actually obeys the flag." +^ strict_call_description; see_also = [Link "can_df"; Link "pread"; - Link "aio_pread_structured"; Link "get_block_size"]; + Link "aio_pread_structured"; Link "get_block_size"; + Link "set_strict_mode"]; }; "pwrite", { @@ -2123,10 +2149,12 @@ Or supply the optional C<completion_callback> which will be invoked as described in L<libnbd(3)/Completion callbacks>. Note that you must ensure C<buf> is valid until the command has -completed. Other parameters behave as documented in L<nbd_pread(3)>."; +completed. Other parameters behave as documented in L<nbd_pread(3)>." +^ strict_call_description; example = Some "examples/aio-connect-read.c"; see_also = [SectionLink "Issuing asynchronous commands"; - Link "aio_pread_structured"; Link "pread"]; + Link "aio_pread_structured"; Link "pread"; + Link "set_strict_mode"]; }; "aio_pread_structured", { @@ -2145,9 +2173,11 @@ 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_pread_structured(3)>."; +Other parameters behave as documented in L<nbd_pread_structured(3)>." +^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; - Link "aio_pread"; Link "pread_structured"]; + Link "aio_pread"; Link "pread_structured"; + Link "set_strict_mode"]; }; "aio_pwrite", { diff --git a/generator/API.mli b/generator/API.mli index db978ca..41e09f5 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -100,6 +100,7 @@ and enum = { } and flags = { flag_prefix : string; (** prefix of each flag name *) + guard : string option; (** additional gating for checking valid flags *) flags : (string * int) list (** flag names and their values in C *) } and permitted_state = diff --git a/generator/C.ml b/generator/C.ml index 86d9c5c..5f68b14 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -494,7 +494,7 @@ let generate_lib_api_c () = ); (* Check parameters are valid. *) - let print_flags_check n { flag_prefix; flags } subset = + let print_flags_check n { flag_prefix; flags; guard } subset = let value = match errcode with | Some value -> value | None -> assert false in @@ -508,7 +508,10 @@ let generate_lib_api_c () = ) flags; sprintf "0x%x" !v | None -> "LIBNBD_" ^ flag_prefix ^ "_MASK" in - pr " if (unlikely ((%s & ~%s) != 0)) {\n" n mask; + let guard = match guard with + | Some value -> " && " ^ value + | None -> "" in + pr " if (unlikely ((%s & ~%s) != 0)%s) {\n" n mask guard; pr " set_error (EINVAL, \"%%s: invalid value for flag: 0x%%x\",\n"; pr " \"%s\", %s);\n" n n; pr " ret = %s;\n" value; diff --git a/lib/disconnect.c b/lib/disconnect.c index 9de1e34..f99fbd0 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -64,7 +64,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); + id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, NULL, NULL); if (id == -1) return -1; h->disconnect_request = true; diff --git a/lib/rw.c b/lib/rw.c index f49fe25..e3e80ad 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -281,7 +281,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, struct command_cb cb = { .completion = *completion }; SET_CALLBACK_TO_NULL (*completion); - return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, + return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count, buf, &cb); } @@ -350,7 +350,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, } SET_CALLBACK_TO_NULL (*completion); - return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0, + return nbd_internal_command_common (h, flags, NBD_CMD_FLUSH, 0, 0, NULL, &cb); } @@ -409,7 +409,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, } SET_CALLBACK_TO_NULL (*completion); - return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count, + return nbd_internal_command_common (h, flags, NBD_CMD_CACHE, offset, count, NULL, &cb); } diff --git a/tests/errors.c b/tests/errors.c index 0c4151a..0cfcac5 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -84,7 +84,7 @@ main (int argc, char *argv[]) * which delays responding to writes until a witness file no longer * exists. */ - const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", "sh", + const char *cmd[] = { "nbdkit", "-s", "-v", "--exit-with-parent", "sh", script, NULL }; uint32_t strict; @@ -244,7 +244,25 @@ main (int argc, char *argv[]) } check (EINVAL, "nbd_pread: "); - /* Use unknown command flags */ + /* Use unknown command flags, client-side */ + strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_FLAGS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_pread (nbd, buf, 1, 0, LIBNBD_CMD_FLAG_MASK + 1) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_pread did not fail with bogus flags\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_pread: "); + /* Use unknown command flags, server-side */ + strict &= ~(LIBNBD_STRICT_FLAGS | LIBNBD_STRICT_COMMANDS); + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_pread (nbd, buf, 1, 0, LIBNBD_CMD_FLAG_MASK + 1) != -1) { fprintf (stderr, "%s: test failed: " "nbd_pread did not fail with bogus flags\n", -- 2.28.0

The NBD protocol states that a 0-length request is undefined; we were inconsistent in that we let it through for read, write, and cache, but blocked it for trim, zero, and block_status. The NBD protocol also has documented rules on handling access beyond EOF, but we are currently wasting traffic to the server when we can give the same answer ourselves. Exposing these as two more strictness knobs gives the user finer-grained control over what behavior they want. --- lib/internal.h | 3 ++- generator/API.ml | 14 ++++++++++++++ lib/disconnect.c | 3 ++- lib/rw.c | 47 +++++++++++++++++++++++------------------------ tests/errors.c | 20 +++++++++++++++++++- 5 files changed, 60 insertions(+), 27 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 2a5147f..cde5dcd 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -432,7 +432,8 @@ extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type); extern int64_t nbd_internal_command_common (struct nbd_handle *h, uint16_t flags, uint16_t type, uint64_t offset, uint64_t count, - void *data, struct command_cb *cb); + int count_err, void *data, + struct command_cb *cb); /* socket.c */ struct socket *nbd_internal_socket_create (int fd); diff --git a/generator/API.ml b/generator/API.ml index 4cd425b..d3b1d1b 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -189,6 +189,8 @@ let strict_flags = { flags = [ "COMMANDS", 1 lsl 0; "FLAGS", 1 lsl 1; + "BOUNDS", 1 lsl 2; + "ZERO_SIZE", 1 lsl 3; ] } let allow_transport_flags = { @@ -772,6 +774,18 @@ Note that the NBD protocol only supports 16 bits of command flags, even though the libnbd API uses C<uint32_t>; bits outside of the range permitted by the protocol are always a client-side error. +=item C<LIBNBD_STRICT_BOUNDS> = 3 + +If set, this flag rejects client requests that would exceed the export +bounds without sending any traffic to the server. If clear, this flag +relies on the server to detect out-of-bounds requests. + +=item C<LIBNBD_STRICT_ZERO_SIZE> = 4 + +If set, this flag rejects client requests with length 0. If clear, +this permits zero-length requests to the server, which may produce +undefined results. + =back For convenience, the constant C<LIBNBD_STRICT_MASK> is available to diff --git a/lib/disconnect.c b/lib/disconnect.c index f99fbd0..822abc1 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -64,7 +64,8 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, NULL, NULL); + id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, 0, + NULL, NULL); if (id == -1) return -1; h->disconnect_request = true; diff --git a/lib/rw.c b/lib/rw.c index e3e80ad..5dbc947 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -168,10 +168,11 @@ nbd_unlocked_block_status (struct nbd_handle *h, 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, uint16_t flags, uint16_t type, - uint64_t offset, uint64_t count, + uint64_t offset, uint64_t count, int count_err, void *data, struct command_cb *cb) { struct command *cmd; @@ -185,6 +186,19 @@ nbd_internal_command_common (struct nbd_handle *h, goto err; } + if (count_err) { + if ((h->strict & LIBNBD_STRICT_ZERO_SIZE) && count == 0) { + set_error (EINVAL, "count cannot be 0"); + goto err; + } + + if ((h->strict & LIBNBD_STRICT_BOUNDS) && + (offset > h->exportsize || offset + count > h->exportsize)) { + set_error (count_err, "request out of bounds"); + return -1; + } + } + switch (type) { /* Commands which send or receive data are limited to MAX_REQUEST_SIZE. */ case NBD_CMD_READ: @@ -282,7 +296,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count, - buf, &cb); + EINVAL, buf, &cb); } int64_t @@ -306,7 +320,7 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, SET_CALLBACK_TO_NULL (*chunk); SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count, - buf, &cb); + EINVAL, buf, &cb); } int64_t @@ -332,7 +346,7 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count, - (void *) buf, &cb); + ENOSPC, (void *) buf, &cb); } int64_t @@ -351,7 +365,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_FLUSH, 0, 0, - NULL, &cb); + 0, NULL, &cb); } int64_t @@ -379,14 +393,9 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, } } - if (count == 0) { /* NBD protocol forbids this. */ - set_error (EINVAL, "count cannot be 0"); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_TRIM, offset, count, - NULL, &cb); + ENOSPC, NULL, &cb); } int64_t @@ -410,7 +419,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_CACHE, offset, count, - NULL, &cb); + EINVAL, NULL, &cb); } int64_t @@ -444,14 +453,9 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, } } - if (count == 0) { /* NBD protocol forbids this. */ - set_error (EINVAL, "count cannot be 0"); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_WRITE_ZEROES, offset, - count, NULL, &cb); + count, ENOSPC, NULL, &cb); } int64_t @@ -478,13 +482,8 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, } } - if (count == 0) { /* NBD protocol forbids this. */ - set_error (EINVAL, "count cannot be 0"); - return -1; - } - SET_CALLBACK_TO_NULL (*extent); SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS, offset, - count, NULL, &cb); + count, EINVAL, NULL, &cb); } diff --git a/tests/errors.c b/tests/errors.c index 0cfcac5..74bd17e 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -235,7 +235,25 @@ main (int argc, char *argv[]) } check (EINVAL, "nbd_aio_command_completed: "); - /* Read from an invalid offset */ + /* Read from an invalid offset, client-side */ + strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_BOUNDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_pread (nbd, buf, 1, -1, 0) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_pread did not fail with bogus offset\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_pread: "); + /* Read from an invalid offset, server-side */ + strict &= ~LIBNBD_STRICT_BOUNDS; + if (nbd_set_strict_mode (nbd, strict) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_pread (nbd, buf, 1, -1, 0) != -1) { fprintf (stderr, "%s: test failed: " "nbd_pread did not fail with bogus offset\n", -- 2.28.0