Re: [Libguestfs] [libnbd PATCH v3 07/22] generator: Add struct nbd_extent in prep for 64-bit extents

On Wed, Jun 07, 2023 at 04:23:27PM +0200, Laszlo Ersek wrote: I'll improve the wording here. I will be rebasing this on a pre-requisite patch where I introduce h->bs_count which contains the number of extents (bs_count * 2 * sizeof(uint{32,64}_t) gets the byte count conusmed over the wire for 32/64-bit replies). v4 will reword things based on this feedback, although probably not with the same language. Yes, quoting the code that is being temporarily added makes legibility nicer. ... > > +++ b/generator/C.ml > > @@ -94,6 +94,7 @@ let > > | Closure { cbname } -> > [ sprintf "%s_callback" cbname; sprintf "%s_user_data" cbname ] > > | Enum (n, _) -> [n] > > + | Extent64 n -> [n] > > | Fd n -> [n] > > | Flags (n, _) -> [n] > > | Int n -> [n] > > This extends "name_of_arg". ... > > + pr " */\n"; > > + pr "typedef struct {\n"; > > + pr " uint64_t length;\n"; > > + pr " uint64_t flags;\n"; > > + pr "} nbd_extent;\n"; > > + pr "\n"; > > print_closure_structs (); > > List.iter ( > > fun (name, { args; optargs; ret }) -> > The "nbd_" prefix for the new type seems consistent with existent > practice (e.g. "nbd_handle"). > > The placement of the new typedef is good as well, IMO; looking at the > generated library header, this typedef appears just before the callback > structures. Also in the generator's source code / context above, we can > see the call to "print_closure_structs" just after the insertion. That's > where our new "extent64_closure" is going to be handled too. > > The question is how we're going to translate "nbd_block_descriptor_ext" > to this new structure, in *C* code. If we populate the members > individually, together with big-endian-to-host-endian conversion, that > should be OK. I figure that belongs in a later patch though. Yes, the translation between wire format and the callback array will be in a later patch. That part of my series is already undergoing some of the biggest churn (you've convinced me in 8/22 that it is better to malloc two separate arrays: one for the bytes read over the wire, one for the array to send to the callback, in order to accomodate all four pairings of server wide/narrow response vs. caller 32/64-bit API without nasty in-place overwriting). I think I've hit the point where I should post v4 of all patches prior to this one and get them settled, while I continue rebasing how I want to express block status internally according to your review comments. I will double check which ones can be turned into unreachable assertions. ... Indeed, it is more than just C.ml that may get this treatment of using unreachable assertions more broadly. Reading ahead[1], I see you mention the idea of splitting this patch across languages. If I do that, then the first patch will add unreachable assertions EVERYWHERE, then later per-language patches will enable what it needs. The end result will be the same, but splitting the work across more patches may be easier to review, so I'm game to try it. > > > @@ -524,6 +528,18 @@ let > > copy(ret, s) > > return ret > > } > > + > > +func copy_extent_array (entries *C.nbd_extent, count C.size_t) []LibnbdExtent { > > + unsafePtr := unsafe.Pointer(entries) > > + arrayPtr := (*[1 << 20]C.nbd_extent)(unsafePtr) > > + slice := arrayPtr[:count:count] > > + ret := make([]LibnbdExtent, count) > > + for i := 0; i < int (count); i++ { > > + ret[i].Length = uint64 (slice[i].length) > > + ret[i].Flags = uint64 (slice[i].flags) > > + } > > + return ret > > +} > > "; > > > List.iter ( > > The pre-existent copy_uint32_array() function uses the hideous trick at > <https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices>;, > and needlessly so, IMO. > > - The trick is (a) hideous because it requires us to use arbitrary sizes > such as "1<<30". > > - The trick is (b) unnecessary because we don't intend to hang on to the > slice indefinitely. We only use it as a means to access the source > object. But at the noted URL, the trick is "being sold" with the pitch > "to create a Go slice backed by a C array (without copying the original > data)" -- and we copy the original data *anyway*! So it's better to use > pointer arithmetic IMO. Separate patch, but reasonable idea. I'm not very proficient in Go to know whether it has a specific style that I'm violating, or whether we value consistency with our C style, or anything else. But I have no qualms adjusting it as appropriate. > > - we cast "count" (a size_t in C) to a Go "int"; similarly the index > variable "i" has Go type "int". > > (8) So how about this instead (should be split in two: the first part > should update copy_uint32_array() in a separate patch, and the second > part should be squashed into this patch): > > > diff --git a/generator/GoLang.ml b/generator/GoLang.ml > > index 8922812b76a4..37b2240ef5bf 100644 > > --- a/generator/GoLang.ml > > @@ -521,22 +521,32 @@ let > > /* Closures. */ > > > func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 { > > - ret := make([]uint32, int (count)) > > - // See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices > > - // TODO: Use unsafe.Slice() when we require Go 1.17. > > - s := (*[1<<30]uint32)(unsafe.Pointer(entries))[:count:count] > > - copy(ret, s) > > + ret := make ([]uint32, count) > > + addr := uintptr (unsafe.Pointer (entries)) > > + var i uint64 = 0 > > + for i < uint64 (count) { > > + ptr := (*C.uint32_t)(unsafe.Pointer (addr)) > > + > > + ret[i] = uint32 (*ptr) > > + > > + addr += unsafe.Sizeof (*ptr) > > + i++ > > + } > > return ret > > } > > > func copy_extent_array (entries *C.nbd_extent, count C.size_t) []LibnbdExtent { > > - unsafePtr := unsafe.Pointer(entries) > > - arrayPtr := (*[1 << 20]C.nbd_extent)(unsafePtr) > > - slice := arrayPtr[:count:count] > > - ret := make([]LibnbdExtent, count) > > - for i := 0; i < int (count); i++ { > > - ret[i].Length = uint64 (slice[i].length) > > - ret[i].Flags = uint64 (slice[i].flags) > > + ret := make ([]LibnbdExtent, count) > > + addr := uintptr (unsafe.Pointer (entries)) > > + var i uint64 = 0 > > + for i < uint64 (count) { > > + ptr := (*C.nbd_extent)(unsafe.Pointer (addr)) > > + > > + ret[i].Length = uint64 ((*ptr).length) > > + ret[i].Flags = uint64 ((*ptr).flags) > > + > > + addr += unsafe.Sizeof (*ptr) > > + i++ > > } > > return ret > > } > > (This passes make and make-check, when the tree is checked out at your > patch#7, and this is applied on top.) Seems reasonable. > > > On 5/25/23 15:00, Eric Blake wrote: > > @@ -537,6 +553,8 @@ let > > match cbarg with > > | CBArrayAndLen (UInt32 n, _) -> > pr "%s []uint32" n; > > + | CBArrayAndLen (Extent64 n, _) -> > + pr "%s []LibnbdExtent" n; > > | CBBytesIn (n, len) -> > pr "%s []byte" n; > > | CBInt n -> > @@ -563,6 +581,8 @@ let > > match cbarg with > > | CBArrayAndLen (UInt32 n, count) -> > pr "%s *C.uint32_t, %s C.size_t" n count > > + | CBArrayAndLen (Extent64 n, count) -> > + pr "%s *C.nbd_extent, %s C.size_t" n count > > | CBBytesIn (n, len) -> > pr "%s unsafe.Pointer, %s C.size_t" n len > > | CBInt n -> > @@ -605,6 +625,8 @@ let > > match cbarg with > > | CBArrayAndLen (UInt32 n, count) -> > pr "copy_uint32_array (%s, %s)" n count > > + | CBArrayAndLen (Extent64 n, count) -> > + pr "copy_extent_array (%s, %s)" n count > > | CBBytesIn (n, len) -> > pr "C.GoBytes (%s, C.int (%s))" n len > > | CBInt n -> > @@ -756,6 +778,8 @@ let > > match cbarg with > > | CBArrayAndLen (UInt32 n, count) -> > pr "uint32_t *%s, size_t %s" n count > > + | CBArrayAndLen (Extent64 n, count) -> > + pr "nbd_extent *%s, size_t %s" n count > > | CBBytesIn (n, len) -> > pr "void *%s, size_t %s" n len > > | CBInt n -> > OK. > > Quoting the rest of the patch out of order, so I can leave the OCaml > stuff for the end (this is a *terribly* large patch, when the contexts > are increased suitably for reviewing, so I really wish we could split it > up somehow): [1] Yes, using unreachable notations everywhere (regardless of context), then doing per-language patching to enable the code that makes sense, should work. > > > diff --git a/golang/handle.go b/golang/handle.go > > index 5fe4ed4f..0b126005 100644 > > --- a/golang/handle.go > > +++ b/golang/handle.go > > @@ -58,6 +58,12 @@ func (h *Libnbd) String() string { > > return "&Libnbd{}" > > } > > > +/* Used for block status callback. */ > > +type LibnbdExtent struct { > > + Length uint64 // length of the extent > > + Flags uint64 // flags describing properties of the extent > > +} > > + > > /* All functions (except Close) return ([result,] LibnbdError). */ > > type LibnbdError struct { > > Op string // operation which failed > > OK. > > > diff --git a/generator/Python.ml b/generator/Python.ml > > index c81878de..ccac5fed 100644 > > --- a/generator/Python.ml > > +++ b/generator/Python.ml > > @@ -158,6 +158,7 @@ let > > let print_python_closure_wrapper { cbname; cbargs } = > > pr "/* Wrapper for %s callback. */\n" cbname; > > pr "static int\n"; > > + pr "__attribute__ ((unused)) /* XXX temporary hack */\n"; > > pr "%s_wrapper " cbname; > > C.print_cbarg_list ~wrap:true cbargs; > > pr "\n"; > > OK, this is as announced in the commit message. > > > @@ -170,6 +171,7 @@ let > > List.iter ( > > function > > | CBArrayAndLen (UInt32 n, _) > > + | CBArrayAndLen (Extent64 n, _) > > | CBBytesIn (n, _) > > | CBMutable (Int n) -> > pr " PyObject *py_%s = NULL;\n" n > > OK, this is responsible for generating: > > PyObject *py_entries = NULL; > > > > @@ -187,6 +189,16 @@ let > > pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n; > > pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; > > pr " }\n" > > + | CBArrayAndLen (Extent64 n, len) -> > + pr " py_%s = PyList_New (%s);\n" n len; > > + pr " size_t i_%s;\n" n; > > (9) Under "CBArrayAndLen (UInt32 n, len)", we also check whether the New > call succeeds; I think it should not be omitted here either: > > pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n; Good catch. Indeed. Will fix. If PyLong_FromUnsignedLongLong() returns NULL, it set an error. I'll double-check the Py_BuildValue() docs; I seem to recall that I could use something other than "OO" as the format string in order to pass in C values directly, rather than having to pre-construct intermediate python objects myself, and doing that would be even better at avoiding leak issues. (Trimming the rest of your Python observations until I can focus on that aspect later, as they may be rendered moot if I can do the conversion smarter in the first place) ... > > +++ b/ocaml/nbd-c.h > > @@ -62,6 +62,7 @@ extern void nbd_internal_ocaml_raise_closed (const char *func) Noreturn; > > > extern const char **nbd_internal_ocaml_string_list (value); > > extern value nbd_internal_ocaml_alloc_i64_from_u32_array (uint32_t *, size_t); > > +extern value nbd_internal_ocaml_alloc_extent64_array (nbd_extent *, size_t); > > extern void nbd_internal_unix_sockaddr_to_sa (value, struct sockaddr_storage *, > > socklen_t *); > > extern void nbd_internal_ocaml_exception_in_wrapper (const char *, value); > > OK. > > > diff --git a/generator/OCaml.ml b/generator/OCaml.ml > > index edb81f25..ea3b044a 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 _ -> "extent" > > | 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 n -> n > > | Fd n -> n > > | Flags (n, _) -> n > > | Int n -> n > > (14) Could we use "assert false" with these too? Same as before. I'm sold on the idea of the intro patch using assert false everywhere, then the per-language followup patches adding code only for what is needed. > > > @@ -147,6 +149,9 @@ let > > > type cookie = int64 > > > +type extent = int64 * int64 > > +(** Length and flags of an extent in block_status_64 callback. *) > > + > > "; > > > List.iter ( > > (15) Can we add a note here that negative values are not permitted? (See > my further comments below.) Yes. Hmm, right now, I think we're implicitly relying on a server never exceeding off_t in its advertised size. I have intentions to actually formalize that in the NBD spec, but have not done so yet; so I guess there is the theoretical case of a server that advertises an unsigned 64-bit size and makes libnbd blow up where we don't expect that. I should probably prepare a pre-requisite patch where the state machine transfers to DEAD if the server ever advertises a too-large size, so that this code can then assert (rather than just comment) that we aren't dealing with negative values. > > > @@ -268,6 +273,7 @@ let > > exception Error of string * Unix.error option > > exception Closed of string > > type cookie = int64 > > +type extent = int64 * int64 > > > (* Give the exceptions names so that they can be raised from the C code. *) > > let () = > > @@ -498,7 +504,8 @@ let > > let argnames = > > List.map ( > > function > > - | CBArrayAndLen (UInt32 n, _) | CBBytesIn (n, _) > > + | CBArrayAndLen (UInt32 n, _) | CBArrayAndLen (Extent64 n, _) > > + | CBBytesIn (n, _) > > | CBInt n | CBInt64 n > > | CBMutable (Int n) | CBString n | CBUInt n | CBUInt64 n -> > n ^ "v" > > This is inside "print_ocaml_closure_wrapper"; OK. > > (16) You can contract the new pattern as follows: > > | CBArrayAndLen ((UInt32 n | Extent64 n), _) Cool; I'm still polishing my OCaml skills. > > > @@ -531,6 +538,14 @@ let > > pr "%s %s,\n" indent n; > > pr "%s %s\n" indent count; > > pr "%s);\n" indent > > + | CBArrayAndLen (Extent64 n, count) -> > + pr " %sv = " n; > > + let fncol = output_column () in > > + let indent = spaces fncol in > > + pr "nbd_internal_ocaml_alloc_extent64_array (\n"; > > + pr "%s %s,\n" indent n; > > + pr "%s %s\n" indent count; > > + pr "%s);\n" indent > > | CBBytesIn (n, len) -> > pr " %sv = caml_alloc_initialized_string (%s, %s);\n" n len n > > | CBInt n | CBUInt n -> > Seems OK superficially; matches the 'CBArrayAndLen (UInt32 n, count)' > case, with only the helper function differing. > > > @@ -554,7 +569,7 @@ let > > > List.iter ( > > function > > - | CBArrayAndLen (UInt32 _, _) > > + | CBArrayAndLen (_, _) > > | CBBytesIn _ > > | CBInt _ > > | CBInt64 _ > > @@ -563,7 +578,7 @@ let > > | CBUInt64 _ -> () > > | CBMutable (Int n) -> > pr " *%s = Int_val (Field (%sv, 0));\n" n n > > - | CBArrayAndLen _ | CBMutable _ -> assert false > > + | CBMutable _ -> assert false > > ) cbargs; > > > pr " if (Is_exception_result (rv)) {\n"; > > (17) We're still inside "print_ocaml_closure_wrapper", so I think the > patterns here should follow the patterns seen in the "argnames" > definition: > > | CBArrayAndLen (UInt32 n, _) | CBArrayAndLen (Extent64 n, _) > > or even better (again) > > | CBArrayAndLen ((UInt32 _ | Extent64 _), _) > > IOW I think "CBArrayAndLen (_, _)" is too generic / inconsistent with > previously added code. We should handle the Extent64 constructor, and > not touch the "CBArrayAndLen _" fallback. Fair enough. I was relying on the compiler telling me that it didn't encounter any uncovered branches, but using a consistent style (even if it means longer source code) doesn't hurt. > > > @@ -578,6 +593,7 @@ let > > pr "}\n"; > > pr "\n"; > > pr "static int\n"; > > + pr "__attribute__ ((unused)) /* XXX temporary hack */\n"; > > pr "%s_wrapper " cbname; > > C.print_cbarg_list ~wrap:true cbargs; > > pr "\n"; > > OK, announced in the commit message. > > > @@ -695,6 +711,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 > > OK, this is in "print_ocaml_binding". > > > @@ -780,6 +797,7 @@ let > > | BytesPersistOut _ > > | Closure _ > > | Enum _ > > + | Extent64 _ > > | Fd _ > > | Flags _ > > | Int _ > > This is still in "print_ocaml_binding", under comment "Any parameters > which need to be freed". > > (18) For consistency I'd suggest "assert false" here as well. > > > diff --git a/ocaml/helpers.c b/ocaml/helpers.c > > index 3361a696..09666daf 100644 > > --- a/ocaml/helpers.c > > +++ b/ocaml/helpers.c > > @@ -133,6 +133,26 @@ nbd_internal_ocaml_alloc_i64_from_u32_array (uint32_t *a, size_t len) > > CAMLreturn (rv); > > } > > > +value > > +nbd_internal_ocaml_alloc_extent64_array (nbd_extent *a, size_t len) > > +{ > > + CAMLparam0 (); > > + CAMLlocal3 (s, v, rv); > > + size_t i; > > + > > + rv = caml_alloc (len, 0); > > + for (i = 0; i < len; ++i) { > > + s = caml_alloc (2, 0); > > + v = caml_copy_int64 (a[i].length); > > + Store_field (s, 0, v); > > + v = caml_copy_int64 (a[i].flags); > > + Store_field (s, 1, v); > > + Store_field (rv, i, s); > > + } > > + > > + CAMLreturn (rv); > > +} > > + > > /* Convert a Unix.sockaddr to a C struct sockaddr. */ > > void > > nbd_internal_unix_sockaddr_to_sa (value sockaddrv, > > (19) I'd suggest the following addition: > > > diff --git a/ocaml/helpers.c b/ocaml/helpers.c > > index 09666dafa7d1..db652943141d 100644 > > --- a/ocaml/helpers.c > > +++ b/ocaml/helpers.c > > @@ -25,6 +25,7 @@ > > #include <string.h> > #include <sys/socket.h> > #include <assert.h> > +#include <inttypes.h> > > #include <caml/alloc.h> > #include <caml/callback.h> > @@ -140,6 +141,16 @@ nbd_internal_ocaml_alloc_extent64_array (nbd_extent *a, size_t len) > > CAMLlocal3 (s, v, rv); > > size_t i; > > > + for (i = 0; i < len; ++i) > > + if (a[i].length > INT64_MAX || a[i].flags > INT64_MAX) { > > + char errmsg[256]; > > + > > + snprintf (errmsg, sizeof errmsg, > > + "%s: extent[%zu] = { .length = %"PRIu64", .flags = %"PRIu64"}", > > + __func__, i, a[i].length, a[i].flags); > > + caml_failwith (errmsg); > > + } For a[i].length, I think I can just use an assertion, instead of caml_failwith(), if I can guarantee with a pre-requisite patch that we will never proceed with a server that sends larger than a 63-bit size. For a[i].flags, things are trickier (as the client really CAN send a 64-bit value). Maybe this is a question for Rich on whether I've picked the best OCaml types. > > + > > rv = caml_alloc (len, 0); > > for (i = 0; i < len; ++i) { > > s = caml_alloc (2, 0); > > *However*, considering the nbd_internal_ocaml_alloc_extent64_array() > call site, in the generated extent64_wrapper_locked() function > [ocaml/nbd-c.c]: > > > /* Wrapper for extent64 callback. */ > > static int > > extent64_wrapper_locked (void *user_data, const char *metacontext, > > uint64_t offset, nbd_extent *entries, > > size_t nr_entries, int *error) > > { > > CAMLparam0 (); > > CAMLlocal4 (metacontextv, offsetv, entriesv, errorv); > > CAMLlocal2 (exn, rv); > > const struct user_data *data = user_data; > > int r; > > value args[4]; > > > metacontextv = caml_copy_string (metacontext); > > offsetv = caml_copy_int64 (offset); > > entriesv = nbd_internal_ocaml_alloc_extent64_array ( > > entries, > > nr_entries > > ); > > errorv = caml_alloc_tuple (1); > > Store_field (errorv, 0, Val_int (*error)); > > args[0] = metacontextv; > > args[1] = offsetv; > > args[2] = entriesv; > > args[3] = errorv; > > rv = caml_callbackN_exn (data->fnv, 4, args); > > *error = Int_val (Field (errorv, 0)); > > if (Is_exception_result (rv)) { > > nbd_internal_ocaml_exception_in_wrapper ("extent64", rv); > > CAMLreturnT (int, -1); > > } > > > r = Int_val (rv); > > assert (r >= 0); > > CAMLreturnT (int, r); > > } > > I'm not sure if raising an OCaml exception like this, in an *inner* C > function, is appropriate. caml_failwith() may only be suitable for C > functions *directly* called by the OCaml runtime. > > If that's the case, then we should propagate the error from > nbd_internal_ocaml_alloc_extent64_array() differently. We might have to > change the signature of nbd_internal_ocaml_alloc_extent64_array(), and > then the generated call site (i.e., extent64_wrapper_locked()) must take > a potential failure into account. > > This is really hard to reason about, as the only "prior art", namely > extent_wrapper_locked() -> nbd_internal_ocaml_alloc_i64_from_u32_array(), does not face such a > conundrum. There we took the easy way: we convert from uint32_t to > OCaml's int64, which cannot overflow. My intent (although we may still need to get the OCaml-specific split patch to match that) is that length will always fit in OCaml's int64 (because it is a 63-bit value); while flags needs all 64 bits represented (whether signed or unsigned may depend on how easy or hard it is to do bit-twiddling on a 64-bit value in OCaml). -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org