[Libguestfs] [libnbd PATCH 2/4] generator: Add RUInt64 for 64-bit counters without error

Tue Sep 6 09:08:20 UTC 2022

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