thr3ads.net - Libguestfs - [Libguestfs] [PATCH libnbd 8/9] generator: Enhance Flags/OFlags with optional "all flags" mask. [Aug 2019]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2019-Aug-10 13:02 UTC

[Libguestfs] [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.

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 1/9] api: Don't duplicate connect_uri documentation.

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 2/9] generator: Generalize OFlags.

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 3/9] generator: Add Enum type for enumerated types / unions.

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 4/9] api: Change nbd_set_tls (, 2) -> nbd_set_tls (, LIBNBD_TLS_REQUIRE).

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 5/9] generator: On entry to API functions, check Enum parameters.

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 6/9] generator: Add non-optional Flags type.

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 7/9] generator: On entry to API functions, check Flags and OFlags parameters.

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 8/9] generator: Enhance Flags/OFlags with optional "all flags" mask.

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

Richard W.M. Jones

2019-Aug-10 13:02 UTC

head link

[Libguestfs] [PATCH libnbd 9/9] FOR DISCUSSION ONLY: api: Add ‘allow’ parameter to nbd_connect_uri to control permitted URIs.

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

Eric Blake

2019-Aug-10 17:22 UTC

head link

Re: [Libguestfs] [PATCH libnbd 2/9] generator: Generalize OFlags.

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

Eric Blake

2019-Aug-10 17:32 UTC

head link

Re: [Libguestfs] [PATCH libnbd 3/9] generator: Add Enum type for enumerated types / unions.

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

Eric Blake

2019-Aug-10 17:37 UTC

head link

Re: [Libguestfs] [PATCH libnbd 4/9] api: Change nbd_set_tls (, 2) -> nbd_set_tls (, LIBNBD_TLS_REQUIRE).

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

Eric Blake

2019-Aug-10 17:42 UTC

head link

Re: [Libguestfs] [PATCH libnbd 5/9] generator: On entry to API functions, check Enum parameters.

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

Eric Blake

2019-Aug-10 21:33 UTC

head link

Re: [Libguestfs] [PATCH libnbd 6/9] generator: Add non-optional Flags type.

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

Eric Blake

2019-Aug-10 21:38 UTC

head link

Re: [Libguestfs] [PATCH libnbd 7/9] generator: On entry to API functions, check Flags and OFlags parameters.

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

Eric Blake

2019-Aug-10 21:41 UTC

head link

Re: [Libguestfs] [PATCH libnbd 8/9] generator: Enhance Flags/OFlags with optional "all flags" mask.

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

Eric Blake

2019-Aug-10 21:50 UTC

head link

Re: [Libguestfs] [PATCH libnbd 9/9] FOR DISCUSSION ONLY: api: Add ‘allow’ parameter to nbd_connect_uri to control permitted URIs.

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

Maybe Matching Threads

Search for more reasonably related threads

Libguestfs - Aug 2019 - [PATCH libnbd 8/9] generator: Enhance Flags/OFlags with optional "all flags" mask.

[Libguestfs] [PATCH libnbd 0/9] Add Enum and Flags types.

[Libguestfs] [PATCH libnbd 1/9] api: Don't duplicate connect_uri documentation.

[Libguestfs] [PATCH libnbd 2/9] generator: Generalize OFlags.

[Libguestfs] [PATCH libnbd 3/9] generator: Add Enum type for enumerated types / unions.

[Libguestfs] [PATCH libnbd 4/9] api: Change nbd_set_tls (, 2) -> nbd_set_tls (, LIBNBD_TLS_REQUIRE).

[Libguestfs] [PATCH libnbd 5/9] generator: On entry to API functions, check Enum parameters.

[Libguestfs] [PATCH libnbd 6/9] generator: Add non-optional Flags type.

[Libguestfs] [PATCH libnbd 7/9] generator: On entry to API functions, check Flags and OFlags parameters.

[Libguestfs] [PATCH libnbd 8/9] generator: Enhance Flags/OFlags with optional "all flags" mask.

[Libguestfs] [PATCH libnbd 9/9] FOR DISCUSSION ONLY: api: Add ‘allow’ parameter to nbd_connect_uri to control permitted URIs.

Re: [Libguestfs] [PATCH libnbd 2/9] generator: Generalize OFlags.

Re: [Libguestfs] [PATCH libnbd 3/9] generator: Add Enum type for enumerated types / unions.

Re: [Libguestfs] [PATCH libnbd 4/9] api: Change nbd_set_tls (, 2) -> nbd_set_tls (, LIBNBD_TLS_REQUIRE).

Re: [Libguestfs] [PATCH libnbd 5/9] generator: On entry to API functions, check Enum parameters.

Re: [Libguestfs] [PATCH libnbd 6/9] generator: Add non-optional Flags type.

Re: [Libguestfs] [PATCH libnbd 7/9] generator: On entry to API functions, check Flags and OFlags parameters.

Re: [Libguestfs] [PATCH libnbd 8/9] generator: Enhance Flags/OFlags with optional "all flags" mask.

Re: [Libguestfs] [PATCH libnbd 9/9] FOR DISCUSSION ONLY: api: Add ‘allow’ parameter to nbd_connect_uri to control permitted URIs.

Maybe Matching Threads