8:40 p.m.
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
8:40 p.m.
New subject: [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
9:13 a.m.
New subject: [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/
9:38 a.m.
New subject: [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types
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
9:52 a.m.
New subject: [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types
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/
9:58 a.m.
New subject: [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types
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
8:40 p.m.
New subject: [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
9:14 a.m.
New subject: [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
8:41 p.m.
New subject: [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
9:20 a.m.
New subject: [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/0c8e78268821b4949b8574bb1e8ddafa73d19...
https://github.com/libguestfs/hivex/blob/77f4ab4f6314457050429ed72b4c609b...
https://libvirt.org/git/?p=libvirt-ocaml.git;a=blob;f=libvirt/libvirt.mli...
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
2:37 a.m.
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(a)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
7:37 a.m.
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
1539
days inactive
1540
days old
11 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones