thr3ads.net - Libguestfs - [Libguestfs] [libnbd PATCH 2/3] golang: Typesafe returns for REnum/RFlags [Sep 2020]

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

Eric Blake

2020-Sep-06 01:40 UTC

[Libguestfs] [libnbd PATCH 0/3] Improve type-safety of ocaml/golang getters

Natural fallout after my recent testsuite additions that fixed a
couple of ocaml bugs in the setters.  However, on at least the OCaml
code, I'm not sure what we should do if a newer libnbd ever returns a
bit that an older NBD.mli was not expecting at the time the OCaml
compiler ran (see below).  I'm also not sure if there is a more
efficient way to avoid outputting Val_FOO() converters for types not
referenced in REnum/RFlags than my hack of __attribute__((unused)).
Hence I'll wait for a review on these.

Now, for the future compatibility lesson, promised above.
nbd_get_handshake_flags is documented as potentially returning
additional bits from a newer libnbd than what the current client code
was compiled against.  How? Let's say a future NBD protocol addition
adds LIBNBD_HANDSHAKE_FLAG_64BIT (as that one might actually be a way
that we implement 64-bit requests...). Presumably, once the NBD
protocol defines it and future libnbd implements it, then future
libnbd will default to setting that bit during nbd_create().  An older
C client that did:

 flags = nbd_get_handshake_flags (nbd) & ~LIBNBD_HANDSHAKE_FLAG_NO_ZEROES;
 nbd_set_handshake_flags (nbd, flags);

will still run (even though flags contains a bit that was not known at
the time the C app was compiled, the libnbd call that checks that all
input bits are known is from the newer libnbd that recognizes
LIBNBD_HANDSHAKE_FLAG_64BIT as valid).  But an OCaml client compiled
against the older interface has no OCaml value to represent the new
bit from the getter, nor any way to specify that new bit to the setter
that is expecting a list of only the old variant type.  Maybe we want
the generator to produce a full list of 31 variants per 'flags' type,
using placeholders for all bits not currently in use, to make it
easier to receive an unknown bit from the getter and turn around to
re-supply it to the setter?  In such a setup, libnbd would still be
rejecting input of out-of-range bits in relation to what libnbd knew
at its compilation time.

Eric Blake (3):
  generator: Introduce REnum/RFlags return types
  golang: Typesafe returns for REnum/RFlags
  ocaml: Typesafe returns for REnum/RFlags

 generator/API.ml                              | 16 ++++--
 generator/API.mli                             |  2 +
 generator/C.ml                                | 20 ++++---
 generator/GoLang.ml                           | 13 ++++-
 generator/OCaml.ml                            | 55 +++++++++++++++++++
 generator/Python.ml                           |  2 +
 ocaml/tests/test_110_defaults.ml              |  5 +-
 ocaml/tests/test_120_set_non_defaults.ml      |  4 +-
 .../libnbd/libnbd_110_defaults_test.go        |  6 +-
 .../libnbd_120_set_non_defaults_test.go       |  2 +-
 10 files changed, 102 insertions(+), 23 deletions(-)

-- 
2.28.0

Eric Blake

2020-Sep-06 01:40 UTC

head link

[Libguestfs] [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types

For now, there is no change to the generated code (although the man
pages are slightly improved), but this will make it easier for later
patches to improve OCaml and Golang bindings so that set/get pairs are
typed symmetrically.
---
 generator/API.ml    | 16 ++++++++++------
 generator/API.mli   |  2 ++
 generator/C.ml      | 20 ++++++++++++--------
 generator/GoLang.ml | 13 ++++++++++++-
 generator/OCaml.ml  |  4 ++++
 generator/Python.ml |  2 ++
 6 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml
index 962b787..bf6030f 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -66,6 +66,8 @@ and ret  | RCookie
 | RString
 | RUInt
+| REnum of enum
+| RFlags of flags
 and closure = {
   cbname : string;
   cbargs : cbarg list;
@@ -442,7 +444,7 @@ test whether this is the case with
L<nbd_supports_tls(3)>.";

   "get_tls", {
     default_call with
-    args = []; ret = RInt;
+    args = []; ret = REnum (tls_enum);
     may_set_error = false;
     shortdesc = "get the TLS request setting";
     longdesc = "\
@@ -678,7 +680,7 @@ Future NBD extensions may add further flags.

   "get_handshake_flags", {
     default_call with
-    args = []; ret = RUInt;
+    args = []; ret = RFlags (handshake_flags);
     may_set_error = false;
     shortdesc = "see which handshake flags are supported";
     longdesc = "\
@@ -2708,11 +2710,13 @@ let ()         failwithf "%s: if may_set_error is
false, permitted_states must be empty (any permitted state)"
                  name

-    (* may_set_error = true is incompatible with RUInt because
-     * these calls cannot return an error indication.
+    (* may_set_error = true is incompatible with RUInt, REnum, and RFlags
+     * because these calls cannot return an error indication.
      *)
-    | name, { ret = RUInt; may_set_error = true } ->
-       failwithf "%s: if ret is RUInt, may_set_error must be false"
name
+    | name, { ret = RUInt; may_set_error = true }
+    | name, { ret = REnum (_); may_set_error = true }
+    | name, { ret = RFlags (_); may_set_error = true } ->
+       failwithf "%s: if ret is RUInt/REnum/RFlags, may_set_error must be
false" name

     (* !may_set_error is incompatible with certain parameters because
      * we have to do a NULL-check on those which may return an error.
diff --git a/generator/API.mli b/generator/API.mli
index 712e837..e45b5c0 100644
--- a/generator/API.mli
+++ b/generator/API.mli
@@ -78,6 +78,8 @@ and ret  | RString                  (** return a newly
allocated string,
                                caller frees, NULL for error *)
 | RUInt                    (** return a bitmask, no error possible *)
+| REnum of enum            (** return an enum, no error possible *)
+| RFlags of flags          (** return bitmask of flags, no error possible *)
 and closure = {
   cbname : string;         (** name of callback function *)
   cbargs : cbarg list;     (** all closures return int for now *)
diff --git a/generator/C.ml b/generator/C.ml
index 6b65f6e..1eb5e85 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -66,15 +66,15 @@ let errcode_of_ret    function
   | RBool | RErr | RFd | RInt | RInt64 | RCookie -> Some "-1"
   | RStaticString | RString -> Some "NULL"
-  | RUInt -> None (* errors not possible *)
+  | RUInt | REnum (_) | RFlags (_) -> None (* errors not possible *)

 let type_of_ret    function
-  | RBool | RErr | RFd | RInt -> "int"
+  | RBool | RErr | RFd | RInt | REnum (_) -> "int"
   | RInt64 | RCookie -> "int64_t"
   | RStaticString -> "const char *"
   | RString -> "char *"
-  | RUInt -> "unsigned"
+  | RUInt | RFlags (_) -> "unsigned"

 let rec name_of_arg = function
 | Bool n -> [n]
@@ -664,22 +664,22 @@ let generate_lib_api_c ()          pr "         
nbd_internal_printable_string (ret);\n"
      | RBool | RErr | RFd | RInt
      | RInt64 | RCookie
-     | RUInt -> ()
+     | RUInt | REnum (_) | RFlags (_) -> ()
     );
     pr "      debug (h, \"leave: ret=\" ";
     (match ret with
-     | RBool | RErr | RFd | RInt -> pr "\"%%d\", ret"
+     | RBool | RErr | RFd | RInt | REnum (_) -> pr "\"%%d\",
ret"
      | RInt64 | RCookie -> pr "\"%%\" PRIi64 \"\",
ret"
      | RStaticString | RString ->
         pr "\"%%s\", ret_printable ? ret_printable :
\"\""
-     | RUInt -> pr "\"%%u\", ret"
+     | RUInt | RFlags (_) -> pr "\"%%u\", ret"
     );
     pr ");\n";
     (match ret with
      | RStaticString | RString -> pr "      free
(ret_printable);\n"
      | RBool | RErr | RFd | RInt
      | RInt64 | RCookie
-     | RUInt -> ()
+     | RUInt | REnum (_) | RFlags (_) -> ()
     );
     pr "    }\n";
     pr "  }\n"
@@ -824,7 +824,11 @@ let generate_docs_nbd_pod name { args; optargs; ret;
       pr "This call returns a string.  The caller must free the\n";
       pr "returned string to avoid a memory leak.\n";
    | RUInt ->
-      pr "This call returns a bitmask.\n"
+      pr "This call returns a bitmask.\n";
+   | REnum ({ enum_prefix }) ->
+      pr "This call returns one of the LIBNBD_%s_* values.\n"
enum_prefix;
+   | RFlags ({ flag_prefix }) ->
+      pr "This call returns a bitmask of LIBNBD_%s_* values.\n"
flag_prefix;
   );
   pr "\n";

diff --git a/generator/GoLang.ml b/generator/GoLang.ml
index 65b3690..732b81f 100644
--- a/generator/GoLang.ml
+++ b/generator/GoLang.ml
@@ -1,3 +1,4 @@
+(* hey emacs, this is OCaml code: -*- tuareg -*- *)
 (* nbd client library in userspace: generator
  * Copyright (C) 2013-2020 Red Hat Inc.
  *
@@ -95,9 +96,11 @@ let go_ret_type = function
   | RCookie -> Some "uint64"
   | RString -> Some "*string"
   (* RUInt returns (type, error) for consistency, but the error is
-   * always nil.
+   * always nil unless h is closed
    *)
   | RUInt -> Some "uint"
+  | REnum (_) -> Some "uint" (* XXX return typed constant instead?
*)
+  | RFlags (_) -> Some "uint" (* XXX return typed constant
instead? *)

 let go_ret_error = function
   | RErr -> None
@@ -109,6 +112,8 @@ let go_ret_error = function
   | RCookie -> Some "0"
   | RString -> Some "nil"
   | RUInt -> Some "0"
+  | REnum (_) -> Some "0"
+  | RFlags (_) -> Some "0"

 let go_ret_c_errcode = function
   | RBool -> Some "-1"
@@ -120,6 +125,8 @@ let go_ret_c_errcode = function
   | RCookie -> Some "-1"
   | RString -> Some "nil"
   | RUInt -> None
+  | REnum (_) -> None
+  | RFlags (_) -> None

 (* We need a wrapper around every function (except Close) to
  * handle errors because cgo calls are sequence points and
@@ -386,6 +393,10 @@ let print_binding (name, { args; optargs; ret; shortdesc })
pr "    return &r, nil\n"
    | RUInt ->
       pr "    return uint (ret), nil\n"
+   | REnum (_) ->
+      pr "    return uint (ret), nil\n" (* XXX Proper enum type? *)
+   | RFlags (_) ->
+      pr "    return uint (ret), nil\n" (* XXX Proper bitmask type?
*)
   );
   pr "}\n";
   pr "\n"
diff --git a/generator/OCaml.ml b/generator/OCaml.ml
index b45480c..4a835b0 100644
--- a/generator/OCaml.ml
+++ b/generator/OCaml.ml
@@ -66,6 +66,8 @@ and ocaml_ret_to_string = function
   | RCookie -> "cookie"
   | RString -> "string"
   | RUInt -> "int"
+  | REnum (_) -> "int" (* XXX return enum_prefix.t instead *)
+  | RFlags (_) -> "int" (* XXX return flag_prefix.t list instead
*)

 and ocaml_optarg_to_string = function
   | OClosure { cbname; cbargs } ->
@@ -617,6 +619,8 @@ let print_ocaml_binding (name, { args; optargs; ret })     |
RBool -> pr "  rv = Val_bool (r);\n"
    | RErr -> pr "  rv = Val_unit;\n"
    | RFd | RInt | RUInt -> pr "  rv = Val_int (r);\n"
+   | REnum (_) -> pr "  rv = Val_int (r);\n" (* XXX Use
Val_enum_prefix() *)
+   | RFlags (_) -> pr "  rv = Val_int (r);\n" (* XXX Use
Val_flag_prefix() *)
    | RInt64 | RCookie -> pr "  rv = caml_copy_int64 (r);\n"
    | RStaticString -> pr "  rv = caml_copy_string (r);\n"
    | RString ->
diff --git a/generator/Python.ml b/generator/Python.ml
index 3fa733a..c544d10 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -521,6 +521,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error }         pr "  Py_INCREF (py_ret);\n"
     | RFd
     | RInt
+    | REnum (_)
+    | RFlags (_)
     | RUInt ->
        pr "  py_ret = PyLong_FromLong (ret);\n"
     | RInt64 | RCookie ->
-- 
2.28.0

Eric Blake

2020-Sep-06 01:40 UTC

head link

[Libguestfs] [libnbd PATCH 2/3] golang: Typesafe returns for REnum/RFlags

Now that we have a distinct branch in the generator when returning an
enum or bitmask that cannot fail, we can use it in Golang for
symmetry, so that the result of a get function can be plugged into a
set function without a trip through other types.
---
 generator/GoLang.ml                                  | 12 ++++++------
 .../libnbd/libnbd_110_defaults_test.go               |  6 +++---
 .../libnbd/libnbd_120_set_non_defaults_test.go       |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/generator/GoLang.ml b/generator/GoLang.ml
index 732b81f..aecd2d5 100644
--- a/generator/GoLang.ml
+++ b/generator/GoLang.ml
@@ -99,8 +99,8 @@ let go_ret_type = function
    * always nil unless h is closed
    *)
   | RUInt -> Some "uint"
-  | REnum (_) -> Some "uint" (* XXX return typed constant instead?
*)
-  | RFlags (_) -> Some "uint" (* XXX return typed constant
instead? *)
+  | REnum ({ enum_prefix}) -> Some (camel_case enum_prefix)
+  | RFlags ({ flag_prefix}) -> Some (camel_case flag_prefix)

 let go_ret_error = function
   | RErr -> None
@@ -393,10 +393,10 @@ let print_binding (name, { args; optargs; ret; shortdesc
})        pr "    return &r, nil\n"
    | RUInt ->
       pr "    return uint (ret), nil\n"
-   | REnum (_) ->
-      pr "    return uint (ret), nil\n" (* XXX Proper enum type? *)
-   | RFlags (_) ->
-      pr "    return uint (ret), nil\n" (* XXX Proper bitmask type?
*)
+   | REnum ({enum_prefix}) ->
+      pr "    return %s (ret), nil\n" (camel_case enum_prefix)
+   | RFlags ({flag_prefix}) ->
+      pr "    return %s (ret), nil\n" (camel_case flag_prefix)
   );
   pr "}\n";
   pr "\n"
diff --git a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
index 7b4ea0a..005289b 100644
--- a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
+++ b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
@@ -47,7 +47,7 @@ func Test110Defaults(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not get tls state: %s", err)
 	}
-	if tls != uint(TLS_DISABLE) {
+	if tls != TLS_DISABLE {
 		t.Fatalf("unexpected tls state")
 	}

@@ -63,8 +63,8 @@ func Test110Defaults(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not get handshake flags: %s", err)
 	}
-	if flags != uint(HANDSHAKE_FLAG_FIXED_NEWSTYLE |
-			 HANDSHAKE_FLAG_NO_ZEROES) {
+	if flags != (HANDSHAKE_FLAG_FIXED_NEWSTYLE |
+		     HANDSHAKE_FLAG_NO_ZEROES) {
 		t.Fatalf("unexpected handshake flags")
 	}

diff --git
a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
index 331304e..685604e 100644
--- a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
+++ b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
@@ -64,7 +64,7 @@ func Test120SetNonDefaults(t *testing.T) {
 		if err != nil {
 			t.Fatalf("could not get tls state: %s", err)
 		}
-		if tls != uint(TLS_ALLOW) {
+		if tls != TLS_ALLOW {
 			t.Fatalf("unexpected tls state")
 		}
 	}
-- 
2.28.0

Eric Blake

2020-Sep-06 01:41 UTC

head link

[Libguestfs] [libnbd PATCH 3/3] ocaml: Typesafe returns for REnum/RFlags

Now that we have a distinct branch in the generator when returning an
enum or bitmask that cannot fail, we can use it in OCaml for symmetry,
so that the result of a get function can be plugged into a set
function without manual conversion of an integer.

While at it, add an abort() if OCaml ever hands us a value we can't
translate to a C int (that one is unreachable per OCaml type safety),
or if we ever get an int from libnbd that we can't translate to an
OCaml type (that one is theoretically probable; perhaps it should
raise an OCaml exception instead?).
---
 generator/OCaml.ml                       | 59 ++++++++++++++++++++++--
 ocaml/tests/test_110_defaults.ml         |  5 +-
 ocaml/tests/test_120_set_non_defaults.ml |  4 +-
 3 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/generator/OCaml.ml b/generator/OCaml.ml
index 4a835b0..cb6633c 100644
--- a/generator/OCaml.ml
+++ b/generator/OCaml.ml
@@ -66,8 +66,8 @@ and ocaml_ret_to_string = function
   | RCookie -> "cookie"
   | RString -> "string"
   | RUInt -> "int"
-  | REnum (_) -> "int" (* XXX return enum_prefix.t instead *)
-  | RFlags (_) -> "int" (* XXX return flag_prefix.t list instead
*)
+  | REnum ({ enum_prefix }) -> enum_prefix ^ ".t"
+  | RFlags ({ flag_prefix }) -> flag_prefix ^ ".t list"

 and ocaml_optarg_to_string = function
   | OClosure { cbname; cbargs } ->
@@ -332,10 +332,33 @@ let print_ocaml_enum_val { enum_prefix; enums }      fun i
(enum, _) ->
       pr "  case %d: r = LIBNBD_%s_%s; break;\n" i enum_prefix enum
   ) enums;
+  pr "  default: abort ();\n";
   pr "  }\n";
   pr "\n";
   pr "  return r;\n";
   pr "}\n";
+  pr "\n";
+  pr "/* Convert int to OCaml %s.t. */\n" enum_prefix;
+  (* Easier to mark function as potentially unused than to search whether
+   * any REnum references this type.
+   *)
+  pr "static value __attribute__ ((unused))\n";
+  pr "Val_%s (int i)\n" enum_prefix;
+  pr "{\n";
+  pr "  CAMLparam0 ();\n";
+  pr "  CAMLlocal1 (rv);\n";
+  pr "\n";
+  pr "  switch (i) {\n";
+  List.iteri (
+    fun i (enum, _) ->
+      pr "  case LIBNBD_%s_%s: rv = Val_int (%d); break;\n"
enum_prefix enum i
+    ) enums;
+  (* Possible if newer libnbd returns value not present in older compilation *)
+  pr "  default: abort ();\n";
+  pr "  }\n";
+  pr "\n";
+  pr "  CAMLreturn (rv);\n";
+  pr "}\n";
   pr "\n"

 let print_ocaml_flag_val { flag_prefix; flags } @@ -360,9 +383,37 @@ let
print_ocaml_flag_val { flag_prefix; flags }      fun i (flag, _) ->
       pr "    case %d: r |= LIBNBD_%s_%s; break;\n" i flag_prefix
flag
   ) flags;
+  pr "    default: abort ();\n";
   pr "    }\n";
   pr "  }\n";
   pr "\n";
   pr "  return r;\n";
   pr "}\n";
+  pr "\n";
+  pr "/* Convert uint32_t bitmask to OCaml %s.t list. */\n"
flag_prefix;
+  (* Easier to mark function as potentially unused than to search whether
+   * any RFlags references this type.
+   *)
+  pr "static value __attribute__ ((unused))\n";
+  pr "Val_%s (unsigned i)\n" flag_prefix;
+  pr "{\n";
+  pr "  CAMLparam0 ();\n";
+  pr "  CAMLlocal2 (cdr, rv);\n";
+  pr "\n";
+  pr "  cdr = Val_emptylist;\n";
+  List.iteri (
+    fun i (flag, _) ->
+      pr "  if (i & LIBNBD_%s_%s) {\n" flag_prefix flag;
+      pr "    rv = caml_alloc(2, 0);\n";
+      pr "    Store_field(rv, 0, Val_int(%d));\n" i;
+      pr "    Store_field(rv, 1, cdr);\n";
+      pr "    cdr = rv;\n";
+      pr "    i &= ~LIBNBD_%s_%s;\n" flag_prefix flag;
+      pr "  }\n"
+  ) flags;
+  (* Possible if newer libnbd returns value not present in older compilation *)
+  pr "  if (i) abort ();\n";
+  pr "\n";
+  pr "  CAMLreturn (rv);\n";
+  pr "}\n";
   pr "\n"

 let print_ocaml_closure_wrapper { cbname; cbargs } @@ -619,8 +670,8 @@ let
print_ocaml_binding (name, { args; optargs; ret })     | RBool -> pr " 
rv = Val_bool (r);\n"
    | RErr -> pr "  rv = Val_unit;\n"
    | RFd | RInt | RUInt -> pr "  rv = Val_int (r);\n"
-   | REnum (_) -> pr "  rv = Val_int (r);\n" (* XXX Use
Val_enum_prefix() *)
-   | RFlags (_) -> pr "  rv = Val_int (r);\n" (* XXX Use
Val_flag_prefix() *)
+   | REnum ({ enum_prefix }) -> pr "  rv = Val_%s (r);\n"
enum_prefix
+   | RFlags ({ flag_prefix }) -> pr "  rv = Val_%s (r);\n"
flag_prefix
    | RInt64 | RCookie -> pr "  rv = caml_copy_int64 (r);\n"
    | RStaticString -> pr "  rv = caml_copy_string (r);\n"
    | RString ->
diff --git a/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml
index 6953b2d..f969b2f 100644
--- a/ocaml/tests/test_110_defaults.ml
+++ b/ocaml/tests/test_110_defaults.ml
@@ -24,11 +24,12 @@ let ()    let info = NBD.get_full_info nbd in
   assert (info = false);
   let tls = NBD.get_tls nbd in
-  assert (tls = 0);   (* XXX Add REnum, to get NBD.TLS.DISABLE? *)
+  assert (tls = NBD.TLS.DISABLE);
   let sr = NBD.get_request_structured_replies nbd in
   assert (sr = true);
   let flags = NBD.get_handshake_flags nbd in
-  assert (flags = 3); (* XXX Add RFlags, to get NBD.HANDSHAKE_FLAG list? *)
+  assert (flags = [ NBD.HANDSHAKE_FLAG.NO_ZEROES;
+                    NBD.HANDSHAKE_FLAG.FIXED_NEWSTYLE ]);
   let opt = NBD.get_opt_mode nbd in
   assert (opt = false)

diff --git a/ocaml/tests/test_120_set_non_defaults.ml
b/ocaml/tests/test_120_set_non_defaults.ml
index bd41e37..df3d479 100644
--- a/ocaml/tests/test_120_set_non_defaults.ml
+++ b/ocaml/tests/test_120_set_non_defaults.ml
@@ -28,14 +28,14 @@ let ()    if NBD.supports_tls nbd then (
     NBD.set_tls nbd NBD.TLS.ALLOW;
     let tls = NBD.get_tls nbd in
-    assert (tls = 1);  (* XXX Add REnum *)
+    assert (tls = NBD.TLS.ALLOW);
   );
   NBD.set_request_structured_replies nbd false;
   let sr = NBD.get_request_structured_replies nbd in
   assert (sr = false);
   NBD.set_handshake_flags nbd [];
   let flags = NBD.get_handshake_flags nbd in
-  assert (flags = 0); (* XXX Add RFlags *)
+  assert (flags = []);
   NBD.set_opt_mode nbd true;
   let opt = NBD.get_opt_mode nbd in
   assert (opt = true)
-- 
2.28.0

Richard W.M. Jones

2020-Sep-07 07:37 UTC

head link

Re: [Libguestfs] [libnbd PATCH 0/3] Improve type-safety of ocaml/golang getters

On Sat, Sep 05, 2020 at 08:40:57PM -0500, Eric Blake
wrote:> Natural fallout after my recent testsuite additions that fixed a
> couple of ocaml bugs in the setters.  However, on at least the OCaml
> code, I'm not sure what we should do if a newer libnbd ever returns a
> bit that an older NBD.mli was not expecting at the time the OCaml
> compiler ran (see below).  I'm also not sure if there is a more
> efficient way to avoid outputting Val_FOO() converters for types not
> referenced in REnum/RFlags than my hack of __attribute__((unused)).
> Hence I'll wait for a review on these.
> 
> Now, for the future compatibility lesson, promised above.
> nbd_get_handshake_flags is documented as potentially returning
> additional bits from a newer libnbd than what the current client code
> was compiled against.  How? Let's say a future NBD protocol addition
> adds LIBNBD_HANDSHAKE_FLAG_64BIT (as that one might actually be a way
> that we implement 64-bit requests...). Presumably, once the NBD
> protocol defines it and future libnbd implements it, then future
> libnbd will default to setting that bit during nbd_create().  An older
> C client that did:
> 
>  flags = nbd_get_handshake_flags (nbd) &
~LIBNBD_HANDSHAKE_FLAG_NO_ZEROES;
>  nbd_set_handshake_flags (nbd, flags);
> 
> will still run (even though flags contains a bit that was not known at
> the time the C app was compiled, the libnbd call that checks that all
> input bits are known is from the newer libnbd that recognizes
> LIBNBD_HANDSHAKE_FLAG_64BIT as valid).  But an OCaml client compiled
> against the older interface has no OCaml value to represent the new
> bit from the getter, nor any way to specify that new bit to the setter
> that is expecting a list of only the old variant type.  Maybe we want
> the generator to produce a full list of 31 variants per 'flags'
type,
> using placeholders for all bits not currently in use, to make it
> easier to receive an unknown bit from the getter and turn around to
> re-supply it to the setter?  In such a setup, libnbd would still be
> rejecting input of out-of-range bits in relation to what libnbd knew
> at its compilation time.
While placeholders could be used, a more natural way is to extend the
flags type (in OCaml) to:

  type t   | FIXED_NEWSTYLE
  | NO_ZEROES
  | UNKNOWN_FLAG of int

where the unknown (at the time of compilation) flags would be
encoded as UNKNOWN_FLAG + the bit index of the unknown flag.  eg:

  [ FIXED_NEWSTYLE; UNKNOWN_FLAG 2 ] => 1 | (1<<2)
  (1<<3) | 2                         => [ UNKNOWN_FLAG 3; NO_ZEROES ]

Even though we distribute the OCaml bindings with libnbd it's
conceivable this could be useful because OCaml bindings are generally
statically linked into the final binary so you could get a newer
libnbd.so.0 / old binary with old OCaml bindings scenario.

Rich.

> Eric Blake (3):
>   generator: Introduce REnum/RFlags return types
>   golang: Typesafe returns for REnum/RFlags
>   ocaml: Typesafe returns for REnum/RFlags
> 
>  generator/API.ml                              | 16 ++++--
>  generator/API.mli                             |  2 +
>  generator/C.ml                                | 20 ++++---
>  generator/GoLang.ml                           | 13 ++++-
>  generator/OCaml.ml                            | 55 +++++++++++++++++++
>  generator/Python.ml                           |  2 +
>  ocaml/tests/test_110_defaults.ml              |  5 +-
>  ocaml/tests/test_120_set_non_defaults.ml      |  4 +-
>  .../libnbd/libnbd_110_defaults_test.go        |  6 +-
>  .../libnbd_120_set_non_defaults_test.go       |  2 +-
>  10 files changed, 102 insertions(+), 23 deletions(-)
> 
> -- 
> 2.28.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Eric Blake

2020-Sep-07 12:37 UTC

head link

Re: [Libguestfs] [libnbd PATCH 0/3] Improve type-safety of ocaml/golang getters

On 9/7/20 2:37 AM, Richard W.M. Jones wrote:> On Sat, Sep 05, 2020 at 08:40:57PM -0500, Eric Blake wrote:
>> Natural fallout after my recent testsuite additions that fixed a
>> couple of ocaml bugs in the setters.  However, on at least the OCaml
>> code, I'm not sure what we should do if a newer libnbd ever returns
a
>> bit that an older NBD.mli was not expecting at the time the OCaml
>> compiler ran (see below).  I'm also not sure if there is a more
>> efficient way to avoid outputting Val_FOO() converters for types not
>> referenced in REnum/RFlags than my hack of __attribute__((unused)).
With a bit more experimentation, I finally figured out how to avoid that 
hack:

+  if List.exists (
+         function
+         | _, { ret = REnum { enum_prefix = prefix } } ->
+            (prefix = enum_prefix)
+         | _ -> false
+       ) handle_calls then (
+    pr "/* Convert int to OCaml %s.t. */\n" enum_prefix;
...
>>     Maybe we want
>> the generator to produce a full list of 31 variants per 'flags'
type,
>> using placeholders for all bits not currently in use, to make it
>> easier to receive an unknown bit from the getter and turn around to
>> re-supply it to the setter?  In such a setup, libnbd would still be
>> rejecting input of out-of-range bits in relation to what libnbd knew
>> at its compilation time.
> 
> While placeholders could be used, a more natural way is to extend the
> flags type (in OCaml) to:
> 
>    type t >    | FIXED_NEWSTYLE
>    | NO_ZEROES
>    | UNKNOWN_FLAG of int
> 
> where the unknown (at the time of compilation) flags would be
> encoded as UNKNOWN_FLAG + the bit index of the unknown flag.  eg:
> 
>    [ FIXED_NEWSTYLE; UNKNOWN_FLAG 2 ] => 1 | (1<<2)
>    (1<<3) | 2                         => [ UNKNOWN_FLAG 3;
NO_ZEROES ]
> 
> Even though we distribute the OCaml bindings with libnbd it's
> conceivable this could be useful because OCaml bindings are generally
> statically linked into the final binary so you could get a newer
> libnbd.so.0 / old binary with old OCaml bindings scenario.
Yes, that sounds nicer.  I'm experimenting with augmenting flags types 
along those lines now.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Richard W.M. Jones

2020-Sep-07 14:13 UTC

head link

Re: [Libguestfs] [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types

On Sat, Sep 05, 2020 at 08:40:58PM -0500, Eric Blake
wrote:> diff --git a/generator/API.mli b/generator/API.mli
> index 712e837..e45b5c0 100644
> --- a/generator/API.mli
> +++ b/generator/API.mli
> @@ -78,6 +78,8 @@ and ret >  | RString                  (** return a
newly allocated string,
>                                 caller frees, NULL for error *)
>  | RUInt                    (** return a bitmask, no error possible *)
> +| REnum of enum            (** return an enum, no error possible *)
> +| RFlags of flags          (** return bitmask of flags, no error possible
*)
>  and closure = {
>    cbname : string;         (** name of callback function *)
>    cbargs : cbarg list;     (** all closures return int for now *)
> diff --git a/generator/C.ml b/generator/C.ml
> index 6b65f6e..1eb5e85 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -66,15 +66,15 @@ let errcode_of_ret >    function
>    | RBool | RErr | RFd | RInt | RInt64 | RCookie -> Some "-1"
>    | RStaticString | RString -> Some "NULL"
> -  | RUInt -> None (* errors not possible *)
> +  | RUInt | REnum (_) | RFlags (_) -> None (* errors not possible *)
> 
>  let type_of_ret >    function
> -  | RBool | RErr | RFd | RInt -> "int"
> +  | RBool | RErr | RFd | RInt | REnum (_) -> "int"
This is good because Enum is passed as int.
> [...]
You used unsigned for RFlags, but shouldn't it be this instead?

+  | RFlags (_) -> "uint32_t"

The rest of the patch looks fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Richard W.M. Jones

2020-Sep-07 14:14 UTC

head link

Re: [Libguestfs] [libnbd PATCH 2/3] golang: Typesafe returns for REnum/RFlags

On Sat, Sep 05, 2020 at 08:40:59PM -0500, Eric Blake
wrote:> Now that we have a distinct branch in the generator when returning an
> enum or bitmask that cannot fail, we can use it in Golang for
> symmetry, so that the result of a get function can be plugged into a
> set function without a trip through other types.
> ---
>  generator/GoLang.ml                                  | 12 ++++++------
>  .../libnbd/libnbd_110_defaults_test.go               |  6 +++---
>  .../libnbd/libnbd_120_set_non_defaults_test.go       |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> index 732b81f..aecd2d5 100644
> --- a/generator/GoLang.ml
> +++ b/generator/GoLang.ml
> @@ -99,8 +99,8 @@ let go_ret_type = function
>     * always nil unless h is closed
>     *)
>    | RUInt -> Some "uint"
> -  | REnum (_) -> Some "uint" (* XXX return typed constant
instead? *)
> -  | RFlags (_) -> Some "uint" (* XXX return typed constant
instead? *)
> +  | REnum ({ enum_prefix}) -> Some (camel_case enum_prefix)
> +  | RFlags ({ flag_prefix}) -> Some (camel_case flag_prefix)
> 
>  let go_ret_error = function
>    | RErr -> None
> @@ -393,10 +393,10 @@ let print_binding (name, { args; optargs; ret;
shortdesc }) >        pr "    return &r, nil\n"
>     | RUInt ->
>        pr "    return uint (ret), nil\n"
> -   | REnum (_) ->
> -      pr "    return uint (ret), nil\n" (* XXX Proper enum type?
*)
> -   | RFlags (_) ->
> -      pr "    return uint (ret), nil\n" (* XXX Proper bitmask
type? *)
> +   | REnum ({enum_prefix}) ->
> +      pr "    return %s (ret), nil\n" (camel_case enum_prefix)
> +   | RFlags ({flag_prefix}) ->
> +      pr "    return %s (ret), nil\n" (camel_case flag_prefix)
>    );
>    pr "}\n";
>    pr "\n"
> diff --git a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
> index 7b4ea0a..005289b 100644
> --- a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
> +++ b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
> @@ -47,7 +47,7 @@ func Test110Defaults(t *testing.T) {
>  	if err != nil {
>  		t.Fatalf("could not get tls state: %s", err)
>  	}
> -	if tls != uint(TLS_DISABLE) {
> +	if tls != TLS_DISABLE {
>  		t.Fatalf("unexpected tls state")
>  	}
> 
> @@ -63,8 +63,8 @@ func Test110Defaults(t *testing.T) {
>  	if err != nil {
>  		t.Fatalf("could not get handshake flags: %s", err)
>  	}
> -	if flags != uint(HANDSHAKE_FLAG_FIXED_NEWSTYLE |
> -			 HANDSHAKE_FLAG_NO_ZEROES) {
> +	if flags != (HANDSHAKE_FLAG_FIXED_NEWSTYLE |
> +		     HANDSHAKE_FLAG_NO_ZEROES) {
>  		t.Fatalf("unexpected handshake flags")
>  	}
> 
> diff --git
a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
> index 331304e..685604e 100644
> --- a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
> +++ b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
> @@ -64,7 +64,7 @@ func Test120SetNonDefaults(t *testing.T) {
>  		if err != nil {
>  			t.Fatalf("could not get tls state: %s", err)
>  		}
> -		if tls != uint(TLS_ALLOW) {
> +		if tls != TLS_ALLOW {
>  			t.Fatalf("unexpected tls state")
>  		}
>  	}
Seems reasonable.  I suppose the test case would fail if it
was wrong, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Richard W.M. Jones

2020-Sep-07 14:20 UTC

head link

Re: [Libguestfs] [libnbd PATCH 3/3] ocaml: Typesafe returns for REnum/RFlags

For this one I'd definitely prefer the UNKNOWN_FLAG of int
trick I think.

Examples of this idea being used in other code:

https://github.com/ocaml/ocaml/blob/0c8e78268821b4949b8574bb1e8ddafa73d19aa7/otherlibs/unix/unix.mli#L96

https://github.com/libguestfs/hivex/blob/77f4ab4f6314457050429ed72b4c609bdfebc89e/generator/generator.ml#L1566

https://libvirt.org/git/?p=libvirt-ocaml.git;a=blob;f=libvirt/libvirt.mli;h=79003923df54997f905040594a3d5f23c056c5f0;hb=HEAD#l797

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Maybe Matching Threads

Search for more apparently analagous threads

Libguestfs - Sep 2020 - [libnbd PATCH 2/3] golang: Typesafe returns for REnum/RFlags

[Libguestfs] [libnbd PATCH 0/3] Improve type-safety of ocaml/golang getters

[Libguestfs] [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types

[Libguestfs] [libnbd PATCH 2/3] golang: Typesafe returns for REnum/RFlags

[Libguestfs] [libnbd PATCH 3/3] ocaml: Typesafe returns for REnum/RFlags

Re: [Libguestfs] [libnbd PATCH 0/3] Improve type-safety of ocaml/golang getters

Re: [Libguestfs] [libnbd PATCH 0/3] Improve type-safety of ocaml/golang getters

Re: [Libguestfs] [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types

Re: [Libguestfs] [libnbd PATCH 2/3] golang: Typesafe returns for REnum/RFlags

Re: [Libguestfs] [libnbd PATCH 3/3] ocaml: Typesafe returns for REnum/RFlags

Maybe Matching Threads