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 <ebl...@redhat.com>
> > ---
> >
> > 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?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
