Eric Blake
2020-Sep-07 21:45 UTC
[Libguestfs] [libnbd PATCH v2 0/3] Improve type-safety of ocaml/golang getters
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
Eric Blake
2020-Sep-07 21:45 UTC
[Libguestfs] [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
Eric Blake
2020-Sep-07 21:45 UTC
[Libguestfs] [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
Eric Blake
2020-Sep-07 21:45 UTC
[Libguestfs] [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
Richard W.M. Jones
2020-Sep-08 13:51 UTC
Re: [Libguestfs] [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
Possibly Parallel Threads
- [libnbd PATCH 0/3] Improve type-safety of ocaml/golang getters
- [libnbd PATCH v2 0/5] Add knobs for client- vs. server-side validation
- [libnbd PATCH v2 3/3] ocaml: Typesafe returns for REnum/RFlags
- [libnbd PATCH 3/3] ocaml: Typesafe returns for REnum/RFlags
- [libnbd PATCH v2 1/5] api: Add xxx_MASK constant for each Flags type