[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
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
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/
On 9/7/20 9:13 AM, Richard W.M. Jones wrote:
+++ 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"
Hmm. For Flags as input, we take uint32_t, but for get_handshake_flags which used to return RUint, we returned unsigned; as written, my patch preserved generated code in its entirety. Changing it to return uint32_t makes sense, but is a minor API change. Fortunately, it would not be an ABI change on any of the platforms we compile on. So I can go ahead and return the smarter type unless we have a strong reason not to (the API change might affect anyone that was storing a function pointer to nbd_get_handshake_flags, and more so in C++ than C).
The rest of the patch looks fine.
Rich.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Sep 07, 2020 at 09:38:15AM -0500, Eric Blake wrote:
On 9/7/20 9:13 AM, Richard W.M. Jones wrote:
+++ 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"
Hmm. For Flags as input, we take uint32_t, but for get_handshake_flags which used to return RUint, we returned unsigned; as written, my patch preserved generated code in its entirety. Changing it to return uint32_t makes sense, but is a minor API change.
Oh I see, good point. I think although technically it might not be an ABI change, best to leave it as you wrote it. Rich.
Fortunately, it would not be an ABI change on any of the platforms we compile on. So I can go ahead and return the smarter type unless we have a strong reason not to (the API change might affect anyone that was storing a function pointer to nbd_get_handshake_flags, and more so in C++ than C).
The rest of the patch looks fine.
Rich.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-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/
On 9/7/20 9:52 AM, Richard W.M. Jones wrote:
You used unsigned for RFlags, but shouldn't it be this instead?
+ | RFlags (_) -> "uint32_t"
Hmm. For Flags as input, we take uint32_t, but for get_handshake_flags which used to return RUint, we returned unsigned; as written, my patch preserved generated code in its entirety. Changing it to return uint32_t makes sense, but is a minor API change.
Oh I see, good point.
I think although technically it might not be an ABI change, best to leave it as you wrote it.
Not only is it not an ABI change, but we got lucky: no one has ever compiled libnbd on a platform where it would be an API change. Note that pre-patch, our generated lib/unlocked.h had 'unsigned nbd_get_handshake_flags' but our hand-written lib/handle.c had 'uint32_t nbd_get_handshake_flags'. On every platform where libnbd compiles, the two types are synonymous, so the API change factor is a non-issue (platforms where it might not be the same could be a 32-bit platform where 'uint32_t' is 'long', but no one has reported trying to build libnbd on such a system). So I'll go ahead and make the change, with the commit message giving this additional justification. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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
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
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
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... https://github.com/libguestfs/hivex/blob/77f4ab4f6314457050429ed72b4c609bdfe... https://libvirt.org/git/?p=libvirt-ocaml.git;a=blob;f=libvirt/libvirt.mli;h=... 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
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
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
participants (2)
-
Eric Blake -
Richard W.M. Jones