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

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.