Richard W.M. Jones
2023-Aug-04 09:09 UTC
[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 at 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. With this change: Reviewed-by: Richard W.M. Jones <rjones at redhat.com> Rich.> | Fd of string (** file descriptor *) > | Flags of string * flags (** flags, uint32_t in C *) > | Int of string (** small int *) > diff --git a/generator/API.ml b/generator/API.ml > index 72c81657..1fbf147b 100644 > --- a/generator/API.ml > +++ b/generator/API.ml > @@ -42,6 +42,7 @@ > | BytesPersistOut of string * string > | Closure of closure > | Enum of string * enum > +| Extent64 of string > | Fd of string > | Flags of string * flags > | Int of string > @@ -157,6 +158,14 @@ let extent_closure > "nr_entries"); > CBMutable (Int "error") ] > } > +let extent64_closure = { > + cbname = "extent64"; > + cbargs = [ CBString "metacontext"; > + CBUInt64 "offset"; > + CBArrayAndLen (Extent64 "entries", > + "nr_entries"); > + CBMutable (Int "error") ] > +} > let list_closure = { > cbname = "list"; > cbargs = [ CBString "name"; CBString "description" ] > @@ -166,7 +175,8 @@ let context_closure > cbargs = [ CBString "name" ] > } > let all_closures = [ chunk_closure; completion_closure; > - debug_closure; extent_closure; list_closure; > + debug_closure; extent_closure; (* extent64_closure; *) > + list_closure; > context_closure ] > > (* Enums. *) > diff --git a/generator/C.ml b/generator/C.ml > index a2670f70..606ba1e0 100644 > --- a/generator/C.ml > +++ b/generator/C.ml > @@ -94,6 +94,7 @@ let > | Closure { cbname } -> > [ sprintf "%s_callback" cbname; sprintf "%s_user_data" cbname ] > | Enum (n, _) -> [n] > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd n -> [n] > | Flags (n, _) -> [n] > | Int n -> [n] > @@ -132,6 +133,7 @@ let > | Bool _ | Closure _ | Enum _ | Fd _ | Flags _ > | Int _ | Int64 _ | SizeT _ > | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ] > + | Extent64 _ -> assert false (* only used in extent64_closure *) > > let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ] > > @@ -191,6 +193,7 @@ and > | Enum (n, _) -> > if types then pr "int "; > pr "%s" n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags (n, _) -> > if types then pr "uint32_t "; > pr "%s" n > @@ -751,6 +754,7 @@ let > | Bool _ | Closure _ | Enum _ | Flags _ | Fd _ | Int _ > | Int64 _ | SizeT _ > | SockAddrAndLen _ | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> () > + | Extent64 _ -> assert false (* only used in extent64_closure *) > ) args; > let indent > if may_set_error then ( > @@ -774,6 +778,7 @@ let > pr " %s=\\\"%%s\\\" %s=%%zu" n count > | Closure { cbname } -> pr " %s=%%s" cbname > | Enum (n, _) -> pr " %s=%%d" n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags (n, _) -> pr " %s=0x%%x" n > | Fd n | Int n -> pr " %s=%%d" n > | Int64 n -> pr " %s=%%\"PRIi64\"" n > @@ -812,6 +817,7 @@ let > pr "%s_printable ? %s_printable : \"\", %s" n n count > | Closure _ -> pr "\"<fun>\"" > | Enum (n, _) -> pr "%s" n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags (n, _) -> pr "%s" n > | Fd n | Int n | Int64 n | SizeT n -> pr "%s" n > | SockAddrAndLen (_, len) -> pr "(int) %s" len > @@ -847,6 +853,7 @@ let > | Bool _ | Closure _ | Enum _ | Flags _ | Fd _ | Int _ > | Int64 _ | SizeT _ > | SockAddrAndLen _ | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> () > + | Extent64 _ -> assert false (* only used in extent64_closure *) > ) args; > pr " }\n" > (* Print the trace when we leave a call with debugging enabled. *) > diff --git a/generator/GoLang.ml b/generator/GoLang.ml > index 8a19f65a..73df5254 100644 > --- a/generator/GoLang.ml > +++ b/generator/GoLang.ml > @@ -38,6 +38,7 @@ let > | BytesPersistOut (n, len) -> n > | Closure { cbname } -> cbname > | Enum (n, _) -> n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd n -> n > | Flags (n, _) -> n > | Int n -> n > @@ -60,6 +61,7 @@ let > | BytesPersistOut _ -> "AioBuffer" > | Closure { cbname } -> sprintf "%sCallback" (camel_case cbname) > | Enum (_, { enum_prefix }) -> camel_case enum_prefix > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd _ -> "int" > | Flags (_, { flag_prefix }) -> camel_case flag_prefix > | Int _ -> "int" > @@ -252,6 +254,7 @@ let > pr " c_%s.user_data = C.alloc_cbid(C.long(%s_cbid))\n" cbname cbname > | Enum (n, _) -> > pr " c_%s := C.int(%s)\n" n n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd n -> > pr " c_%s := C.int(%s)\n" n n > | Flags (n, _) -> > @@ -324,6 +327,7 @@ let > | BytesPersistOut (n, len) -> pr ", c_%s, c_%s" n len > | Closure { cbname } -> pr ", c_%s" cbname > | Enum (n, _) -> pr ", c_%s" n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd n -> pr ", c_%s" n > | Flags (n, _) -> pr ", c_%s" n > | Int n -> pr ", c_%s" n > diff --git a/generator/OCaml.ml b/generator/OCaml.ml > index efc1af22..7971ac40 100644 > --- a/generator/OCaml.ml > +++ b/generator/OCaml.ml > @@ -44,6 +44,7 @@ and > | Closure { cbargs } -> > sprintf "(%s)" (ocaml_closuredecl_to_string cbargs) > | Enum (_, { enum_prefix }) -> sprintf "%s.t" enum_prefix > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd _ -> "Unix.file_descr" > | Flags (_, { flag_prefix }) -> sprintf "%s.t list" flag_prefix > | Int _ -> "int" > @@ -102,6 +103,7 @@ let > | BytesPersistOut (n, len) -> n > | Closure { cbname } -> cbname > | Enum (n, _) -> n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd n -> n > | Flags (n, _) -> n > | Int n -> n > @@ -695,6 +697,7 @@ let > pr " %s_callback.free = free_user_data;\n" cbname > | Enum (n, { enum_prefix }) -> > pr " int %s = %s_val (%sv);\n" n enum_prefix n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Fd n -> > pr " /* OCaml Unix.file_descr is just an int, at least on Unix. */\n"; > pr " int %s = Int_val (%sv);\n" n n > @@ -792,6 +795,7 @@ let > | UInt32 _ > | UInt64 _ > | UIntPtr _ -> () > + | Extent64 _ -> assert false (* only used in extent64_closure *) > ) args; > > pr " CAMLreturn (rv);\n"; > diff --git a/generator/Python.ml b/generator/Python.ml > index beaeaa4c..3a77b81f 100644 > --- a/generator/Python.ml > +++ b/generator/Python.ml > @@ -307,6 +307,7 @@ let > pr ".callback = %s_wrapper, .free = free_user_data" cbname); > pr " };\n" > | Enum (n, _) -> pr " int %s;\n" n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags (n, _) -> > pr " uint32_t %s_u32;\n" n; > pr " unsigned int %s; /* really uint32_t */\n" n > @@ -363,6 +364,7 @@ let > "O", sprintf "&%s" n, sprintf "py_%s->buf, py_%s->len" n n > | Closure { cbname } -> "O", sprintf "&py_%s_fn" cbname, cbname > | Enum (n, _) -> "i", sprintf "&%s" n, n > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags (n, _) -> "I", sprintf "&%s" n, sprintf "%s_u32" n > | Fd n | Int n -> "i", sprintf "&%s" n, n > | Int64 n -> "L", sprintf "&%s" n, sprintf "%s_i64" n > @@ -454,6 +456,7 @@ let > pr " if (!chunk_user_data->view) goto out;\n" > ) > | Enum _ -> () > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags (n, _) -> pr " %s_u32 = %s;\n" n n > | Fd _ | Int _ -> () > | Int64 n -> pr " %s_i64 = %s;\n" n n > @@ -552,6 +555,7 @@ let > | Closure { cbname } -> > pr " free_user_data (%s_user_data);\n" cbname > | Enum _ -> () > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags _ -> () > | Fd _ | Int _ -> () > | Int64 _ -> () > @@ -882,6 +886,7 @@ let > | BytesPersistOut (n, _) -> n, None > | Closure { cbname } -> cbname, None > | Enum (n, _) -> n, None > + | Extent64 _ -> assert false (* only used in extent64_closure *) > | Flags (n, _) -> n, None > | Fd n | Int n -> n, None > | Int64 n -> n, None > -- > 2.41.0 > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Eric Blake
2023-Aug-04 15:11 UTC
[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 at 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