Eric Blake
2022-Sep-06 09:08 UTC
[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
On Mon, Sep 05, 2022 at 02:00:22PM +0200, Laszlo Ersek wrote:> 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.And I did have a comment about RUInt64 being a counter in API.mli. ...> > @@ -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?Yes. I failed to call that out, but the commit is already in now. Missed in commit 85798518.> > +++ b/generator/C.ml > > @@ -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"Here, it was more of a consistency issue. Inserting a bare "" during string constant concatenation looks funny; we already omitted it in RUIntPtr (again, 85798518), and I liked that style better for RUInt64, so I copied it to RInt64 and RCookie. We could further compress things to have: pr " debug (h, \"leave: ret=\""; (match ret with | RBool -> pr "%%d\", ret" and so on, to get rid of yet another spurious pair of "" in the generated C code (since the return format is always exacty one argument, there's no need to have the generated api.c have "... ret=" "%<>" when "... ret=%<>" would do). I can touch that up in a followup, if it makes sense.> > ); > > 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.Actually, this line rearranged RUIntPtr to be earlier in the line (we don't have a strong precedent on whether branches to the match should be in alphabetical, declaration, or abitrary order).> > > ); > > 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 :/If we later find ourselves needing an actual 64-bit unsigned value, unrelated to counters, we can add RCounter at that point and repurpose RUInt64 to the new purpose. We've done that sort of enum splitting before; the enum names are less important that the underlying abstract type that then has to be ported to all the language bindings for the semantics it will be representing.> > +++ 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()).D'oh. Actual bug; now fixed as followup commit f79a1ac1 Thanks for reviewing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2022-Sep-08 12:53 UTC
[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
On 09/06/22 11:08, Eric Blake wrote:> On Mon, Sep 05, 2022 at 02:00:22PM +0200, Laszlo Ersek wrote:> We could further compress things to have: > > pr " debug (h, \"leave: ret=\""; > (match ret with > | RBool -> pr "%%d\", ret" > > and so on, to get rid of yet another spurious pair of "" in the > generated C code (since the return format is always exacty one > argument, there's no need to have the generated api.c have > "... ret=" "%<>" when "... ret=%<>" would do). I can touch that > up in a followup, if it makes sense.It seems to simplify the generated code (which we sometimes quote in commit messages, when patching the generator), so if it's not a big burden, I'd be in favor of it.> >>> ); >>> 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. > > Actually, this line rearranged RUIntPtr to be earlier in the line (we > don't have a strong precedent on whether branches to the match should > be in alphabetical, declaration, or abitrary order).Yup, sorry. The reordering, combined with the insertion, threw me off.> >> >>> ); >>> 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 :/ > > If we later find ourselves needing an actual 64-bit unsigned value, > unrelated to counters, we can add RCounter at that point and > repurpose RUInt64 to the new purpose. We've done that sort of enum > splitting before; the enum names are less important that the > underlying abstract type that then has to be ported to all the > language bindings for the semantics it will be representing.OK.> >>> +++ 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()). > > D'oh. Actual bug; now fixed as followup commit f79a1ac1 > > Thanks for reviewing. >Cheers Laszlo
Richard W.M. Jones
2022-Sep-09 11:55 UTC
[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error
On Tue, Sep 06, 2022 at 04:08:20AM -0500, Eric Blake wrote:> If we later find ourselves needing an actual 64-bit unsigned value, > unrelated to counters, we can add RCounter at that point and > repurpose RUInt64 to the new purpose. We've done that sort of enum > splitting before; the enum names are less important that the > underlying abstract type that then has to be ported to all the > language bindings for the semantics it will be representing.Right - the generator can change as much as we like. What matters is the C API/ABI. Less so the OCaml API: there's no guarantee for that, and we've changed that a bit over time, we just not to break it for no good reason. Rich. -- 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