[PATCH libnbd 0/9] Add Enum and Flags types.
This largish series adds several new features to the generator. Enum maps to enumerated types (like enum in C). The only current use for this is replacing the nbd_set_tls (nbd, 0/1/2) parameter with LIBNBD_TLS_DISABLE, LIBNBD_TLS_ALLOW, LIBNBD_TLS_REQUIRE (and natural equivalents in other programming languages). Flags maps to any uint32_t bitmask. It is basically a non-optional, generalized variation on OFlags with some nice features. Two commits also add checking so that we check that the Enum, Flags or OFlags parameters don't contain values which are invalid at the time of compilation. This breaks new caller / old DLL at runtime (which may not be a bad thing). Note that at least patch #9 and maybe even #7 and #8 are mainly being posted for discussion. Rich.
The documentation for nbd_aio_connect_uri already refers the user to the documentation for nbd_connect_uri, so we don't need to duplicate documentation between these two calls. --- generator/generator | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/generator/generator b/generator/generator index 60e926b..78c6ca6 100755 --- a/generator/generator +++ b/generator/generator @@ -1712,12 +1712,7 @@ documented in C<nbd_connect_uri>. You can check if the connection is still connecting by calling C<nbd_aio_is_connecting>, or if it has connected to the server and completed the NBD handshake by calling C<nbd_aio_is_ready>, -on the connection. - -This call will fail if libnbd was not compiled with libxml2; you can -test whether this is the case with C<nbd_supports_uri>. Support for -URIs that require TLS will fail if libnbd was not compiled with -gnutls; you can test whether this is the case with C<nbd_supports_tls>."; +on the connection."; }; "aio_connect_unix", { -- 2.22.0
In a future commit we want to add (non-optional) Flags arg. As a step to doing this, generalize OFlags so it's not tied to just NBD_CMD_FLAG_*. This does not change the C API. It does introduce a small change to the OCaml API -- putting related flags into a submodule, basically renaming them. Note we don't provide guarantees for non-C APIs. --- generator/generator | 180 +++++++++++++++++------ ocaml/nbd-c.h | 15 -- ocaml/tests/test_405_pread_structured.ml | 6 +- ocaml/tests/test_410_pwrite.ml | 3 +- ocaml/tests/test_460_block_status.ml | 3 +- ocaml/tests/test_510_aio_pwrite.ml | 3 +- 6 files changed, 142 insertions(+), 68 deletions(-) diff --git a/generator/generator b/generator/generator index 78c6ca6..5823686 100755 --- a/generator/generator +++ b/generator/generator @@ -865,7 +865,7 @@ and arg = | UInt32 of string (* 32 bit unsigned int *) | UInt64 of string (* 64 bit unsigned int *) and optarg = -| OFlags of string (* NBD_CMD_FLAG_* flags *) +| OFlags of string * flags (* optional flags, uint32_t in C *) and ret = | RBool (* return a boolean, or error *) | RStaticString (* return a static string (must be located in @@ -890,6 +890,10 @@ and cbarg = | CBString of string (* like String *) | CBUInt of string (* like UInt *) | CBUInt64 of string (* like UInt64 *) +and flags = { + flag_prefix : string; (* prefix of each flag name *) + flags : (string * int) list (* flag names and their values in C *) +} and permitted_state = | Created (* can be called in the START state *) | Connecting (* can be called when connecting/handshaking *) @@ -906,6 +910,18 @@ 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." +(* Flags. *) +let cmd_flags = { + flag_prefix = "CMD_FLAG"; + flags = [ + "FUA", 1 lsl 0; + "NO_HOLE", 1 lsl 1; + "DF", 1 lsl 2; + "REQ_ONE", 1 lsl 3; + ] +} +let all_flags = [ cmd_flags ] + (* Calls. * * The first parameter [struct nbd_handle *nbd] is implicit. @@ -1387,7 +1403,7 @@ Returns the size in bytes of the NBD export." "pread", { default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1411,7 +1427,7 @@ protocol extensions)."; cbargs=[CBBytesIn ("subbuf", "count"); CBUInt64 "offset"; CBUInt "status"; CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1485,7 +1501,7 @@ actually obeys the flag."; "pwrite", { default_call with args = [ BytesIn ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; @@ -1505,7 +1521,7 @@ C<nbd_can_fua>)."; "shutdown", { default_call with - args = []; optargs = [ OFlags "flags" ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -1525,7 +1541,7 @@ protocol extensions)."; "flush", { default_call with - args = []; optargs = [ OFlags "flags" ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send flush command to the NBD server"; longdesc = "\ @@ -1541,7 +1557,7 @@ protocol extensions)."; "trim", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send trim command to the NBD server"; @@ -1562,7 +1578,7 @@ C<nbd_can_fua>)."; "cache", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send cache (prefetch) command to the NBD server"; @@ -1581,7 +1597,7 @@ protocol extensions)."; "zero", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send write zeroes command to the NBD server"; @@ -1610,7 +1626,7 @@ punching a hole."; CBArrayAndLen (UInt32 "entries", "nr_entries"); CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send block status command to the NBD server"; @@ -1765,7 +1781,7 @@ on the connection."; "aio_pread", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1784,7 +1800,7 @@ C<nbd_pread>."; args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "read from the NBD server, with callback on completion"; @@ -1808,7 +1824,7 @@ completed. Other parameters behave as documented in C<nbd_pread>."; CBUInt64 "offset"; CBUInt "status"; CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1831,7 +1847,7 @@ documented in C<nbd_pread_structured>."; CBMutable (Int "error")] }; Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "read from the NBD server, with callback on completion"; @@ -1849,7 +1865,7 @@ Other parameters behave as documented in C<nbd_pread_structured>."; "aio_pwrite", { default_call with args = [ BytesPersistIn ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; @@ -1868,7 +1884,7 @@ C<nbd_pwrite>."; args = [ BytesPersistIn ("buf", "count"); UInt64 "offset"; Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "write to the NBD server, with callback on completion"; @@ -1886,7 +1902,7 @@ completed. Other parameters behave as documented in C<nbd_pwrite>."; "aio_disconnect", { default_call with - args = []; optargs = [ OFlags "flags" ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -1909,7 +1925,7 @@ however, C<nbd_shutdown> will call this function if appropriate."; "aio_flush", { default_call with - args = []; optargs = [ OFlags "flags" ]; ret = RInt64; + args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send flush command to the NBD server"; longdesc = "\ @@ -1924,7 +1940,7 @@ Parameters behave as documented in C<nbd_flush>."; default_call with args = [ Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send flush command to the NBD server, with callback on completion"; @@ -1942,7 +1958,7 @@ Other parameters behave as documented in C<nbd_flush>."; "aio_trim", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send trim command to the NBD server"; @@ -1959,7 +1975,7 @@ Parameters behave as documented in C<nbd_trim>."; args = [ UInt64 "count"; UInt64 "offset"; Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send trim command to the NBD server, with callback on completion"; @@ -1977,7 +1993,7 @@ Other parameters behave as documented in C<nbd_trim>."; "aio_cache", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send cache (prefetch) command to the NBD server"; @@ -1994,7 +2010,7 @@ Parameters behave as documented in C<nbd_cache>."; args = [ UInt64 "count"; UInt64 "offset"; Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send cache (prefetch) command to the NBD server, with callback on completion"; @@ -2012,7 +2028,7 @@ Other parameters behave as documented in C<nbd_cache>."; "aio_zero", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send write zeroes command to the NBD server"; @@ -2029,7 +2045,7 @@ Parameters behave as documented in C<nbd_zero>."; args = [ UInt64 "count"; UInt64 "offset"; Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send write zeroes command to the NBD server, with callback on completion"; @@ -2052,7 +2068,7 @@ Other parameters behave as documented in C<nbd_zero>."; CBArrayAndLen (UInt32 "entries", "nr_entries"); CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send block status command to the NBD server"; @@ -2074,7 +2090,7 @@ Parameters behave as documented in C<nbd_block_status>."; CBMutable (Int "error")] }; Closure { cbname="completion"; cbargs=[CBMutable (Int "error")] } ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ]; ret = RInt64; permitted_states = [ Connected ]; shortdesc = "send block status command to the NBD server, with callback on completion"; @@ -2369,17 +2385,12 @@ C<nbd_aio_connect_uri>."; ] -(* Constants, flags, etc. *) +(* Constants, etc. *) let constants = [ "AIO_DIRECTION_READ", 1; "AIO_DIRECTION_WRITE", 2; "AIO_DIRECTION_BOTH", 3; - "CMD_FLAG_FUA", 1 lsl 0; - "CMD_FLAG_NO_HOLE", 1 lsl 1; - "CMD_FLAG_DF", 1 lsl 2; - "CMD_FLAG_REQ_ONE", 1 lsl 3; - "READ_DATA", 1; "READ_HOLE", 2; "READ_ERROR", 3; @@ -3373,7 +3384,7 @@ let rec print_arg_list ?(handle = false) ?(types = true) args optargs = if !comma then pr ", "; comma := true; match optarg with - | OFlags n -> + | OFlags (n, _) -> if types then pr "uint32_t "; pr "%s" n ) optargs; @@ -3501,7 +3512,20 @@ let generate_include_libnbd_h () = pr "\n"; pr "struct nbd_handle;\n"; pr "\n"; - List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) constants; + List.iter ( + fun { flag_prefix; flags } -> + List.iter ( + fun (flag, i) -> + let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in + pr "#define %-40s %d\n" flag i + ) flags; + pr "\n" + ) all_flags; + List.iter ( + fun (n, i) -> + let n = sprintf "LIBNBD_%s" n in + pr "#define %-40s %d\n" n i + ) constants; pr "\n"; pr "#define LIBNBD_CALLBACK_VALID 1\n"; pr "#define LIBNBD_CALLBACK_FREE 2\n"; @@ -3689,7 +3713,7 @@ let generate_lib_api_c () = ) args; List.iter ( function - | OFlags n -> pr " %s=0x%%x" n + | OFlags (n, _) -> pr " %s=0x%%x" n ) optargs; pr "\""; List.iter ( @@ -3709,7 +3733,7 @@ let generate_lib_api_c () = ) args; List.iter ( function - | OFlags n -> pr ", %s" n + | OFlags (n, _) -> pr ", %s" n ) optargs; pr ");\n" (* Print the trace when we leave a call with debugging enabled. *) @@ -4208,7 +4232,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) args; List.iter ( function - | OFlags n -> + | OFlags (n, _) -> pr " uint32_t %s_u32;\n" n; pr " unsigned int %s; /* really uint32_t */\n" n ) optargs; @@ -4236,7 +4260,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) args; List.iter ( function - | OFlags n -> pr " \"I\"" + | OFlags _ -> pr " \"I\"" ) optargs; pr "\n"; pr " \":nbd_%s\",\n" name; @@ -4260,7 +4284,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) args; List.iter ( function - | OFlags n -> pr ", &%s" n + | OFlags (n, _) -> pr ", &%s" n ) optargs; pr "))\n"; pr " return NULL;\n"; @@ -4299,7 +4323,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) args; List.iter ( function - | OFlags n -> pr " %s_u32 = %s;\n" n n + | OFlags (n, _) -> pr " %s_u32 = %s;\n" n n ) optargs; (* Call the underlying C function. *) @@ -4326,7 +4350,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) args; List.iter ( function - | OFlags n -> pr ", %s_u32" n + | OFlags (n, _) -> pr ", %s_u32" n ) optargs; pr ");\n"; if may_set_error then ( @@ -4481,6 +4505,15 @@ Error.__str__ = _str "; + List.iter ( + fun { flag_prefix; flags } -> + List.iter ( + fun (flag, i) -> + let flag = sprintf "%s_%s" flag_prefix flag in + pr "%-30s = %d\n" flag i + ) flags; + pr "\n" + ) all_flags; List.iter (fun (n, i) -> pr "%-30s = %d\n" n i) constants; List.iter ( fun (ns, ctxts) -> @@ -4545,7 +4578,7 @@ class NBD (object): let optargs = List.map ( function - | OFlags n -> n, "0" + | OFlags (n, _) -> n, "0" ) optargs in let () = let params = args @ List.map (fun (n, def) -> n ^ "=" ^ def) optargs in @@ -4628,7 +4661,7 @@ and ocaml_ret_to_string = function | RUInt -> "int" and ocaml_optarg_to_string = function - | OFlags n -> sprintf "?%s:int32 list" n + | 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 @@ -4664,7 +4697,7 @@ let ocaml_name_of_arg = function | UInt64 n -> n let ocaml_name_of_optarg = function - | OFlags n -> n + | OFlags (n, _) -> n let num_params args optargs = List.length optargs + 1 (* handle *) + List.length args @@ -4694,6 +4727,17 @@ exception Closed of string "; + List.iter ( + fun { flag_prefix; flags } -> + pr "module %s : sig\n" flag_prefix; + pr " type t =\n"; + List.iter ( + fun (flag, _) -> + pr " | %s\n" flag + ) flags; + pr "end\n"; + pr "\n" + ) all_flags; List.iter ( fun (n, _) -> pr "val %s : int32\n" (String.lowercase_ascii n) ) constants; @@ -4775,6 +4819,17 @@ let () = "; + List.iter ( + fun { flag_prefix; flags } -> + pr "module %s = struct\n" flag_prefix; + pr " type t =\n"; + List.iter ( + fun (flag, _) -> + pr " | %s\n" flag + ) flags; + pr "end\n"; + pr "\n" + ) all_flags; List.iter ( fun (n, i) -> pr "let %s = %d_l\n" (String.lowercase_ascii n) i ) constants; @@ -4819,6 +4874,33 @@ external close : t -> unit = \"nbd_internal_ocaml_nbd_close\" pr "\"nbd_internal_ocaml_nbd_%s\"\n" name ) handle_calls +let print_ocaml_flag_val { flag_prefix; flags } = + pr "/* Convert OCaml %s.t list to uint32_t bitmask. */\n" flag_prefix; + pr "static uint32_t\n"; + pr "%s_val (value v)\n" flag_prefix; + pr "{\n"; + pr " CAMLparam1 (v);\n"; + pr " int i;\n"; + pr " uint32_t r = 0;\n"; + pr "\n"; + pr " for (; v != Val_emptylist; v = Field (v, 1)) {\n"; + pr " i = Int_val (Field (v, 0));\n"; + pr " /* i is the index of the flag in the type\n"; + pr " * (eg. i = 0 => flag = %s.%s).\n" flag_prefix (fst (List.hd flags)); + pr " * Convert it to the C representation.\n"; + pr " */\n"; + pr " switch (i) {\n"; + List.iteri ( + fun i (flag, _) -> + pr " case %d: r |= LIBNBD_%s_%s; break;\n" i flag_prefix flag + ) flags; + pr " }\n"; + pr " }\n"; + pr "\n"; + pr " CAMLreturnT (uint32_t, r);\n"; + pr "}\n"; + pr "\n" + let print_ocaml_binding (name, { args; optargs; ret }) = (* Functions with a callback parameter require special handling. *) List.iter ( @@ -4978,10 +5060,11 @@ let print_ocaml_binding (name, { args; optargs; ret }) = List.iter ( function - | OFlags n -> + | OFlags (n, { flag_prefix }) -> pr " uint32_t %s;\n" n; - pr " if (%sv != Val_int (0)) /* Some flags */\n" n; - pr " %s = Flags_val (Field (%sv, 0));\n" n n; + pr " if (%sv != Val_int (0)) /* Some [ list of %s.t ] */\n" + n flag_prefix; + pr " %s = %s_val (Field (%sv, 0));\n" n flag_prefix n; pr " else /* None */\n"; pr " %s = 0;\n" n ) optargs; @@ -5126,6 +5209,7 @@ let generate_ocaml_nbd_c () = pr "#pragma GCC diagnostic ignored \"-Wmissing-prototypes\"\n"; pr "\n"; + List.iter print_ocaml_flag_val all_flags; List.iter print_ocaml_binding handle_calls end diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h index 0c12dc2..ffd51d2 100644 --- a/ocaml/nbd-c.h +++ b/ocaml/nbd-c.h @@ -99,21 +99,6 @@ Val_nbd_buffer (struct nbd_buffer b) CAMLreturn (rv); } -/* Convert flags to uint32_t. This is simple because flags are just - * lists of int32 values so we only have to add them together. - */ -static inline uint32_t -Flags_val (value v) -{ - CAMLparam1 (v); - uint32_t r = 0; - - for (; v != Val_emptylist; v = Field (v, 1)) - r += Int32_val (Field (v, 0)); - - CAMLreturnT (uint32_t, r); -} - struct callback_data { value *cb; value *data; diff --git a/ocaml/tests/test_405_pread_structured.ml b/ocaml/tests/test_405_pread_structured.ml index d226af0..e6a3b15 100644 --- a/ocaml/tests/test_405_pread_structured.ml +++ b/ocaml/tests/test_405_pread_structured.ml @@ -54,11 +54,13 @@ let () = NBD.pread_structured nbd buf 0_L (f 42); assert (buf = expected); - NBD.pread_structured nbd buf 0_L (f 42) ~flags:[NBD.cmd_flag_df]; + let flags = let open NBD.CMD_FLAG in [DF] in + + NBD.pread_structured nbd buf 0_L (f 42) ~flags; assert (buf = expected); try - NBD.pread_structured nbd buf 0_L (f 43) ~flags:[NBD.cmd_flag_df]; + NBD.pread_structured nbd buf 0_L (f 43) ~flags; assert false with NBD.Error (_, errno) -> diff --git a/ocaml/tests/test_410_pwrite.ml b/ocaml/tests/test_410_pwrite.ml index eb4fc7a..ab0feda 100644 --- a/ocaml/tests/test_410_pwrite.ml +++ b/ocaml/tests/test_410_pwrite.ml @@ -33,7 +33,8 @@ let () = let nbd = NBD.create () in NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "-v"; "file"; datafile]; - NBD.pwrite nbd buf1 0_L ~flags:[NBD.cmd_flag_fua]; + let flags = let open NBD.CMD_FLAG in [FUA] in + NBD.pwrite nbd buf1 0_L ~flags; let buf2 = Bytes.create 512 in NBD.pread nbd buf2 0_L; diff --git a/ocaml/tests/test_460_block_status.ml b/ocaml/tests/test_460_block_status.ml index df861c9..69635b4 100644 --- a/ocaml/tests/test_460_block_status.ml +++ b/ocaml/tests/test_460_block_status.ml @@ -50,7 +50,8 @@ let () = assert (!entries = [| 512_l; 3_l; 16384_l; 2_l |]); - NBD.block_status nbd 1024_L 32256_L (f 42) ~flags:[NBD.cmd_flag_req_one]; + let flags = let open NBD.CMD_FLAG in [REQ_ONE] in + NBD.block_status nbd 1024_L 32256_L (f 42) ~flags; assert (!entries = [| 512_l; 3_l |]) let () = Gc.compact () diff --git a/ocaml/tests/test_510_aio_pwrite.ml b/ocaml/tests/test_510_aio_pwrite.ml index 9a46b83..3d04e41 100644 --- a/ocaml/tests/test_510_aio_pwrite.ml +++ b/ocaml/tests/test_510_aio_pwrite.ml @@ -35,7 +35,8 @@ let () = "file"; datafile]; let buf1 = NBD.Buffer.of_bytes buf in - let cookie = NBD.aio_pwrite nbd buf1 0_L ~flags:[NBD.cmd_flag_fua] in + let flags = let open NBD.CMD_FLAG in [FUA] in + let cookie = NBD.aio_pwrite nbd buf1 0_L ~flags in while not (NBD.aio_command_completed nbd cookie) do ignore (NBD.poll nbd (-1)) done; -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
In a future commit we want to add (non-optional) Flags arg. As a step to doing this, generalize OFlags so it's not tied to just NBD_CMD_FLAG_*.
This does not change the C API.
It does introduce a small change to the OCaml API -- putting related flags into a submodule, basically renaming them. Note we don't provide guarantees for non-C APIs.
And even if we decide to provide guarantees for non-C, we're not at 1.0 yet ;)
@@ -1387,7 +1403,7 @@ Returns the size in bytes of the NBD export." "pread", { default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ];
Do we want to use this as a chance to document which flags a given API supports? For example, pread supports DF but not FUA, REQ_ONE, or MAY_TRIM. Then again, there's still a dynamic element - the API supports the DF flag for compilation, but the server must also support it (nbd_can_df) before you can use it. So any further restrictions we decide to encode in the generator rather (or in addition) to restrictions in lib/rw.c can be a later patch.
+++ b/ocaml/tests/test_405_pread_structured.ml @@ -54,11 +54,13 @@ let () = NBD.pread_structured nbd buf 0_L (f 42); assert (buf = expected);
- NBD.pread_structured nbd buf 0_L (f 42) ~flags:[NBD.cmd_flag_df]; + let flags = let open NBD.CMD_FLAG in [DF] in + + NBD.pread_structured nbd buf 0_L (f 42) ~flags;
blank line here...
+++ b/ocaml/tests/test_410_pwrite.ml @@ -33,7 +33,8 @@ let () = let nbd = NBD.create () in NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "-v"; "file"; datafile]; - NBD.pwrite nbd buf1 0_L ~flags:[NBD.cmd_flag_fua]; + let flags = let open NBD.CMD_FLAG in [FUA] in + NBD.pwrite nbd buf1 0_L ~flags;
...but not here. It doesn't affect the code, but the consistency may be worth it. This and patch 1 look fine to me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Sat, Aug 10, 2019 at 12:22:44PM -0500, Eric Blake wrote:
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
In a future commit we want to add (non-optional) Flags arg. As a step to doing this, generalize OFlags so it's not tied to just NBD_CMD_FLAG_*.
This does not change the C API.
It does introduce a small change to the OCaml API -- putting related flags into a submodule, basically renaming them. Note we don't provide guarantees for non-C APIs.
And even if we decide to provide guarantees for non-C, we're not at 1.0 yet ;)
@@ -1387,7 +1403,7 @@ Returns the size in bytes of the NBD export." "pread", { default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags "flags" ]; + optargs = [ OFlags ("flags", cmd_flags) ];
Do we want to use this as a chance to document which flags a given API supports? For example, pread supports DF but not FUA, REQ_ONE, or MAY_TRIM. Then again, there's still a dynamic element - the API supports the DF flag for compilation, but the server must also support it (nbd_can_df) before you can use it. So any further restrictions we decide to encode in the generator rather (or in addition) to restrictions in lib/rw.c can be a later patch.
It's a bit tricky to do this in the generator and especially as you say if it is dynamic. On a similar theme the checks introduced in a later patch in this series are not a subsitute for more fine-grained checks in the nbd_unlocked_* function.
+++ b/ocaml/tests/test_405_pread_structured.ml @@ -54,11 +54,13 @@ let () = NBD.pread_structured nbd buf 0_L (f 42); assert (buf = expected);
- NBD.pread_structured nbd buf 0_L (f 42) ~flags:[NBD.cmd_flag_df]; + let flags = let open NBD.CMD_FLAG in [DF] in + + NBD.pread_structured nbd buf 0_L (f 42) ~flags;
blank line here...
OK fixed in my copy.
+++ b/ocaml/tests/test_410_pwrite.ml @@ -33,7 +33,8 @@ let () = let nbd = NBD.create () in NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "-v"; "file"; datafile]; - NBD.pwrite nbd buf1 0_L ~flags:[NBD.cmd_flag_fua]; + let flags = let open NBD.CMD_FLAG in [FUA] in + NBD.pwrite nbd buf1 0_L ~flags;
...but not here. It doesn't affect the code, but the consistency may be worth it.
This and patch 1 look fine to me.
Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Previously nbd_set_tls had an integer argument which was 0 for disable, 1 for allow and 2 for require. This commit adds a proper enumerated type to describe this, defining LIBNBD_TLS_DISABLE = 0, LIBNBD_TLS_ALLOW = 1 and LIBNBD_TLS_REQUIRE = 2. (Note the C API doesn't change). In C the enumerated type is still defined and passed as an int (not as an enum). While we could define an enum type for this, there are ABI stability problems inherent in enums in C. In OCaml this is implemented as a variant type. There is no equivalent for returning an enum (eg. for nbd_get_tls). We should add that later. It won't affect the C API but would change the OCaml API. --- TODO | 2 - generator/generator | 108 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 104 insertions(+), 6 deletions(-) diff --git a/TODO b/TODO index 65c95ee..8e067c0 100644 --- a/TODO +++ b/TODO @@ -35,8 +35,6 @@ Suggested API improvements: - nbd_connect_command: allow passing char **env TLS: - - nbd_set_tls: either remove optiona (1/2) interface, or - define symbols for it - should be individual APIs for setting each TLS file (set_tls_certificates can continue to exist) - TLS should have a way to pass in PIN or password to diff --git a/generator/generator b/generator/generator index 5823686..73b8b79 100755 --- a/generator/generator +++ b/generator/generator @@ -855,6 +855,7 @@ and arg = | BytesPersistIn of string * string (* same as above, but buffer persists *) | BytesPersistOut of string * string | Closure of closure (* function pointer + void *opaque *) +| Enum of string * enum (* enum/union type, int in C *) | Int of string (* small int *) | Int64 of string (* 64 bit signed int *) | Path of string (* filename or path *) @@ -890,6 +891,10 @@ and cbarg = | CBString of string (* like String *) | CBUInt of string (* like UInt *) | CBUInt64 of string (* like UInt64 *) +and enum = { + enum_prefix : string; (* prefix of each enum variant *) + enums : (string * int) list (* enum names and their values in C *) +} and flags = { flag_prefix : string; (* prefix of each flag name *) flags : (string * int) list (* flag names and their values in C *) @@ -910,6 +915,17 @@ 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." +(* Enums. *) +let tls_enum = { + enum_prefix = "TLS"; + enums = [ + "DISABLE", 0; + "ALLOW", 1; + "REQUIRE", 2; + ] +} +let all_enums = [ tls_enum ] + (* Flags. *) let cmd_flags = { flag_prefix = "CMD_FLAG"; @@ -1020,7 +1036,7 @@ Get the export name associated with the handle."; "set_tls", { default_call with - args = [Int "tls"]; ret = RErr; + args = [Enum ("tls", tls_enum)]; ret = RErr; permitted_states = [ Created ]; shortdesc = "enable or require TLS (authentication and encryption)"; longdesc = "\ @@ -1029,12 +1045,12 @@ NBD server. The possible settings are: =over 4 -=item C<tls=0> +=item C<LIBNBD_TLS_DISABLE> Disable TLS. (The default setting, unless using C<nbd_connect_uri> with a URI that requires TLS) -=item C<tls=1> +=item C<LIBNBD_TLS_ALLOW> Enable TLS if possible. In some cases this will fall back to an unencrypted and/or unauthenticated connection if @@ -1042,7 +1058,7 @@ TLS could not be established. However some servers will drop the connection if TLS fails so fallback may not be possible. -=item C<tls=2> +=item C<LIBNBD_TLS_REQUIRE> Require an encrypted and authenticated TLS connection. Always fail to connect if the connection is not encrypted @@ -3307,6 +3323,7 @@ let rec name_of_arg = function | BytesPersistOut (n, len) -> [n; len] | Closure { cbname } -> [ sprintf "%s_callback" cbname; sprintf "%s_user_data" cbname ] +| Enum (n, _) -> [n] | Int n -> [n] | Int64 n -> [n] | Path n -> [n] @@ -3351,6 +3368,9 @@ let rec print_arg_list ?(handle = false) ?(types = true) args optargs = pr ", "; if types then pr "void *"; pr "%s_user_data" cbname + | Enum (n, _) -> + if types then pr "int "; + pr "%s" n | Int n -> if types then pr "int "; pr "%s" n @@ -3512,6 +3532,15 @@ let generate_include_libnbd_h () = pr "\n"; pr "struct nbd_handle;\n"; pr "\n"; + List.iter ( + fun { enum_prefix; enums } -> + List.iter ( + fun (enum, i) -> + let enum = sprintf "LIBNBD_%s_%s" enum_prefix enum in + pr "#define %-40s %d\n" enum i + ) enums; + pr "\n" + ) all_enums; List.iter ( fun { flag_prefix; flags } -> List.iter ( @@ -3701,6 +3730,7 @@ let generate_lib_api_c () = | BytesOut (n, count) | BytesPersistOut (n, count) -> pr " %s=<buf> %s=%%zu" n count | Closure { cbname } -> pr " %s=<fun>" cbname + | Enum (n, _) -> pr " %s=%%d" n | Int n -> pr " %s=%%d" n | Int64 n -> pr " %s=%%\" PRIi64 \"" n | SockAddrAndLen (n, len) -> pr " %s=<sockaddr> %s=%%d" n len @@ -3724,6 +3754,7 @@ let generate_lib_api_c () = | BytesOut (_, count) | BytesPersistOut (_, count) -> pr ", %s" count | Closure { cbname } -> () + | Enum (n, _) -> pr ", %s" n | Int n -> pr ", %s" n | Int64 n -> pr ", %s" n | SockAddrAndLen (_, len) -> pr ", (int) %s" len @@ -4206,6 +4237,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " struct py_aio_buffer *%s_buf;\n" n | Closure { cbname } -> pr " PyObject *%s_user_data;\n" cbname + | Enum (n, _) -> pr " int %s;\n" n | Int n -> pr " int %s;\n" n | Int64 n -> pr " int64_t %s_i64;\n" n; @@ -4248,6 +4280,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesOut (_, count) -> pr " \"n\"" | BytesPersistOut (_, count) -> pr " \"O\"" | Closure _ -> pr " \"O\"" + | Enum _ -> pr " \"i\"" | Int n -> pr " \"i\"" | Int64 n -> pr " \"L\"" | Path n -> pr " \"O&\"" @@ -4272,6 +4305,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesPersistOut (n, _) -> pr ", &%s" n | BytesOut (_, count) -> pr ", &%s" count | Closure { cbname } -> pr ", &%s_user_data" cbname + | Enum (n, _) -> pr ", &%s" n | Int n -> pr ", &%s" n | Int64 n -> pr ", &%s" n | Path n -> pr ", PyUnicode_FSConverter, &py_%s" n @@ -4306,6 +4340,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " \"callback parameter %s is not callable\");\n" cbname; pr " return NULL;\n"; pr " }\n" + | Enum _ -> () | Int _ -> () | Int64 n -> pr " %s_i64 = %s;\n" n n | Path n -> @@ -4338,6 +4373,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | Closure { cbname } -> pr ", %s_%s_wrapper" name cbname; pr ", %s_user_data" cbname + | Enum (n, _) -> pr ", %s" n | Int n -> pr ", %s" n | Int64 n -> pr ", %s_i64" n | Path n -> pr ", %s" n @@ -4373,6 +4409,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesIn _ | BytesPersistIn _ | BytesPersistOut _ | Closure _ + | Enum _ | Int _ | Int64 _ | Path _ @@ -4413,6 +4450,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesIn (n, _) -> pr " PyBuffer_Release (&%s);\n" n | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> () | Closure _ -> () + | Enum _ -> () | Int _ -> () | Int64 _ -> () | Path n -> @@ -4505,6 +4543,15 @@ Error.__str__ = _str "; + List.iter ( + fun { enum_prefix; enums } -> + List.iter ( + fun (enum, i) -> + let enum = sprintf "%s_%s" enum_prefix enum in + pr "%-30s = %d\n" enum i + ) enums; + pr "\n" + ) all_enums; List.iter ( fun { flag_prefix; flags } -> List.iter ( @@ -4565,6 +4612,7 @@ class NBD (object): | BytesPersistOut (n, _) -> n | BytesOut (_, count) -> count | Closure { cbname } -> cbname + | Enum (n, _) -> n | Int n -> n | Int64 n -> n | Path n -> n @@ -4640,6 +4688,7 @@ and ocaml_arg_to_string = function | BytesPersistOut _ -> "Buffer.t" | Closure { cbargs } -> sprintf "(%s)" (ocaml_closuredecl_to_string cbargs) + | Enum (_, { enum_prefix }) -> sprintf "%s.t" enum_prefix | Int _ -> "int" | Int64 _ -> "int64" | Path _ -> "string" @@ -4686,6 +4735,7 @@ let ocaml_name_of_arg = function | BytesPersistIn (n, len) -> n | BytesPersistOut (n, len) -> n | Closure { cbname } -> cbname + | Enum (n, _) -> n | Int n -> n | Int64 n -> n | Path n -> n @@ -4727,6 +4777,17 @@ exception Closed of string "; + List.iter ( + fun { enum_prefix; enums } -> + pr "module %s : sig\n" enum_prefix; + pr " type t =\n"; + List.iter ( + fun (enum, _) -> + pr " | %s\n" enum + ) enums; + pr "end\n"; + pr "\n" + ) all_enums; List.iter ( fun { flag_prefix; flags } -> pr "module %s : sig\n" flag_prefix; @@ -4819,6 +4880,17 @@ let () = "; + List.iter ( + fun { enum_prefix; enums } -> + pr "module %s = struct\n" enum_prefix; + pr " type t =\n"; + List.iter ( + fun (enum, _) -> + pr " | %s\n" enum + ) enums; + pr "end\n"; + pr "\n" + ) all_enums; List.iter ( fun { flag_prefix; flags } -> pr "module %s = struct\n" flag_prefix; @@ -4874,6 +4946,30 @@ external close : t -> unit = \"nbd_internal_ocaml_nbd_close\" pr "\"nbd_internal_ocaml_nbd_%s\"\n" name ) handle_calls +let print_ocaml_enum_val { enum_prefix; enums } = + pr "/* Convert OCaml %s.t to int. */\n" enum_prefix; + pr "static int\n"; + pr "%s_val (value v)\n" enum_prefix; + pr "{\n"; + pr " CAMLparam1 (v);\n"; + pr " int i, r = 0;\n"; + pr "\n"; + pr " i = Int_val (Field (v, 0));\n"; + pr " /* i is the index of the enum in the type\n"; + pr " * (eg. i = 0 => enum = %s.%s).\n" enum_prefix (fst (List.hd enums)); + pr " * Convert it to the C representation.\n"; + pr " */\n"; + pr " switch (i) {\n"; + List.iteri ( + fun i (enum, _) -> + pr " case %d: r = LIBNBD_%s_%s; break;\n" i enum_prefix enum + ) enums; + pr " }\n"; + pr "\n"; + pr " CAMLreturnT (int, r);\n"; + pr "}\n"; + pr "\n" + let print_ocaml_flag_val { flag_prefix; flags } = pr "/* Convert OCaml %s.t list to uint32_t bitmask. */\n" flag_prefix; pr "static uint32_t\n"; @@ -5097,6 +5193,8 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " *%s_user_data = %sv;\n" cbname cbname; pr " caml_register_generational_global_root (%s_user_data);\n" cbname; pr " const void *%s_callback = %s_%s_wrapper;\n" cbname name cbname + | Enum (n, { enum_prefix }) -> + pr " int %s = %s_val (%sv);\n" n enum_prefix n | Int n -> pr " int %s = Int_val (%sv);\n" n n | Int64 n -> @@ -5154,6 +5252,7 @@ let print_ocaml_binding (name, { args; optargs; ret }) = | BytesOut _ | BytesPersistOut _ | Closure _ + | Enum _ | Int _ | Int64 _ | Path _ @@ -5209,6 +5308,7 @@ let generate_ocaml_nbd_c () = pr "#pragma GCC diagnostic ignored \"-Wmissing-prototypes\"\n"; pr "\n"; + List.iter print_ocaml_enum_val all_enums; List.iter print_ocaml_flag_val all_flags; List.iter print_ocaml_binding handle_calls end -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
Previously nbd_set_tls had an integer argument which was 0 for disable, 1 for allow and 2 for require. This commit adds a proper enumerated type to describe this, defining LIBNBD_TLS_DISABLE = 0, LIBNBD_TLS_ALLOW = 1 and LIBNBD_TLS_REQUIRE = 2. (Note the C API doesn't change).
In C the enumerated type is still defined and passed as an int (not as an enum). While we could define an enum type for this, there are ABI stability problems inherent in enums in C.
In OCaml this is implemented as a variant type.
There is no equivalent for returning an enum (eg. for nbd_get_tls). We should add that later. It won't affect the C API but would change the OCaml API. --- TODO | 2 - generator/generator | 108 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 104 insertions(+), 6 deletions(-)
diff --git a/TODO b/TODO index 65c95ee..8e067c0 100644 --- a/TODO +++ b/TODO @@ -35,8 +35,6 @@ Suggested API improvements: - nbd_connect_command: allow passing char **env
TLS: - - nbd_set_tls: either remove optiona (1/2) interface, or - define symbols for it
Nice fix of a typo while at it :)
@@ -1029,12 +1045,12 @@ NBD server. The possible settings are:
=over 4
-=item C<tls=0> +=item C<LIBNBD_TLS_DISABLE>
Disable TLS. (The default setting, unless using C<nbd_connect_uri> with a URI that requires TLS)
-=item C<tls=1> +=item C<LIBNBD_TLS_ALLOW>
Enable TLS if possible. In some cases this will fall back to an unencrypted and/or unauthenticated connection if
While touching this, we should also call out that this is a security risk if there is any possibility of a man-in-the-middle attack. Otherwise looks good. Should we also expose an enum type for the pread_structured callback function? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
This is the same, but now we have a symbol for it. --- docs/libnbd.pod | 6 ++++-- interop/interop.c | 2 +- lib/connect.c | 2 +- tests/aio-parallel-load.c | 2 +- tests/aio-parallel.c | 2 +- tests/connect-tls.c | 2 +- tests/synch-parallel.c | 2 +- 7 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/libnbd.pod b/docs/libnbd.pod index b42d000..01964de 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -522,8 +522,10 @@ and servers. Libnbd defaults to TLS I<disabled> for maximum interoperability. To enable it on a handle you must call C<nbd_set_tls> before connecting: - nbd_set_tls (nbd, 1); // to allow TLS, but fall back to unencrypted - nbd_set_tls (nbd, 2); // to require TLS, and fail otherwise + // to allow TLS, but fall back to unencrypted + nbd_set_tls (nbd, LIBNBD_TLS_ALLOW); + // to require TLS, and fail otherwise + nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE); It may also be necessary to verify that the server’s identity is correct. For some servers it may be necessary to verify to the server diff --git a/interop/interop.c b/interop/interop.c index a3973db..662d871 100644 --- a/interop/interop.c +++ b/interop/interop.c @@ -77,7 +77,7 @@ main (int argc, char *argv[]) fprintf (stderr, "skip: compiled without TLS support\n"); exit (77); } - if (nbd_set_tls (nbd, 2) == -1) { + if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/lib/connect.c b/lib/connect.c index 5e760c6..f98bcdb 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -292,7 +292,7 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) } /* TLS */ - if (tls && nbd_unlocked_set_tls (h, 2) == -1) + if (tls && nbd_unlocked_set_tls (h, LIBNBD_TLS_REQUIRE) == -1) goto cleanup; /* XXX If uri->query_raw includes TLS parameters, we should call * nbd_unlocked_set_tls_* to match... diff --git a/tests/aio-parallel-load.c b/tests/aio-parallel-load.c index f4ba635..614c22b 100644 --- a/tests/aio-parallel-load.c +++ b/tests/aio-parallel-load.c @@ -207,7 +207,7 @@ start_thread (void *arg) /* Require TLS on the handle and fail if not available or if the * handshake fails. */ - if (nbd_set_tls (nbd, 2) == -1) { + if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/aio-parallel.c b/tests/aio-parallel.c index f8d4891..b6a0682 100644 --- a/tests/aio-parallel.c +++ b/tests/aio-parallel.c @@ -220,7 +220,7 @@ start_thread (void *arg) /* Require TLS on the handle and fail if not available or if the * handshake fails. */ - if (nbd_set_tls (nbd, 2) == -1) { + if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/connect-tls.c b/tests/connect-tls.c index be2ef32..0666d12 100644 --- a/tests/connect-tls.c +++ b/tests/connect-tls.c @@ -46,7 +46,7 @@ main (int argc, char *argv[]) /* Require TLS on the handle and fail if not available or if the * handshake fails. */ - if (nbd_set_tls (nbd, 2) == -1) { + if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/synch-parallel.c b/tests/synch-parallel.c index 40df85d..830d12a 100644 --- a/tests/synch-parallel.c +++ b/tests/synch-parallel.c @@ -188,7 +188,7 @@ start_thread (void *arg) /* Require TLS on the handle and fail if not available or if the * handshake fails. */ - if (nbd_set_tls (nbd, 2) == -1) { + if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
This is the same, but now we have a symbol for it. --- docs/libnbd.pod | 6 ++++-- interop/interop.c | 2 +- lib/connect.c | 2 +- tests/aio-parallel-load.c | 2 +- tests/aio-parallel.c | 2 +- tests/connect-tls.c | 2 +- tests/synch-parallel.c | 2 +- 7 files changed, 10 insertions(+), 8 deletions(-)
ACK; could also be squashed with the previous one.
diff --git a/docs/libnbd.pod b/docs/libnbd.pod index b42d000..01964de 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -522,8 +522,10 @@ and servers. Libnbd defaults to TLS I<disabled> for maximum interoperability. To enable it on a handle you must call C<nbd_set_tls> before connecting:
- nbd_set_tls (nbd, 1); // to allow TLS, but fall back to unencrypted - nbd_set_tls (nbd, 2); // to require TLS, and fail otherwise + // to allow TLS, but fall back to unencrypted + nbd_set_tls (nbd, LIBNBD_TLS_ALLOW);
Again, probably worth calling attention to the fact that this one is a potential security risk for MitM and should be avoided if that is a concern. Maybe as simple as adding: // warning: avoid this mode if man-in-the-middle attacks are a concern
+ // to require TLS, and fail otherwise + nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE);
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
In the generated wrapper code this adds checks for all Enum parameters. Since only nbd_set_tls uses an Enum parameter, the only extra code generated by this change is: int nbd_set_tls (struct nbd_handle *h, int tls) { // ... switch (tls) { case LIBNBD_TLS_DISABLE: case LIBNBD_TLS_ALLOW: case LIBNBD_TLS_REQUIRE: break; default: set_error (EINVAL, "%s: invalid value for parameter: %d", "tls", tls); ret = -1; goto out; } This doesn't change the C API, but previously this parameter was not checked. So programs using this API which previously happened to work would now get an error. --- generator/generator | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/generator/generator b/generator/generator index 73b8b79..2b37cea 100755 --- a/generator/generator +++ b/generator/generator @@ -3686,6 +3686,23 @@ let generate_lib_api_c () = (* Check parameters are valid. *) List.iter ( function + | Enum (n, { enum_prefix; enums }) -> + let value = match errcode with + | Some value -> value + | None -> assert false in + pr " switch (%s) {\n" n; + List.iter ( + fun (enum, _) -> + pr " case LIBNBD_%s_%s:\n" enum_prefix enum + ) enums; + pr " break;\n"; + pr " default:\n"; + pr " set_error (EINVAL, \"%%s: invalid value for parameter: %%d\",\n"; + pr " \"%s\", %s);\n" n n; + pr " ret = %s;\n" value; + pr " goto out;\n"; + pr " }\n"; + need_out_label := true | String n -> let value = match errcode with | Some value -> value -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
In the generated wrapper code this adds checks for all Enum parameters. Since only nbd_set_tls uses an Enum parameter, the only extra code generated by this change is:
int nbd_set_tls (struct nbd_handle *h, int tls) { // ... switch (tls) { case LIBNBD_TLS_DISABLE: case LIBNBD_TLS_ALLOW: case LIBNBD_TLS_REQUIRE: break; default: set_error (EINVAL, "%s: invalid value for parameter: %d", "tls", tls); ret = -1; goto out; }
This doesn't change the C API, but previously this parameter was not checked. So programs using this API which previously happened to work would now get an error.
programs using this API with an out-of-bounds value (it doesn't change programs using it as documented)
--- generator/generator | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
This works just like OFlags but is a non-optional argument. --- generator/generator | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/generator/generator b/generator/generator index 2b37cea..96d1148 100755 --- a/generator/generator +++ b/generator/generator @@ -856,6 +856,7 @@ and arg = | BytesPersistOut of string * string | Closure of closure (* function pointer + void *opaque *) | Enum of string * enum (* enum/union type, int in C *) +| Flags of string * flags (* flags, uint32_t in C *) | Int of string (* small int *) | Int64 of string (* 64 bit signed int *) | Path of string (* filename or path *) @@ -3324,6 +3325,7 @@ let rec name_of_arg = function | Closure { cbname } -> [ sprintf "%s_callback" cbname; sprintf "%s_user_data" cbname ] | Enum (n, _) -> [n] +| Flags (n, _) -> [n] | Int n -> [n] | Int64 n -> [n] | Path n -> [n] @@ -3371,6 +3373,9 @@ let rec print_arg_list ?(handle = false) ?(types = true) args optargs = | Enum (n, _) -> if types then pr "int "; pr "%s" n + | Flags (n, _) -> + if types then pr "uint32_t "; + pr "%s" n | Int n -> if types then pr "int "; pr "%s" n @@ -3748,6 +3753,7 @@ let generate_lib_api_c () = | BytesPersistOut (n, count) -> pr " %s=<buf> %s=%%zu" n count | Closure { cbname } -> pr " %s=<fun>" cbname | Enum (n, _) -> pr " %s=%%d" n + | Flags (n, _) -> pr " %s=0x%%x" n | Int n -> pr " %s=%%d" n | Int64 n -> pr " %s=%%\" PRIi64 \"" n | SockAddrAndLen (n, len) -> pr " %s=<sockaddr> %s=%%d" n len @@ -3772,6 +3778,7 @@ let generate_lib_api_c () = | BytesPersistOut (_, count) -> pr ", %s" count | Closure { cbname } -> () | Enum (n, _) -> pr ", %s" n + | Flags (n, _) -> pr ", %s" n | Int n -> pr ", %s" n | Int64 n -> pr ", %s" n | SockAddrAndLen (_, len) -> pr ", (int) %s" len @@ -4255,6 +4262,9 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | Closure { cbname } -> pr " PyObject *%s_user_data;\n" cbname | Enum (n, _) -> pr " int %s;\n" n + | Flags (n, _) -> + pr " uint32_t %s_u32;\n" n; + pr " unsigned int %s; /* really uint32_t */\n" n | Int n -> pr " int %s;\n" n | Int64 n -> pr " int64_t %s_i64;\n" n; @@ -4298,6 +4308,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesPersistOut (_, count) -> pr " \"O\"" | Closure _ -> pr " \"O\"" | Enum _ -> pr " \"i\"" + | Flags _ -> pr " \"I\"" | Int n -> pr " \"i\"" | Int64 n -> pr " \"L\"" | Path n -> pr " \"O&\"" @@ -4323,6 +4334,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesOut (_, count) -> pr ", &%s" count | Closure { cbname } -> pr ", &%s_user_data" cbname | Enum (n, _) -> pr ", &%s" n + | Flags (n, _) -> pr ", &%s" n | Int n -> pr ", &%s" n | Int64 n -> pr ", &%s" n | Path n -> pr ", PyUnicode_FSConverter, &py_%s" n @@ -4358,6 +4370,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " return NULL;\n"; pr " }\n" | Enum _ -> () + | Flags (n, _) -> pr " %s_u32 = %s;\n" n n | Int _ -> () | Int64 n -> pr " %s_i64 = %s;\n" n n | Path n -> @@ -4391,6 +4404,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr ", %s_%s_wrapper" name cbname; pr ", %s_user_data" cbname | Enum (n, _) -> pr ", %s" n + | Flags (n, _) -> pr ", %s_u32" n | Int n -> pr ", %s" n | Int64 n -> pr ", %s_i64" n | Path n -> pr ", %s" n @@ -4427,6 +4441,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesPersistIn _ | BytesPersistOut _ | Closure _ | Enum _ + | Flags _ | Int _ | Int64 _ | Path _ @@ -4468,6 +4483,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> () | Closure _ -> () | Enum _ -> () + | Flags _ -> () | Int _ -> () | Int64 _ -> () | Path n -> @@ -4630,6 +4646,7 @@ class NBD (object): | BytesOut (_, count) -> count | Closure { cbname } -> cbname | Enum (n, _) -> n + | Flags (n, _) -> n | Int n -> n | Int64 n -> n | Path n -> n @@ -4706,6 +4723,7 @@ and ocaml_arg_to_string = function | Closure { cbargs } -> sprintf "(%s)" (ocaml_closuredecl_to_string cbargs) | Enum (_, { enum_prefix }) -> sprintf "%s.t" enum_prefix + | Flags (_, { flag_prefix }) -> sprintf "%s.t" flag_prefix | Int _ -> "int" | Int64 _ -> "int64" | Path _ -> "string" @@ -4753,6 +4771,7 @@ let ocaml_name_of_arg = function | BytesPersistOut (n, len) -> n | Closure { cbname } -> cbname | Enum (n, _) -> n + | Flags (n, _) -> n | Int n -> n | Int64 n -> n | Path n -> n @@ -5212,6 +5231,8 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " const void *%s_callback = %s_%s_wrapper;\n" cbname name cbname | Enum (n, { enum_prefix }) -> pr " int %s = %s_val (%sv);\n" n enum_prefix n + | Flags (n, { flag_prefix }) -> + pr " uint32_t %s = %s_val (%sv);\n" n flag_prefix n | Int n -> pr " int %s = Int_val (%sv);\n" n n | Int64 n -> @@ -5270,6 +5291,7 @@ let print_ocaml_binding (name, { args; optargs; ret }) = | BytesPersistOut _ | Closure _ | Enum _ + | Flags _ | Int _ | Int64 _ | Path _ -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
This works just like OFlags but is a non-optional argument. --- generator/generator | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
Presumably used in an upcoming patch; no impact in the meantime. ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Generate checks that no unknown (at the time of compilation) flags are passed to Flags or OFlags parameters. --- generator/generator | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/generator/generator b/generator/generator index 96d1148..a6aea26 100755 --- a/generator/generator +++ b/generator/generator @@ -3689,6 +3689,19 @@ let generate_lib_api_c () = ); (* Check parameters are valid. *) + let print_flags_check n { flag_prefix; flags } = + 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 ((%s & ~%d) != 0) {\n" n mask; + pr " set_error (EINVAL, \"%%s: invalid value for flag: %%d\",\n"; + pr " \"%s\", %s);\n" n n; + pr " ret = %s;\n" value; + pr " goto out;\n"; + pr " }\n"; + need_out_label := true + in List.iter ( function | Enum (n, { enum_prefix; enums }) -> @@ -3708,6 +3721,8 @@ let generate_lib_api_c () = pr " goto out;\n"; pr " }\n"; need_out_label := true + | Flags (n, flags) -> + print_flags_check n flags | String n -> let value = match errcode with | Some value -> value @@ -3720,6 +3735,11 @@ let generate_lib_api_c () = need_out_label := true | _ -> () ) args; + List.iter ( + function + | OFlags (n, flags) -> + print_flags_check n flags + ) optargs; (* Make the call. *) pr " ret = nbd_unlocked_%s " name; -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
Generate checks that no unknown (at the time of compilation) flags are passed to Flags or OFlags parameters. --- generator/generator | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
We may still want to introduce a testing mode where you can use libnbd to drive unknown flags to a server that understands them, for experimenting with protocol extensions not yet included in the NBD specification. But if we ever do add such a mode, it shouldn't be hard to make this sanity checking conditional on that mode.
diff --git a/generator/generator b/generator/generator index 96d1148..a6aea26 100755 --- a/generator/generator +++ b/generator/generator @@ -3689,6 +3689,19 @@ let generate_lib_api_c () = );
(* Check parameters are valid. *) + let print_flags_check n { flag_prefix; flags } = + 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 ((%s & ~%d) != 0) {\n" n mask; + pr " set_error (EINVAL, \"%%s: invalid value for flag: %%d\",\n"; + pr " \"%s\", %s);\n" n n; + pr " ret = %s;\n" value; + pr " goto out;\n";
Some of the checks in lib/rw.c are now unreachable with this in place. Is it worth simplifying that? (But not all of them - there are still checks that depend on runtime values, such as nbd_pread accepting _DF only if the server advertises it after the client requests structured replies). Also, this lets us pass all four existing command flags to all commands that accept an OFlags parameter, even though none of the commands accept all flags at once - the real protection being added here is the check for completely unrecognized flags. But the changes here look reasonable. ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Sat, Aug 10, 2019 at 04:38:20PM -0500, Eric Blake wrote:
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
Generate checks that no unknown (at the time of compilation) flags are passed to Flags or OFlags parameters. --- generator/generator | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
We may still want to introduce a testing mode where you can use libnbd to drive unknown flags to a server that understands them, for experimenting with protocol extensions not yet included in the NBD specification. But if we ever do add such a mode, it shouldn't be hard to make this sanity checking conditional on that mode.
diff --git a/generator/generator b/generator/generator index 96d1148..a6aea26 100755 --- a/generator/generator +++ b/generator/generator @@ -3689,6 +3689,19 @@ let generate_lib_api_c () = );
(* Check parameters are valid. *) + let print_flags_check n { flag_prefix; flags } = + 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 ((%s & ~%d) != 0) {\n" n mask; + pr " set_error (EINVAL, \"%%s: invalid value for flag: %%d\",\n"; + pr " \"%s\", %s);\n" n n; + pr " ret = %s;\n" value; + pr " goto out;\n";
Some of the checks in lib/rw.c are now unreachable with this in place. Is it worth simplifying that? (But not all of them - there are still checks that depend on runtime values, such as nbd_pread accepting _DF only if the server advertises it after the client requests structured replies). Also, this lets us pass all four existing command flags to all commands that accept an OFlags parameter, even though none of the commands accept all flags at once - the real protection being added here is the check for completely unrecognized flags.
Right - this doesn't replace fine-grained checks in the functions themselves. I'll have a look at lib/rw.c and see what checks might be removed. Rich.
But the changes here look reasonable. ACK.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Sun, Aug 11, 2019 at 09:26:14AM +0100, Richard W.M. Jones wrote:
On Sat, Aug 10, 2019 at 04:38:20PM -0500, Eric Blake wrote:
Some of the checks in lib/rw.c are now unreachable with this in place. Is it worth simplifying that? (But not all of them - there are still checks that depend on runtime values, such as nbd_pread accepting _DF only if the server advertises it after the client requests structured replies). Also, this lets us pass all four existing command flags to all commands that accept an OFlags parameter, even though none of the commands accept all flags at once - the real protection being added here is the check for completely unrecognized flags.
Right - this doesn't replace fine-grained checks in the functions themselves. I'll have a look at lib/rw.c and see what checks might be removed.
Hmm - I think all the flag tests in lib/rw.c are still necessary ..? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 8/11/19 3:30 AM, Richard W.M. Jones wrote:
On Sun, Aug 11, 2019 at 09:26:14AM +0100, Richard W.M. Jones wrote:
On Sat, Aug 10, 2019 at 04:38:20PM -0500, Eric Blake wrote:
Some of the checks in lib/rw.c are now unreachable with this in place. Is it worth simplifying that? (But not all of them - there are still checks that depend on runtime values, such as nbd_pread accepting _DF only if the server advertises it after the client requests structured replies). Also, this lets us pass all four existing command flags to all commands that accept an OFlags parameter, even though none of the commands accept all flags at once - the real protection being added here is the check for completely unrecognized flags.
Right - this doesn't replace fine-grained checks in the functions themselves. I'll have a look at lib/rw.c and see what checks might be removed.
Hmm - I think all the flag tests in lib/rw.c are still necessary ..?
Yeah, it looks like that is the case for the current code. We have the common pattern of checking that only flags valid for the particular command are present, followed by runtime checks that each flag requested can actually be sent. The first check could be moved into the generator, but only if we let the generator track the subset of flags valid for a given command; but it's just as easy to keep that first check in rw.c and have the generator do the simpler check for no flags that are completely unknown. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Optionally generate a mask which sets all currently known flags. As written this commit does nothing, but it makes more sense with currently proposed changes to nbd_connect_uri. --- generator/generator | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/generator/generator b/generator/generator index a6aea26..fe24738 100755 --- a/generator/generator +++ b/generator/generator @@ -898,6 +898,7 @@ and enum = { } and flags = { flag_prefix : string; (* prefix of each flag name *) + all_flags_bitmask : bool; (* if true, generate a bitmask of all flags *) flags : (string * int) list (* flag names and their values in C *) } and permitted_state = @@ -930,6 +931,7 @@ let all_enums = [ tls_enum ] (* Flags. *) let cmd_flags = { flag_prefix = "CMD_FLAG"; + all_flags_bitmask = false; flags = [ "FUA", 1 lsl 0; "NO_HOLE", 1 lsl 1; @@ -3547,13 +3549,18 @@ let generate_include_libnbd_h () = pr "\n" ) all_enums; List.iter ( - fun { flag_prefix; flags } -> + fun { flag_prefix; all_flags_bitmask; flags } -> List.iter ( fun (flag, i) -> let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in pr "#define %-40s %d\n" flag i ) flags; - pr "\n" + if all_flags_bitmask then ( + let all = List.fold_left (lor) 0 (List.map snd flags) in + let n = sprintf "LIBNBD_%s_ALL" flag_prefix in + pr "#define %-40s %d\n" n all; + ); + pr "\n"; ) all_flags; List.iter ( fun (n, i) -> @@ -4606,12 +4613,17 @@ Error.__str__ = _str pr "\n" ) all_enums; List.iter ( - fun { flag_prefix; flags } -> + fun { flag_prefix; all_flags_bitmask; flags } -> List.iter ( fun (flag, i) -> let flag = sprintf "%s_%s" flag_prefix flag in pr "%-30s = %d\n" flag i ) flags; + if all_flags_bitmask then ( + let all = List.fold_left (lor) 0 (List.map snd flags) in + let n = sprintf "%s_ALL" flag_prefix in + pr "%-30s = %d\n" n all; + ); pr "\n" ) all_flags; List.iter (fun (n, i) -> pr "%-30s = %d\n" n i) constants; @@ -4845,13 +4857,15 @@ exception Closed of string pr "\n" ) all_enums; List.iter ( - fun { flag_prefix; flags } -> + fun { flag_prefix; all_flags_bitmask; flags } -> pr "module %s : sig\n" flag_prefix; pr " type t =\n"; List.iter ( fun (flag, _) -> pr " | %s\n" flag ) flags; + if all_flags_bitmask then + pr " val all : t list\n"; pr "end\n"; pr "\n" ) all_flags; @@ -4948,13 +4962,15 @@ let () = pr "\n" ) all_enums; List.iter ( - fun { flag_prefix; flags } -> + fun { flag_prefix; all_flags_bitmask; flags } -> pr "module %s = struct\n" flag_prefix; pr " type t =\n"; List.iter ( fun (flag, _) -> pr " | %s\n" flag ) flags; + if all_flags_bitmask then + pr " let all = [%s]\n" (String.concat ";" (List.map fst flags)); pr "end\n"; pr "\n" ) all_flags; -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
Optionally generate a mask which sets all currently known flags. As written this commit does nothing, but it makes more sense with currently proposed changes to nbd_connect_uri. --- generator/generator | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)
ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Add an extra parameter to nbd_connect_uri to control what URIs are permitted, in case the caller wants to pass in user-controlled URIs but have some control over who/what/how the connection happens. For example: nbd_connect_uri (nbd, "nbd://localhost", LIBNBD_CONNECT_URI_REQUIRE_TLS) => error: URI must specify an encrypted connection: Permission denied This obviously breaks the existing C API. --- TODO | 1 - docs/libnbd.pod | 2 +- examples/batched-read-write.c | 2 +- examples/strict-structured-reads.c | 2 +- examples/threaded-reads-and-writes.c | 5 +-- generator/generator | 50 +++++++++++++++++++++++++--- lib/connect.c | 33 ++++++++++++++++-- tests/aio-parallel-load.c | 3 +- tests/connect-uri-tcp.c | 2 +- tests/connect-uri-unix.c | 3 +- 10 files changed, 87 insertions(+), 16 deletions(-) diff --git a/TODO b/TODO index 8e067c0..1dd64d7 100644 --- a/TODO +++ b/TODO @@ -31,7 +31,6 @@ Suggested API improvements: connecting: - nbd_connect_tcp: allow control over whether IPv4 or IPv6 is desired - - nbd_connect_uri: allow control over which features are enabled - nbd_connect_command: allow passing char **env TLS: diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 01964de..7018621 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -364,7 +364,7 @@ when it is available. To connect to a URI via the high level API, use: - nbd_connect_uri (nbd, "nbd://example.com/"); + nbd_connect_uri (nbd, "nbd://example.com/", LIBNBD_CONNECT_URI_ALL); This feature is implemented by calling other libnbd APIs to set up the export name, TLS parameters, and finally connect over a Unix domain diff --git a/examples/batched-read-write.c b/examples/batched-read-write.c index d39a1e5..1b876da 100644 --- a/examples/batched-read-write.c +++ b/examples/batched-read-write.c @@ -143,7 +143,7 @@ main (int argc, char *argv[]) /* Connect synchronously as this is simpler. */ if (argc == 2) { if (strstr (argv[1], "://")) { - if (nbd_connect_uri (nbd, argv[1]) == -1) { + if (nbd_connect_uri (nbd, argv[1], LIBNBD_CONNECT_URI_ALL) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index b3880b7..fcf1186 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -197,7 +197,7 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } if (strstr (argv[1], "://")) { - if (nbd_connect_uri (nbd, argv[1]) == -1) { + if (nbd_connect_uri (nbd, argv[1], LIBNBD_CONNECT_URI_ALL) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/examples/threaded-reads-and-writes.c b/examples/threaded-reads-and-writes.c index 85d6e42..864e2f4 100644 --- a/examples/threaded-reads-and-writes.c +++ b/examples/threaded-reads-and-writes.c @@ -89,7 +89,7 @@ main (int argc, char *argv[]) /* Connect first to check if the server supports writes and multi-conn. */ if (argc == 2) { if (strstr (argv[1], "://")) { - if (nbd_connect_uri (nbd, argv[1]) == -1) { + if (nbd_connect_uri (nbd, argv[1], LIBNBD_CONNECT_URI_ALL) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } @@ -211,7 +211,8 @@ start_thread (void *arg) if (status->argc == 2) { if (strstr (status->argv[1], "://")) { - if (nbd_connect_uri (nbd, status->argv[1]) == -1) { + if (nbd_connect_uri (nbd, status->argv[1], + LIBNBD_CONNECT_URI_ALL) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/generator/generator b/generator/generator index fe24738..5f5523a 100755 --- a/generator/generator +++ b/generator/generator @@ -939,7 +939,17 @@ let cmd_flags = { "REQ_ONE", 1 lsl 3; ] } -let all_flags = [ cmd_flags ] +let connect_uri_allow_flags = { + flag_prefix = "CONNECT_URI"; + all_flags_bitmask = true; + flags = [ + "ALLOW_TCP", 1 lsl 0; + "ALLOW_UNIX", 1 lsl 1; + "ALLOW_TLS", 1 lsl 2; + "REQUIRE_TLS", 1 lsl 3; + ] +} +let all_flags = [ cmd_flags; connect_uri_allow_flags ] (* Calls. * @@ -1225,7 +1235,8 @@ C<\"qemu:dirty-bitmap:...\"> for qemu-nbd "connect_uri", { default_call with - args = [ String "uri" ]; ret = RErr; + args = [ String "uri"; Flags ("allow", connect_uri_allow_flags) ]; + ret = RErr; permitted_states = [ Created ]; shortdesc = "connect to NBD URI"; longdesc = "\ @@ -1238,7 +1249,37 @@ the connection has been made. This call will fail if libnbd was not compiled with libxml2; you can test whether this is the case with C<nbd_supports_uri>. Support for URIs that require TLS will fail if libnbd was not compiled with -gnutls; you can test whether this is the case with C<nbd_supports_tls>."; +gnutls; you can test whether this is the case with C<nbd_supports_tls>. + +The C<allow> parameter lets you choose which NBD URI features +are enabled, in case for example you don't want to allow +remote connections. Currently defined flags are: + +=over 4 + +=item C<LIBNBD_CONNECT_URI_ALLOW_TCP> + +Allow TCP sockets. + +=item C<LIBNBD_CONNECT_URI_ALLOW_UNIX> + +Allow Unix domain sockets. + +=item C<LIBNBD_CONNECT_URI_ALLOW_TLS> + +Allow TLS encryption. + +=item C<LIBNBD_CONNECT_URI_REQUIRE_TLS> + +Require TLS encryption. + +=item C<LIBNBD_CONNECT_URI_ALL> + +Enables all features which are defined at the time that +the program is compiled. Later features added to libnbd +will not be allowed unless you recompile your program. + +=back"; }; "connect_unix", { @@ -1737,7 +1778,8 @@ on the connection."; "aio_connect_uri", { default_call with - args = [ String "uri" ]; ret = RErr; + args = [ String "uri"; Flags ("allow", connect_uri_allow_flags) ]; + ret = RErr; permitted_states = [ Created ]; shortdesc = "connect to an NBD URI"; longdesc = "\ diff --git a/lib/connect.c b/lib/connect.c index f98bcdb..a4c594e 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -73,9 +73,10 @@ wait_until_connected (struct nbd_handle *h) /* Connect to an NBD URI. */ int -nbd_unlocked_connect_uri (struct nbd_handle *h, const char *uri) +nbd_unlocked_connect_uri (struct nbd_handle *h, + const char *uri, uint32_t allow) { - if (nbd_unlocked_aio_connect_uri (h, uri) == -1) + if (nbd_unlocked_aio_connect_uri (h, uri, allow) == -1) return -1; return wait_until_connected (h); @@ -228,7 +229,8 @@ error: #endif int -nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) +nbd_unlocked_aio_connect_uri (struct nbd_handle *h, + const char *raw_uri, uint32_t allow) { #ifdef HAVE_LIBXML2 xmlURIPtr uri = NULL; @@ -276,6 +278,31 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) goto cleanup; } + /* If the user specified the REQUIRE_TLS flag, we assume they must + * also mean to ALLOW_TLS. + */ + if ((allow & LIBNBD_CONNECT_URI_REQUIRE_TLS) != 0) + allow |= LIBNBD_CONNECT_URI_ALLOW_TLS; + + /* Check allow flags. */ + if (tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_TCP)) { + set_error (EPERM, "TCP URIs are not allowed"); + goto cleanup; + } + if (!tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_UNIX)) { + set_error (EPERM, "Unix domain socket URIs are not allowed"); + goto cleanup; + } + if (tls && !(allow & LIBNBD_CONNECT_URI_ALLOW_TLS)) { + set_error (EPERM, "TLS encrypted URIs are not allowed"); + goto cleanup; + } + if (!tls && (allow & LIBNBD_CONNECT_URI_REQUIRE_TLS)) { + set_error (EPERM, "URI must specify an encrypted connection " + "(use nbds: or nbds+unix:)"); + goto cleanup; + } + /* Insist on the scheme://[authority][/absname][?queries] form. */ if (strncmp (raw_uri + strlen (uri->scheme), "://", 3)) { set_error (EINVAL, "URI must begin with '%s://'", uri->scheme); diff --git a/tests/aio-parallel-load.c b/tests/aio-parallel-load.c index 614c22b..16c2aa2 100644 --- a/tests/aio-parallel-load.c +++ b/tests/aio-parallel-load.c @@ -220,7 +220,8 @@ start_thread (void *arg) /* Connect to nbdkit. */ if (strstr (connection, "://")) { - if (nbd_connect_uri (nbd, connection) == -1) { + if (nbd_connect_uri (nbd, connection, + LIBNBD_CONNECT_URI_ALLOW_UNIX) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/connect-uri-tcp.c b/tests/connect-uri-tcp.c index 0c5c1df..b33d94c 100644 --- a/tests/connect-uri-tcp.c +++ b/tests/connect-uri-tcp.c @@ -80,7 +80,7 @@ main (int argc, char *argv[]) snprintf (uri, sizeof uri, "nbd://localhost:%d", port); - if (nbd_connect_uri (nbd, uri) == -1) { + if (nbd_connect_uri (nbd, uri, LIBNBD_CONNECT_URI_ALLOW_TCP) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/connect-uri-unix.c b/tests/connect-uri-unix.c index 1e27d26..601fdf0 100644 --- a/tests/connect-uri-unix.c +++ b/tests/connect-uri-unix.c @@ -71,7 +71,8 @@ main (int argc, char *argv[]) exit (77); } - if (nbd_connect_uri (nbd, "nbd+unix:///?socket=" SOCKET) == -1) { + if (nbd_connect_uri (nbd, "nbd+unix:///?socket=" SOCKET, + LIBNBD_CONNECT_URI_ALLOW_UNIX) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } -- 2.22.0
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
Add an extra parameter to nbd_connect_uri to control what URIs are permitted, in case the caller wants to pass in user-controlled URIs but have some control over who/what/how the connection happens. For example:
nbd_connect_uri (nbd, "nbd://localhost", LIBNBD_CONNECT_URI_REQUIRE_TLS) => error: URI must specify an encrypted connection: Permission denied
This obviously breaks the existing C API.
Alternative: we could leave nbd_connect_uri() alone, and make it forward to a new API nbd_connect_uri_flags(, default_flags).
+++ b/docs/libnbd.pod @@ -364,7 +364,7 @@ when it is available.
To connect to a URI via the high level API, use:
- nbd_connect_uri (nbd, "nbd://example.com/"); + nbd_connect_uri (nbd, "nbd://example.com/", LIBNBD_CONNECT_URI_ALL);
As written later in this patch, this change in the docs and example code implies the use of the REQUIRE_TLS flag. Is that intentional that passing all flags forbids the use of non-encrypted connections?
+++ b/generator/generator @@ -939,7 +939,17 @@ let cmd_flags = { "REQ_ONE", 1 lsl 3; ] } -let all_flags = [ cmd_flags ] +let connect_uri_allow_flags = { + flag_prefix = "CONNECT_URI"; + all_flags_bitmask = true; + flags = [ + "ALLOW_TCP", 1 lsl 0; + "ALLOW_UNIX", 1 lsl 1; + "ALLOW_TLS", 1 lsl 2; + "REQUIRE_TLS", 1 lsl 3; + ]
The REQUIRE_TLS flag is worth having, but putting it in the bitmask for all flags makes for some odd semantics.
+} +let all_flags = [ cmd_flags; connect_uri_allow_flags ]
(* Calls. * @@ -1225,7 +1235,8 @@ C<\"qemu:dirty-bitmap:...\"> for qemu-nbd
"connect_uri", { default_call with - args = [ String "uri" ]; ret = RErr; + args = [ String "uri"; Flags ("allow", connect_uri_allow_flags) ]; + ret = RErr; permitted_states = [ Created ]; shortdesc = "connect to NBD URI"; longdesc = "\ @@ -1238,7 +1249,37 @@ the connection has been made. This call will fail if libnbd was not compiled with libxml2; you can test whether this is the case with C<nbd_supports_uri>. Support for URIs that require TLS will fail if libnbd was not compiled with -gnutls; you can test whether this is the case with C<nbd_supports_tls>."; +gnutls; you can test whether this is the case with C<nbd_supports_tls>. + +The C<allow> parameter lets you choose which NBD URI features +are enabled, in case for example you don't want to allow +remote connections. Currently defined flags are: + +=over 4 + +=item C<LIBNBD_CONNECT_URI_ALLOW_TCP> + +Allow TCP sockets. + +=item C<LIBNBD_CONNECT_URI_ALLOW_UNIX> + +Allow Unix domain sockets. + +=item C<LIBNBD_CONNECT_URI_ALLOW_TLS> + +Allow TLS encryption. + +=item C<LIBNBD_CONNECT_URI_REQUIRE_TLS> + +Require TLS encryption. + +=item C<LIBNBD_CONNECT_URI_ALL> + +Enables all features which are defined at the time that +the program is compiled. Later features added to libnbd +will not be allowed unless you recompile your program.
This probably needs to call more attention to the fact that all flags means encryption will be required.
@@ -276,6 +278,31 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) goto cleanup; }
+ /* If the user specified the REQUIRE_TLS flag, we assume they must + * also mean to ALLOW_TLS. + */ + if ((allow & LIBNBD_CONNECT_URI_REQUIRE_TLS) != 0) + allow |= LIBNBD_CONNECT_URI_ALLOW_TLS; + + /* Check allow flags. */ + if (tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_TCP)) { + set_error (EPERM, "TCP URIs are not allowed"); + goto cleanup; + } + if (!tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_UNIX)) { + set_error (EPERM, "Unix domain socket URIs are not allowed"); + goto cleanup; + } + if (tls && !(allow & LIBNBD_CONNECT_URI_ALLOW_TLS)) { + set_error (EPERM, "TLS encrypted URIs are not allowed"); + goto cleanup; + } + if (!tls && (allow & LIBNBD_CONNECT_URI_REQUIRE_TLS)) { + set_error (EPERM, "URI must specify an encrypted connection " + "(use nbds: or nbds+unix:)"); + goto cleanup; + } +
Are there any other flags we might want to support, such as permitting or forbidding an authority section that specifies a username?
/* Insist on the scheme://[authority][/absname][?queries] form. */ if (strncmp (raw_uri + strlen (uri->scheme), "://", 3)) { set_error (EINVAL, "URI must begin with '%s://'", uri->scheme); diff --git a/tests/aio-parallel-load.c b/tests/aio-parallel-load.c index 614c22b..16c2aa2 100644 --- a/tests/aio-parallel-load.c +++ b/tests/aio-parallel-load.c @@ -220,7 +220,8 @@ start_thread (void *arg)
/* Connect to nbdkit. */ if (strstr (connection, "://")) { - if (nbd_connect_uri (nbd, connection) == -1) { + if (nbd_connect_uri (nbd, connection, + LIBNBD_CONNECT_URI_ALLOW_UNIX) == -1) {
Is this too strict? 'make check' may not use TCP, but I don't see that as something we can't support for other uses of this binary. Similar question to other changes under tests/. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Sat, Aug 10, 2019 at 04:50:18PM -0500, Eric Blake wrote:
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
+++ b/docs/libnbd.pod @@ -364,7 +364,7 @@ when it is available.
To connect to a URI via the high level API, use:
- nbd_connect_uri (nbd, "nbd://example.com/"); + nbd_connect_uri (nbd, "nbd://example.com/", LIBNBD_CONNECT_URI_ALL);
As written later in this patch, this change in the docs and example code implies the use of the REQUIRE_TLS flag. Is that intentional that passing all flags forbids the use of non-encrypted connections?
Yes I believe it's wrong.
+++ b/generator/generator @@ -939,7 +939,17 @@ let cmd_flags = { "REQ_ONE", 1 lsl 3; ] } -let all_flags = [ cmd_flags ] +let connect_uri_allow_flags = { + flag_prefix = "CONNECT_URI"; + all_flags_bitmask = true; + flags = [ + "ALLOW_TCP", 1 lsl 0; + "ALLOW_UNIX", 1 lsl 1; + "ALLOW_TLS", 1 lsl 2; + "REQUIRE_TLS", 1 lsl 3; + ]
The REQUIRE_TLS flag is worth having, but putting it in the bitmask for all flags makes for some odd semantics.
Indeed. My other idea for this patch was to have a list of features which are denied. Of course this fails open in the case where we add new features, but I think it would fix this case. [...]
+ /* Check allow flags. */ + if (tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_TCP)) { + set_error (EPERM, "TCP URIs are not allowed"); + goto cleanup; + } + if (!tcp && !(allow & LIBNBD_CONNECT_URI_ALLOW_UNIX)) { + set_error (EPERM, "Unix domain socket URIs are not allowed"); + goto cleanup; + } + if (tls && !(allow & LIBNBD_CONNECT_URI_ALLOW_TLS)) { + set_error (EPERM, "TLS encrypted URIs are not allowed"); + goto cleanup; + } + if (!tls && (allow & LIBNBD_CONNECT_URI_REQUIRE_TLS)) { + set_error (EPERM, "URI must specify an encrypted connection " + "(use nbds: or nbds+unix:)"); + goto cleanup; + } +
Are there any other flags we might want to support, such as permitting or forbidding an authority section that specifies a username?
Yes this was just an initial set of ideas for flags. I also thought that instead of these "easy" checks we might instead (or as well) check for more purpose-oriented features. For example "allow connections to remote servers" (if not set, then TCP connections to localhost would be OK). "Allow connections to other users on the system" could take into account the authority section. Actually implementing these can be tricky. Yet another way to think about this problem is that we leave it up to the caller to sort it out. We might offer some help such as some functions to parse fields out of NBD URIs, but leave the details of validating to them.
/* Insist on the scheme://[authority][/absname][?queries] form. */ if (strncmp (raw_uri + strlen (uri->scheme), "://", 3)) { set_error (EINVAL, "URI must begin with '%s://'", uri->scheme); diff --git a/tests/aio-parallel-load.c b/tests/aio-parallel-load.c index 614c22b..16c2aa2 100644 --- a/tests/aio-parallel-load.c +++ b/tests/aio-parallel-load.c @@ -220,7 +220,8 @@ start_thread (void *arg)
/* Connect to nbdkit. */ if (strstr (connection, "://")) { - if (nbd_connect_uri (nbd, connection) == -1) { + if (nbd_connect_uri (nbd, connection, + LIBNBD_CONNECT_URI_ALLOW_UNIX) == -1) {
Is this too strict? 'make check' may not use TCP, but I don't see that as something we can't support for other uses of this binary. Similar question to other changes under tests/.
I made the checks match what the tests need currently. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 8/11/19 3:39 AM, Richard W.M. Jones wrote:
On Sat, Aug 10, 2019 at 04:50:18PM -0500, Eric Blake wrote:
On 8/10/19 8:02 AM, Richard W.M. Jones wrote:
+++ b/docs/libnbd.pod @@ -364,7 +364,7 @@ when it is available.
To connect to a URI via the high level API, use:
- nbd_connect_uri (nbd, "nbd://example.com/"); + nbd_connect_uri (nbd, "nbd://example.com/", LIBNBD_CONNECT_URI_ALL);
As written later in this patch, this change in the docs and example code implies the use of the REQUIRE_TLS flag. Is that intentional that passing all flags forbids the use of non-encrypted connections?
Yes I believe it's wrong.
+++ b/generator/generator @@ -939,7 +939,17 @@ let cmd_flags = { "REQ_ONE", 1 lsl 3; ] } -let all_flags = [ cmd_flags ] +let connect_uri_allow_flags = { + flag_prefix = "CONNECT_URI"; + all_flags_bitmask = true; + flags = [ + "ALLOW_TCP", 1 lsl 0; + "ALLOW_UNIX", 1 lsl 1; + "ALLOW_TLS", 1 lsl 2; + "REQUIRE_TLS", 1 lsl 3; + ]
The REQUIRE_TLS flag is worth having, but putting it in the bitmask for all flags makes for some odd semantics.
Indeed.
My other idea for this patch was to have a list of features which are denied. Of course this fails open in the case where we add new features, but I think it would fix this case.
In libvirt, we added various ListAll APIs with a notion of filtering groups; for example, virConnectListAllDomains has the following list of flags: typedef enum { VIR_CONNECT_LIST_DOMAINS_ACTIVE = 1 << 0, VIR_CONNECT_LIST_DOMAINS_INACTIVE = 1 << 1, VIR_CONNECT_LIST_DOMAINS_PERSISTENT = 1 << 2, VIR_CONNECT_LIST_DOMAINS_TRANSIENT = 1 << 3, VIR_CONNECT_LIST_DOMAINS_RUNNING = 1 << 4, VIR_CONNECT_LIST_DOMAINS_PAUSED = 1 << 5, VIR_CONNECT_LIST_DOMAINS_SHUTOFF = 1 << 6, VIR_CONNECT_LIST_DOMAINS_OTHER = 1 << 7, VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE = 1 << 8, VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE = 1 << 9, VIR_CONNECT_LIST_DOMAINS_AUTOSTART = 1 << 10, VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART = 1 << 11, VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT = 1 << 12, VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT = 1 << 13, VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT = 1 << 14, VIR_CONNECT_LIST_DOMAINS_NO_CHECKPOINT = 1 << 15, } virConnectListAllDomainsFlags; The way it is documented is that every group of flags (active/inactive, persistent/transient, running/paused/shutoff/other, etc) is an orthogonal coverage of the complete list of domains. If a user does not request any of the filters within a group, then the API does not do filtering based on that group; likewise, if a user requests all of the filters within a group, there is usually no observable difference because it selects all of the domains. But where it gets interesting is that if we add a bit to an existing group (such as adding a fifth bit alongside running/paused...), passing none of the bits in the group selects the domains meeting the new category, but passing all of the previously-known bits in the group now performs a filtering that excludes domains matching the new fifth bit. On the other hand, if we add a new feature that is an entirely orthogonal group, then only the domains compiled against the newer libvirt are able to filter on that new feature (an example: libvirt 5.6 added the has_checkpoint/no_checkpoint group). Older clients that don't know about that filter will never set any bits in that group, and therefore be unaffected by the new filtering capability. So taking that same approach to libnbd, we would declare that the bits exposed to nbd_connect_uri are designed to be arranged in orthogonal groups each with 100% coverage, and that any new features would be supported by adding a new group of bits. Our initial groups could thus be: allow_tcp/allow_unix, allow_uncrypted/allow_tls but still leaving the door open to future groups such as: allow_ipv4/allow_ipv6 When adding a new feature, we then decide if the new feature belongs to an existing group; a user passing 0 (for that group) gets to use the new feature, a user passing all_bits avoids the new feature. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones