[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
Laszlo Ersek
lersek at redhat.com
Mon Sep 5 12:00:22 UTC 2022
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
More information about the Libguestfs
mailing list