4:45 p.m.
Well, the golang changes (patch 1 and 2/3 of v1) were already
committed, all that was left was the OCaml changes. I'm a lot happier
with how things turned out with an UNKNOWN constructor in the OCaml
variants.
Eric Blake (3):
tests: Enhance coverage of enum/flag range checking
ocaml: Support unknown values for Enum/Flags
ocaml: Typesafe returns for REnum/RFlags
generator/OCaml.ml | 123 +++++++++++++++---
python/t/120-set-non-defaults.py | 12 ++
ocaml/tests/test_110_defaults.ml | 5 +-
ocaml/tests/test_120_set_non_defaults.ml | 19 ++-
tests/errors.c | 32 +++++
.../libnbd_120_set_non_defaults_test.go | 28 +++-
6 files changed, 194 insertions(+), 25 deletions(-)
--
2.28.0
4:45 p.m.
New subject: [libnbd PATCH v2 1/3] tests: Enhance coverage of enum/flag range checking
Add testsuite coverage to ensure that we reject out-of-range values
for enum/flag parameters. The fact that we can't provoke it in ocaml
is worth fixing in the next patch.
---
python/t/120-set-non-defaults.py | 12 +++++++
ocaml/tests/test_120_set_non_defaults.ml | 10 ++++++
tests/errors.c | 32 +++++++++++++++++++
.../libnbd_120_set_non_defaults_test.go | 28 ++++++++++++++--
4 files changed, 80 insertions(+), 2 deletions(-)
diff --git a/python/t/120-set-non-defaults.py b/python/t/120-set-non-defaults.py
index c04ede9..fe0bb87 100644
--- a/python/t/120-set-non-defaults.py
+++ b/python/t/120-set-non-defaults.py
@@ -22,11 +22,23 @@ h.set_export_name ("name")
assert h.get_export_name () == "name"
h.set_full_info (True)
assert h.get_full_info () == True
+try:
+ h.set_tls (nbd.TLS_REQUIRE + 1)
+ assert False
+except nbd.Error as e:
+ pass
+assert h.get_tls () == nbd.TLS_DISABLE
if h.supports_tls ():
h.set_tls (nbd.TLS_ALLOW)
assert h.get_tls () == nbd.TLS_ALLOW
h.set_request_structured_replies (False)
assert h.get_request_structured_replies () == False
+try:
+ h.set_handshake_flags (nbd.HANDSHAKE_FLAG_NO_ZEROES * 2)
+ assert False
+except nbd.Error as e:
+ assert h.get_handshake_flags () == (nbd.HANDSHAKE_FLAG_NO_ZEROES |
+ nbd.HANDSHAKE_FLAG_FIXED_NEWSTYLE)
h.set_handshake_flags (0)
assert h.get_handshake_flags () == 0
h.set_opt_mode (True)
diff --git a/ocaml/tests/test_120_set_non_defaults.ml
b/ocaml/tests/test_120_set_non_defaults.ml
index bd41e37..e616291 100644
--- a/ocaml/tests/test_120_set_non_defaults.ml
+++ b/ocaml/tests/test_120_set_non_defaults.ml
@@ -25,6 +25,11 @@ let () =
NBD.set_full_info nbd true;
let info = NBD.get_full_info nbd in
assert (info = true);
+ (* XXX No way to pass out-of-range enum...
+ try NBD.set_tls nbd XXX
+ *)
+ let tls = NBD.get_tls nbd in
+ assert (tls = 0); (* XXX Add REnum, to get NBD.TLS.DISABLE? *)
if NBD.supports_tls nbd then (
NBD.set_tls nbd NBD.TLS.ALLOW;
let tls = NBD.get_tls nbd in
@@ -33,6 +38,11 @@ let () =
NBD.set_request_structured_replies nbd false;
let sr = NBD.get_request_structured_replies nbd in
assert (sr = false);
+ (* XXX No way to pass out-of-range flags...
+ try NBD.set_handshake_flags nbd [ XXX ]
+ *)
+ let flags = NBD.get_handshake_flags nbd in
+ assert (flags = 3); (* XXX Add RFlags, to get NBD.HANDSHAKE_FLAG list? *)
NBD.set_handshake_flags nbd [];
let flags = NBD.get_handshake_flags nbd in
assert (flags = 0); (* XXX Add RFlags *)
diff --git a/tests/errors.c b/tests/errors.c
index 6627ce2..906c7da 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -86,6 +86,7 @@ main (int argc, char *argv[])
*/
const char *cmd[] = { "nbdkit", "-s",
"--exit-with-parent", "sh",
script, NULL };
+ int i;
progname = argv[0];
@@ -131,6 +132,37 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
+ /* Attempt to set an enum to garbage. */
+ if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE + 1) != -1) {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_set_tls did not reject invalid enum\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ if (nbd_get_tls (nbd) != LIBNBD_TLS_DISABLE) {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_get_tls not left at default value\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+ /* Attempt to set a bitmask with an unknown bit. */
+ i = LIBNBD_HANDSHAKE_FLAG_NO_ZEROES << 1;
+ if (nbd_set_handshake_flags (nbd, i) != -1) {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_set_handshake_flags did not reject invalid bitmask\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ i = LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE | LIBNBD_HANDSHAKE_FLAG_NO_ZEROES;
+ if (nbd_get_handshake_flags (nbd) != i) {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_get_handshake_flags not left at default value\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+
/* Issue a connected command when not connected. */
if (nbd_pread (nbd, buf, 1, 0, 0) != -1) {
fprintf (stderr, "%s: test failed: "
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 685604e..5d8cce7 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
@@ -51,6 +51,18 @@ func Test120SetNonDefaults(t *testing.T) {
t.Fatalf("unexpected full info state")
}
+ err = h.SetTls(TLS_REQUIRE + 1)
+ if err == nil {
+ t.Fatalf("expect failure for out-of-range enum")
+ }
+ tls, err := h.GetTls()
+ if err != nil {
+ t.Fatalf("could not get tls state: %s", err)
+ }
+ if tls != TLS_DISABLE {
+ t.Fatalf("unexpected tls state")
+ }
+
support, err := h.SupportsTls()
if err != nil {
t.Fatalf("could not check if tls is supported: %s", err)
@@ -60,7 +72,7 @@ func Test120SetNonDefaults(t *testing.T) {
if err != nil {
t.Fatalf("could not set tls state: %s", err)
}
- tls, err := h.GetTls()
+ tls, err = h.GetTls()
if err != nil {
t.Fatalf("could not get tls state: %s", err)
}
@@ -81,11 +93,23 @@ func Test120SetNonDefaults(t *testing.T) {
t.Fatalf("unexpected structured replies state")
}
+ err = h.SetHandshakeFlags(HANDSHAKE_FLAG_NO_ZEROES << 1)
+ if err == nil {
+ t.Fatalf("expect failure for out-of-range flags")
+ }
+ flags, err := h.GetHandshakeFlags()
+ if err != nil {
+ t.Fatalf("could not get handshake flags: %s", err)
+ }
+ if flags != HANDSHAKE_FLAG_FIXED_NEWSTYLE | HANDSHAKE_FLAG_NO_ZEROES {
+ t.Fatalf("unexpected handshake flags")
+ }
+
err = h.SetHandshakeFlags(0)
if err != nil {
t.Fatalf("could not set handshake flags: %s", err)
}
- flags, err := h.GetHandshakeFlags()
+ flags, err = h.GetHandshakeFlags()
if err != nil {
t.Fatalf("could not get handshake flags: %s", err)
}
--
2.28.0
4:45 p.m.
New subject: [libnbd PATCH v2 2/3] ocaml: Support unknown values for Enum/Flags
Of our existing enum types, tls_enum is probably not going to extend,
but block_size_enum definitely will. Similarly, for our flags types,
all of them have the possibility of future NBD protocol additions
defining new bit positions. While it is nice to name known bits, the
typical OCaml variant representation of an extendible C enum also
includes a catch-all constructor for unknown C values (see for example
Unix.error with its 'EUNKNOWNERR of int').
Our C code rejects unknown values, but allowing OCaml the flexibility
of passing in all valid C values, rather than just the ones that were
known at the time the OCaml bindings were compiled, lets us deal with
situations where a newer libnbd.so could return a new bit in a get_*
function, and where we want to leave that bit still set in the
corresponding set_* function. And it lets us match the testsuite
coverage to the support present in other languages in the previous
patch.
Thanks: Rich Jones
---
generator/OCaml.ml | 46 +++++++++++++++++-------
ocaml/tests/test_120_set_non_defaults.ml | 16 +++++----
2 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/generator/OCaml.ml b/generator/OCaml.ml
index 63f442c..db7003c 100644
--- a/generator/OCaml.ml
+++ b/generator/OCaml.ml
@@ -151,6 +151,7 @@ type cookie = int64
fun (enum, _) ->
pr " | %s\n" enum
) enums;
+ pr " | UNKNOWN of int\n";
pr "end\n";
pr "\n"
) all_enums;
@@ -162,6 +163,7 @@ type cookie = int64
fun (flag, _) ->
pr " | %s\n" flag
) flags;
+ pr " | UNKNOWN of int\n";
pr "end\n";
pr "\n"
) all_flags;
@@ -254,6 +256,7 @@ let () =
fun (enum, _) ->
pr " | %s\n" enum
) enums;
+ pr " | UNKNOWN of int\n";
pr "end\n";
pr "\n"
) all_enums;
@@ -265,6 +268,7 @@ let () =
fun (flag, _) ->
pr " | %s\n" flag
) flags;
+ pr " | UNKNOWN of int\n";
pr "end\n";
pr "\n"
) all_flags;
@@ -320,19 +324,23 @@ let print_ocaml_enum_val { enum_prefix; enums } =
pr " /* NB: No allocation in this function, don't need to use\n";
pr " * CAML* wrappers.\n";
pr " */\n";
- pr " int i, r = 0;\n";
+ pr " int r = 0;\n";
pr "\n";
- pr " i = Int_val (v);\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";
+ pr " if (Is_long (v)) {\n";
+ pr " /* Int_val (v) is the index of the enum in the type\n";
+ pr " * (eg. v = 0 => enum = %s.%s).\n" enum_prefix (fst (List.hd
enums));
+ pr " * Convert it to the C representation.\n";
+ pr " */\n";
+ pr " switch (Int_val (v)) {\n";
List.iteri (
fun i (enum, _) ->
- pr " case %d: r = LIBNBD_%s_%s; break;\n" i enum_prefix enum
+ pr " case %d: r = LIBNBD_%s_%s; break;\n" i enum_prefix enum
) enums;
+ pr " default: abort ();\n";
+ pr " }\n";
pr " }\n";
+ pr " else\n";
+ pr " r = Int_val (Field (v, 0)); /* UNKNOWN of int */\n";
pr "\n";
pr " return r;\n";
pr "}\n";
@@ -346,20 +354,32 @@ let print_ocaml_flag_val { flag_prefix; flags } =
pr " /* NB: No allocation in this function, don't need to use\n";
pr " * CAML* wrappers.\n";
pr " */\n";
- pr " int i;\n";
+ pr " value i;\n";
+ pr " unsigned bit;\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 " i = Field (v, 0);\n";
+ pr " /* i contains either the index of the flag in the type,\n";
+ pr " * or UNKNOWN of int containing the bit position.\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";
+ pr " if (Is_long (i)) {\n";
+ pr " switch (Int_val (i)) {\n";
List.iteri (
fun i (flag, _) ->
- pr " case %d: r |= LIBNBD_%s_%s; break;\n" i flag_prefix flag
+ pr " case %d: r |= LIBNBD_%s_%s; break;\n" i flag_prefix flag
) flags;
+ pr " default: abort ();\n";
+ pr " }\n";
+ pr " }\n";
+ pr " else {\n";
+ pr " bit = Int_val (Field (i, 0)); /* UNKNOWN of int */\n";
+ pr " if (bit > 31)\n";
+ pr " caml_invalid_argument (\"bitmask value out of
range\");\n";
+ pr " else\n";
+ pr " r |= 1 << bit;\n";
pr " }\n";
pr " }\n";
pr "\n";
diff --git a/ocaml/tests/test_120_set_non_defaults.ml
b/ocaml/tests/test_120_set_non_defaults.ml
index e616291..0d14710 100644
--- a/ocaml/tests/test_120_set_non_defaults.ml
+++ b/ocaml/tests/test_120_set_non_defaults.ml
@@ -25,9 +25,11 @@ let () =
NBD.set_full_info nbd true;
let info = NBD.get_full_info nbd in
assert (info = true);
- (* XXX No way to pass out-of-range enum...
- try NBD.set_tls nbd XXX
- *)
+ try
+ NBD.set_tls nbd (NBD.TLS.UNKNOWN 3);
+ assert (false)
+ with
+ NBD.Error _ -> ();
let tls = NBD.get_tls nbd in
assert (tls = 0); (* XXX Add REnum, to get NBD.TLS.DISABLE? *)
if NBD.supports_tls nbd then (
@@ -38,9 +40,11 @@ let () =
NBD.set_request_structured_replies nbd false;
let sr = NBD.get_request_structured_replies nbd in
assert (sr = false);
- (* XXX No way to pass out-of-range flags...
- try NBD.set_handshake_flags nbd [ XXX ]
- *)
+ try
+ NBD.set_handshake_flags nbd [ NBD.HANDSHAKE_FLAG.UNKNOWN 2 ];
+ assert false
+ with
+ NBD.Error _ -> ();
let flags = NBD.get_handshake_flags nbd in
assert (flags = 3); (* XXX Add RFlags, to get NBD.HANDSHAKE_FLAG list? *)
NBD.set_handshake_flags nbd [];
--
2.28.0
4:45 p.m.
New subject: [libnbd PATCH v2 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. This includes the
use of the recently-added UNKNOWN catch-all for encoding C values
returned by a newer libnbd.so than when the OCaml bindings were
compiled.
---
generator/OCaml.ml | 77 ++++++++++++++++++++++--
ocaml/tests/test_110_defaults.ml | 5 +-
ocaml/tests/test_120_set_non_defaults.ml | 9 +--
3 files changed, 79 insertions(+), 12 deletions(-)
diff --git a/generator/OCaml.ml b/generator/OCaml.ml
index db7003c..4bcd450 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 } ->
@@ -344,7 +344,34 @@ let print_ocaml_enum_val { enum_prefix; enums } =
pr "\n";
pr " return r;\n";
pr "}\n";
- pr "\n"
+ pr "\n";
+ 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;
+ pr "static value\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;
+ pr " default:\n";
+ pr " rv = caml_alloc (1, 0); /* UNKNOWN of int */\n";
+ pr " Store_field (rv, 0, Val_int (i));\n";
+ pr " }\n";
+ pr "\n";
+ pr " CAMLreturn (rv);\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;
@@ -385,7 +412,45 @@ let print_ocaml_flag_val { flag_prefix; flags } =
pr "\n";
pr " return r;\n";
pr "}\n";
- pr "\n"
+ pr "\n";
+ if List.exists (
+ function
+ | _, { ret = RFlags { flag_prefix = prefix } } ->
+ (prefix = flag_prefix)
+ | _ -> false
+ ) handle_calls then (
+ pr "/* Convert uint32_t bitmask to OCaml %s.t list. */\n" flag_prefix;
+ pr "static value\n";
+ pr "Val_%s (unsigned flags)\n" flag_prefix;
+ pr "{\n";
+ pr " CAMLparam0 ();\n";
+ pr " CAMLlocal3 (cdr, rv, v);\n";
+ pr " int i;\n";
+ pr "\n";
+ pr " rv = Val_emptylist;\n";
+ pr " for (i = 31; i >= 0; i--) {\n";
+ pr " if (flags & (1 << i)) {\n";
+ pr " switch (1 << i) {\n";
+ List.iteri (
+ fun i (flag, _) ->
+ pr " case LIBNBD_%s_%s: v = Val_int (%d); break;\n" flag_prefix
flag i;
+ ) flags;
+ pr " default:\n";
+ pr " v = caml_alloc (1, 0); /* UNKNOWN of int */\n";
+ pr " Store_field (v, 0, Val_int (i));\n";
+ pr " }\n";
+ pr "\n";
+ pr " cdr = rv;\n";
+ pr " rv = caml_alloc (2, 0);\n";
+ pr " Store_field (rv, 0, v);\n";
+ pr " Store_field (rv, 1, cdr);\n";
+ pr " }\n";
+ pr " }\n";
+ pr "\n";
+ pr " CAMLreturn (rv);\n";
+ pr "}\n";
+ pr "\n"
+ )
let print_ocaml_closure_wrapper { cbname; cbargs } =
let argnames =
@@ -639,8 +704,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..54f2cbc 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.FIXED_NEWSTYLE;
+ NBD.HANDSHAKE_FLAG.NO_ZEROES ]);
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 0d14710..79fe184 100644
--- a/ocaml/tests/test_120_set_non_defaults.ml
+++ b/ocaml/tests/test_120_set_non_defaults.ml
@@ -31,11 +31,11 @@ let () =
with
NBD.Error _ -> ();
let tls = NBD.get_tls nbd in
- assert (tls = 0); (* XXX Add REnum, to get NBD.TLS.DISABLE? *)
+ assert (tls = NBD.TLS.DISABLE);
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
@@ -46,10 +46,11 @@ let () =
with
NBD.Error _ -> ();
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.FIXED_NEWSTYLE;
+ NBD.HANDSHAKE_FLAG.NO_ZEROES ]);
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
8:51 a.m.
New subject: [libnbd PATCH v2 3/3] ocaml: Typesafe returns for REnum/RFlags
This series all looks good to me.
The fact that the new tests have Gc.compact () at the end of them
means they ought to crash hard if there is any subtle mistake in the
code which interfaces with the GC. (I didn't see any.)
ACK
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
1517
days inactive
1518
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones