On 09/03/22 00:14, Eric Blake wrote:
The next patch will add some APIs that expose long-term statistics
counters. A counter is always accessible (no need to return -1; if
the handle is new the count is still 0, and even after the handle is
in DEAD the counter is still useful). But it is also long-running;
the existing RUInt might only be 32 bits, while it is easy to prove
some counters (bytes transmitted, for example) will need 64-bits. So
time to plumb in a new return type.
Using signed int64 in OCaml bindings is okay (actually sending 2^63
bytes of data to overflow the counter takes a LOOONG time, so we don't
worry if we can't get to 2^64).
Should be OK for counters (the first use of the new data type in the
generator), could be a problem for other purposes. See e.g. commit
0e714a6e06e6 ("ocaml: map C's uint32_t to OCaml's int64", 2022-01-17);
mishandling the sign bit there caused infinite loops there.
This is not an argument against the patch / new return data type in
general, just a remark that in the future, once we try to use Uint64 for
other things (not just byte / chunk counters), we could be surprised.
Perhaps add a comment somewhere near the RUint64 data constructor in
"generator/OCaml.ml"?
... Well, at the same time, we already silently map UInt64 (not RUInt64)
to "int64", so there's already prior art for this. OK, feel free to
ignore this remark.
---
generator/API.ml | 5 ++++-
generator/API.mli | 3 ++-
generator/C.ml | 13 +++++++++----
generator/GoLang.ml | 9 ++++++---
generator/OCaml.ml | 5 +++--
generator/Python.ml | 2 +-
6 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/generator/API.ml b/generator/API.ml
index 32a9ad79..e4e2ea0a 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -70,6 +70,7 @@
| RString
| RUInt
| RUIntPtr
+| RUInt64
| REnum of enum
| RFlags of flags
and closure = {
@@ -3368,10 +3369,12 @@ 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, REnum, and RFlags
+ (* 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 }
+ | name, { ret = RUIntPtr; may_set_error = true }
Silent fix for a different omission?
+ | name, { ret = RUInt64; 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
diff --git a/generator/API.mli b/generator/API.mli
index d284637f..b0267705 100644
--- a/generator/API.mli
+++ b/generator/API.mli
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* nbd client library in userspace: the API
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -83,6 +83,7 @@ and
caller frees, NULL for error *)
| RUInt (** return a bitmask, no error possible *)
| RUIntPtr (** uintptr_t in C, same as RUInt in non-C *)
+| RUInt64 (** 64 bit int, no error possible *)
| REnum of enum (** return an enum, no error possible *)
| RFlags of flags (** return bitmask of flags, no error possible *)
and closure = {
diff --git a/generator/C.ml b/generator/C.ml
index 9c67efaa..560f56db 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -68,7 +68,8 @@ let
function
| RBool | RErr | RFd | RInt | RInt64 | RCookie | RSizeT -> Some "-1"
| RStaticString | RString -> Some "NULL"
- | RUInt | RUIntPtr | REnum _ | RFlags _ -> None (* errors not possible *)
+ | RUInt | RUIntPtr | RUInt64 | REnum _
+ | RFlags _ -> None (* errors not possible *)
let type_of_ret =
function
@@ -79,6 +80,7 @@ let
| RString -> "char *"
| RUInt -> "unsigned"
| RUIntPtr -> "uintptr_t"
+ | RUInt64 -> "uint64_t"
| RFlags _ -> "uint32_t"
let rec name_of_arg = function
@@ -730,24 +732,25 @@ let
pr " nbd_internal_printable_string (ret);\n"
| RBool | RErr | RFd | RInt
| RInt64 | RCookie | RSizeT
- | RUInt | RUIntPtr | REnum _ | RFlags _ -> ()
+ | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> ()
);
pr " debug (h, \"leave: ret=\" ";
(match ret with
| RBool | RErr | RFd | RInt | REnum _ -> pr "\"%%d\", ret"
- | RInt64 | RCookie -> pr "\"%%\" PRIi64 \"\",
ret"
+ | RInt64 | RCookie -> pr "\"%%\" PRIi64, ret"
another silent fix for an earlier problem?
| RSizeT -> pr "\"%%zd\", ret"
| RStaticString | RString ->
pr "\"%%s\", ret_printable ? ret_printable :
\"\""
| RUInt | RFlags _ -> pr "\"%%u\", ret"
| RUIntPtr -> pr "\"%%\" PRIuPTR, ret"
+ | RUInt64 -> pr "\"%%\" PRIu64, ret"
);
pr ");\n";
(match ret with
| RStaticString | RString -> pr " free (ret_printable);\n"
| RBool | RErr | RFd | RInt
| RInt64 | RCookie | RSizeT
- | RUInt | REnum _ | RFlags _ | RUIntPtr -> ()
+ | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> ()
looks like there were multiple RUintPtr omissions, those could go into a
unified (but from this, separate) patch.
);
pr " }\n";
pr " }\n"
@@ -904,6 +907,8 @@ let
pr "This call returns a bitmask.\n";
| RUIntPtr ->
pr "This call returns a C<uintptr_t>.\n";
+ | RUInt64 ->
+ pr "This call returns a counter.\n";
Hmmm. On one hand, I like that here we do tie the new type to counters.
On the other hand, the explanation doesn't match the generic name
"RUint64". Not sure what to suggest :/
| REnum { enum_prefix } ->
pr "This call returns one of the LIBNBD_%s_* values.\n" enum_prefix;
| RFlags { flag_prefix } ->
diff --git a/generator/GoLang.ml b/generator/GoLang.ml
index 73838199..eb7279a4 100644
--- a/generator/GoLang.ml
+++ b/generator/GoLang.ml
@@ -100,11 +100,12 @@ let
| RCookie -> Some "uint64"
| RSizeT -> Some "uint"
| RString -> Some "*string"
- (* RUInt | RUIntPtr returns (type, error) for consistency, but the
+ (* RUInt | RUIntPtr | RUInt64 returns (type, error) for consistency, but the
* error is always nil unless h is closed
*)
| RUInt -> Some "uint"
| RUIntPtr -> Some "uint"
+ | RUInt64 -> Some "uint64"
| REnum { enum_prefix } -> Some (camel_case enum_prefix)
| RFlags { flag_prefix } -> Some (camel_case flag_prefix)
@@ -112,7 +113,7 @@ let
| RErr -> None
| RBool -> Some "false"
| RStaticString | RString -> Some "nil"
- | RFd | RInt | RInt64 | RCookie | RSizeT | RUInt | RUIntPtr
+ | RFd | RInt | RInt64 | RCookie | RSizeT | RUInt | RUIntPtr | RUInt64
| REnum _ | RFlags _ -> Some "0"
let go_ret_c_errcode = function
@@ -120,7 +121,7 @@ let
| RStaticString -> Some "nil"
| RErr | RFd | RInt | RInt64 | RCookie | RSizeT -> Some "-1"
| RString -> Some "nil"
- | RUInt | RUIntPtr | REnum _ | RFlags _ -> None
+ | RUInt | RUIntPtr | RUInt64 | REnum _ | RFlags _ -> None
(* We need a wrapper around every function (except Close) to
* handle errors because cgo calls are sequence points and
@@ -400,6 +401,8 @@ let
pr " return uint (ret), nil\n"
| RUIntPtr ->
pr " return uint (ret), nil\n"
+ | RUInt64 ->
+ pr " return uint64 (ret), nil\n"
| REnum { enum_prefix } ->
pr " return %s (ret), nil\n" (camel_case enum_prefix)
| RFlags { flag_prefix } ->
diff --git a/generator/OCaml.ml b/generator/OCaml.ml
index c708d454..f2ca0d18 100644
--- a/generator/OCaml.ml
+++ b/generator/OCaml.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* nbd client library in userspace: generator
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -69,6 +69,7 @@ and
| RSizeT -> "int"
| RString -> "string"
| RUInt | RUIntPtr -> "int"
+ | RUInt64 -> "int64"
| REnum { enum_prefix } -> enum_prefix ^ ".t"
| RFlags { flag_prefix } -> flag_prefix ^ ".t list"
@@ -740,7 +741,7 @@ let
| RFd | RInt | RSizeT | RUInt | RUIntPtr -> pr " rv = Val_int (r);\n"
| 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"
+ | RInt64 | RCookie | RUInt64 -> pr " rv = caml_copy_int64 (r);\n"
| RStaticString -> pr " rv = caml_copy_string (r);\n"
| RString ->
pr " rv = caml_copy_string (r);\n";
diff --git a/generator/Python.ml b/generator/Python.ml
index cb89ccd7..449e1f9b 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -525,7 +525,7 @@ let
| RErr ->
pr " py_ret = Py_None;\n";
pr " Py_INCREF (py_ret);\n"
- | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr ->
+ | RFd | RInt | REnum _ | RFlags _ | RSizeT | RUInt | RUIntPtr | RUInt64 ->
pr " py_ret = PyLong_FromLong (ret);\n"
| RInt64 | RCookie ->
pr " py_ret = PyLong_FromLongLong (ret);\n"
This does not look right; I think RUInt64 belongs with RInt64 and
RCookie (so that we invoke PyLong_FromLongLong()).
Thanks
Laszlo