Eric Blake
2022-Sep-02 22:14 UTC
[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
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). --- 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 } + | 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" | 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 _ -> () ); 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"; | 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" -- 2.37.2
Laszlo Ersek
2022-Sep-05 12:00 UTC
[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
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