Re: [Libguestfs] [libnbd PATCH v4 02/25] generator: Add Extent64 arg type for upcoming use

On Fri, Aug 04, 2023 at 10:09:14AM +0100, Richard W.M. Jones wrote: > On Wed, Aug 02, 2023 at 08:50:22PM -0500, Eric Blake wrote: > > The existing nbd_block_status() callback is permanently stuck with an > > array of uint32_t pairs (each of the h->bs_count extents is > > represented by a pair of uint32_t; we have to pass bs_count*2 as the > > array size to the callback, and the user has to compute len/2 to > > determine how many extents to process). Furthermore, the 32-bit > > nature of both values adds some constraints: we cannot represent > > extents larger than 4G, and status flags must fit in 32 bits. While > > the "base:allocation" metacontext will never exceed 32 bits, it is not > > hard to envision other future extension metacontexts where a 64-bit > > status would be useful (for example, Zoned Block Devices expressing a > > 64-bit offset[1]). > > > > Exposing 64-bit extents will require a new API; we now have the > > decision of whether to copy the existing API practice of returning a > > bare array containing h->bs_count*2 raw uint64_t (where the user > > continues the same practice of pairing those off into extents), or > > going with a saner idea of an array of h->bs_count structs directly > > describing extents. Returning an array of structs has an advantage: > > although both items in the 64-bit extent are 64-bit values over the > > wire, they are conceptually different types (an extent length is > > inherently bounded by 63-bit off_t limitations; while the extent flags > > can be a full 64 bits of unsigned flags), and this difference in types > > can be more precisely represented in non-C languages. > > > > This patch starts the ball rolling by adding a new arg type for use in > > the generator, although all match statements that process args are > > minimally changed to merely assert that we aren't using the type yet. > > Then the next few patches can provide an implementation per language > > binding (for ease of review), prior to a final patch that finally adds > > the new nbd_block_status_64() API that actually takes advantage of the > > new type. > > > > [1] https://zonedstorage.io/docs/linux/zbd-api > > > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > > --- > > > > v4: improve commit message [Laszlo], split into several smaller > > patches [Laszlo] > > --- > > generator/API.mli | 1 + > > generator/API.ml | 12 +++++++++++- > > generator/C.ml | 7 +++++++ > > generator/GoLang.ml | 4 ++++ > > generator/OCaml.ml | 4 ++++ > > generator/Python.ml | 5 +++++ > > 6 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/generator/API.mli b/generator/API.mli > > index c5bba8c2..80633018 100644 > > --- a/generator/API.mli > > +++ b/generator/API.mli > > @@ -52,6 +52,7 @@ and > > | BytesPersistOut of string * string > > | Closure of closure (** function pointer + void *opaque *) > > | Enum of string * enum (** enum/union type, int in C *) > > +| Extent64 of string (** extent descriptor *) > > In the next patch this becomes: > > +| Extent64 of string (** extent descriptor, struct nbd_extent in C *) > > but neither is really helpful if you're just looking at this line of > code in isolation. Could we change the comment to say something like > this (no need to update it in the next patch): > > | Extent64 of string (** extent descriptor, > struct nbd_extent in C, > pair of 64 bit ints in other languages *) > > as that seems much clearer to me. Seems reasonable. Although maybe it would be more accurate as: (** extent descriptor, with 63-bit size and 64-bit flags; struct nbd_extent in C, tuple or pair in other languages *) to make it a bit more obvious that language bindings may give the size component a different type than the flags?