For 32-bit block status, we were able to cheat and use an array with an odd number of elements, with array[0] holding the context id, and passing &array[1] to the user's callback. But once we have 64-bit extents, we can no longer abuse array element 0 like that, for two reasons: 64-bit extents contain uint64_t which might not be alignment-compatible with an array of uint32_t on all architectures, and the new NBD_REPLY_TYPE_BLOCK_STATUS_EXT adds an additional count field before the array. Split out a new state STRUCTURED_REPLY.BS_HEADER to receive the context id (and eventually the new count field for 64-bit replies) separately from the extents array, and add another structured_reply type in the payload section for tracking it. No behavioral change, other than the rare possibility of landing in the new state. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- lib/internal.h | 1 + lib/nbd-protocol.h | 17 +++++---- generator/state_machine.ml | 9 ++++- generator/states-reply-structured.c | 56 ++++++++++++++++++++--------- 4 files changed, 60 insertions(+), 23 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index b155681d..25eeea34 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -242,6 +242,7 @@ struct nbd_handle { union { struct nbd_structured_reply_offset_data offset_data; struct nbd_structured_reply_offset_hole offset_hole; + struct nbd_structured_reply_block_status_hdr bs_hdr; struct { struct nbd_structured_reply_error error; char msg[NBD_MAX_STRING]; /* Common to all error types */ diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h index 0217891e..f4640a98 100644 --- a/lib/nbd-protocol.h +++ b/lib/nbd-protocol.h @@ -182,12 +182,6 @@ struct nbd_fixed_new_option_reply_meta_context { /* followed by a string */ } NBD_ATTRIBUTE_PACKED; -/* NBD_REPLY_TYPE_BLOCK_STATUS block descriptor. */ -struct nbd_block_descriptor { - uint32_t length; /* length of block */ - uint32_t status_flags; /* block type (hole etc) */ -} NBD_ATTRIBUTE_PACKED; - /* Request (client -> server). */ struct nbd_request { uint32_t magic; /* NBD_REQUEST_MAGIC. */ @@ -224,6 +218,17 @@ struct nbd_structured_reply_offset_hole { uint32_t length; /* Length of hole. */ } NBD_ATTRIBUTE_PACKED; +/* NBD_REPLY_TYPE_BLOCK_STATUS block descriptor. */ +struct nbd_block_descriptor { + uint32_t length; /* length of block */ + uint32_t status_flags; /* block type (hole etc) */ +} NBD_ATTRIBUTE_PACKED; + +struct nbd_structured_reply_block_status_hdr { + uint32_t context_id; /* metadata context ID */ + /* followed by array of nbd_block_descriptor extents */ +} NBD_ATTRIBUTE_PACKED; + struct nbd_structured_reply_error { uint32_t error; /* NBD_E* error number */ uint16_t len; /* Length of human readable error. */ diff --git a/generator/state_machine.ml b/generator/state_machine.ml index 126159b9..1f0d00b0 100644 --- a/generator/state_machine.ml +++ b/generator/state_machine.ml @@ -871,10 +871,17 @@ and external_events = []; }; + State { + default_state with + name = "RECV_BS_HEADER"; + comment = "Receive header of structured reply block-status payload"; + external_events = []; + }; + State { default_state with name = "RECV_BS_ENTRIES"; - comment = "Receive a structured reply block-status payload"; + comment = "Receive entries array of structured reply block-status payload"; external_events = []; }; diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 5aca7262..96182222 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -126,19 +126,10 @@ REPLY.STRUCTURED_REPLY.CHECK: length < 12 || ((length-4) & 7) != 0) goto resync; assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); - /* We read the context ID followed by all the entries into a - * single array and deal with it at the end. - */ - free (h->bs_entries); - h->bs_entries = malloc (length); - if (h->bs_entries == NULL) { - SET_NEXT_STATE (%.DEAD); - set_error (errno, "malloc"); - break; - } - h->rbuf = h->bs_entries; - h->rlen = length; - SET_NEXT_STATE (%RECV_BS_ENTRIES); + /* Start by reading the context ID. */ + h->rbuf = &h->sbuf.sr.payload.bs_hdr; + h->rlen = sizeof h->sbuf.sr.payload.bs_hdr; + SET_NEXT_STATE (%RECV_BS_HEADER); break; default: @@ -424,9 +415,41 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_HOLE: } return 0; + REPLY.STRUCTURED_REPLY.RECV_BS_HEADER: + struct command *cmd = h->reply_cmd; + uint32_t length; + + switch (recv_into_rbuf (h)) { + case -1: SET_NEXT_STATE (%.DEAD); return 0; + case 1: + save_reply_state (h); + SET_NEXT_STATE (%.READY); + return 0; + case 0: + length = be32toh (h->sbuf.sr.structured_reply.length); + + assert (cmd); /* guaranteed by CHECK */ + assert (cmd->type == NBD_CMD_BLOCK_STATUS); + assert (length >= 12); + length -= sizeof h->sbuf.sr.payload.bs_hdr; + + free (h->bs_entries); + h->bs_entries = malloc (length); + if (h->bs_entries == NULL) { + SET_NEXT_STATE (%.DEAD); + set_error (errno, "malloc"); + return 0; + } + h->rbuf = h->bs_entries; + h->rlen = length; + SET_NEXT_STATE (%RECV_BS_ENTRIES); + } + return 0; + REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES: struct command *cmd = h->reply_cmd; uint32_t length; + uint32_t count; size_t i; uint32_t context_id; @@ -445,15 +468,16 @@ REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES: assert (h->bs_entries); assert (length >= 12); assert (h->meta_valid); + count = (length - sizeof h->sbuf.sr.payload.bs_hdr) / sizeof *h->bs_entries; /* Need to byte-swap the entries returned, but apart from that we * don't validate them. */ - for (i = 0; i < length/4; ++i) + for (i = 0; i < count; ++i) h->bs_entries[i] = be32toh (h->bs_entries[i]); /* Look up the context ID. */ - context_id = h->bs_entries[0]; + context_id = be32toh (h->sbuf.sr.payload.bs_hdr.context_id); for (i = 0; i < h->meta_contexts.len; ++i) if (context_id == h->meta_contexts.ptr[i].context_id) break; @@ -464,7 +488,7 @@ REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES: if (CALL_CALLBACK (cmd->cb.fn.extent, h->meta_contexts.ptr[i].name, cmd->offset, - &h->bs_entries[1], (length-4) / 4, + h->bs_entries, count, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; -- 2.40.1

On Thu, May 25, 2023 at 06:30:37PM +0200, Laszlo Ersek wrote: And when 64-bit replies are added, there's a counterpart that contains a header (4+4 bytes) and then a positive integral number of block descriptors (16 bytes each). Likewise, once 64-bit replies are added, we will depend on (h->len/8) being an *odd* integer >= 3, but additionally that the count in the header match the (h->len-8)/16 computation. This is only valid for the 32-bit reply; but could easily be generalized to cover the 64-bit reply as well. I definitely like wrapping the computation behind a helper function, so that we don't have to open-code repeat it in multiple spots. I could even go with doing this sort of cleanup as its own prerequisite patch instead of squashing it in with this one, because.... ...that does indeed look easier to read. That static assertion also makes sense. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On 5/25/23 15:00, Eric Blake wrote: This makes sense, and the following, strictly related code change in the patch body corresponds to the explanation (except for the "__attribute__ ((packed))" removal, but more on that later): - struct nbd_simple_reply simple_reply; struct { - struct nbd_structured_reply structured_reply; + union { + struct nbd_simple_reply simple; + struct nbd_structured_reply structured; + } hdr; union { struct nbd_structured_reply_offset_data offset_data; struct nbd_structured_reply_offset_hole offset_hole; struct nbd_structured_reply_block_status_hdr bs_hdr; struct { struct nbd_structured_reply_error error; char msg[NBD_MAX_STRING]; /* Common to all error types */ uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */ } __attribute__ ((packed)) error; } payload; - } __attribute__ ((packed)) sr; + } reply; One interesting consequence is that the payload union now becomes possible after the simple header, which IIUC makes no sense per NBD spec. But, it doesn't hurt either. At the same time: I've been staring at this part for an extremely long time, and I just can't make any sense of it. My (strongly held) opinion is that this part of the patch should be split off to a separate patch. Here's a possible interpretation: - The "sbuf.or" ("option reply") member of the "sbuf" union is a structure. It has two members: "sbuf.or.option_reply" (another structure) and "sbuf.or.payload" (a union). We never read into "sbuf.or.option_reply" and "sbuf.or.payload" at the same time (i.e., we never cross the structure field boundary between them, with a single recv operation), therefore we can remove the "packed" attribute from "sbuf.or". This permits the compiler to insert padding between "sbuf.or.option_reply" and "sbuf.or.payload", and/or after "sbuf.or". - Similarly, the "sbuf.sr" ("structured reply") member of the "sbuf" union is a structure. It has two members: "sbuf.sr.structured_reply" (a structure) and "sbuf.sr.payload" (a union). We never read into "sbuf.sr.structured_reply" and "sbuf.sr.payload" at the same time (i.e., we never cross the structure field boundary between them, with a single recv operation), therefore we can remove the "packed" attribute from "sbuf.sr". This permits the compiler to insert padding between "sbuf.sr.structured_reply" and "sbuf.sr.payload", and/or after "sbuf.sr". In turn, my problem with this interpretation is that, if we never cross the struct member boundary within "sbuf.or" (i.e., between "option_reply" and "payload"), and similarly we never cross the struct member boundary within "sbuf.sr" (i.e., between "structured_reply" and "payload"), then *why* are "sbuf.or" and "sbuf.sr" structures in the first place (and not unions)? Now, I can imagine that the answer sounds like this: indeed we don't cross those field boundaries with a single recv, however we still need the data from both fields *at the same time*, later on. And this certainly sounds like a valid explanation, and dropping the "packed" attributes is fine / justified as well, under this interpreation, but it absolutely must be a separate patch, IMO. I think I've spent at least half an hour explaining it to myself. Whitespace fixup, tolerable as a part of this patch. So I disagree with doing this here. I'd like it to be its own patch. Yes, fine (although formatting it with a much larger context would have helped -- anyway I'm reviewing the series with -W in my local clone). Here too, the __attribute__ ((packed)) removal shoould be in a separate patch. Renaming "sr" to "reply" is "in scope", of course. Yes. Yes; same thing, just different name. Here I disagree with the mechanical approach. I even take issue with the pre-patch code. Pre-patch, we fill in "h->sbuf.simple_reply" (in "generator/states-reply.c"), i.e., one member of the top-level "sbuf" union, but then continue filling a member of a *different* member (i.e., "sr.structured_reply") of the "sbuf" union here. This looks undefined by the C standard, but even if it's not undefined, it's supremely confusing. Now, if structure "nbd_simple_reply" were the first member of "nbd_structured_reply", maybe we could salvage this somehow -- but that's not the case. ... Right, in "generator/states-reply.c", we have: /* We read all replies initially as if they are simple replies, but * check the magic in CHECK_SIMPLE_OR_STRUCTURED_REPLY below. * This works because the structured_reply header is larger. */ This is precisely my problem. The common initial subsequence is only the "uint32_t magic" field. Therefore the code in "generator/states-reply.c" should only read the magic field at first, and then branch to simple vs. structured reply. Post-patch, the sbuf.reply.hdr union actually reflects this well! The structure members of that union all begin with the common subsequence "uint32_t magic", so using the special guarantee from the C standard, it's fine to fill in "magic" via one union member, and read it back via another. Pre-patch, the guarantee doesn't apply, because "sbuf.simple_reply" and "sbuf.sr.structured_reply" are not members of the same union. However, even with this patch applied (i.e., with "sbuf.reply.hdr" existing), the code performing the reading continues making me uncomfortable. We effectively have this sequence: - take address of "h->sbuf.reply.hdr" - read "sizeof h->sbuf.reply.hdr.simple" bytes into it - (at this point we can safely access "h->sbuf.reply.hdr.simple", *and* "h->sbuf.reply.hdr.structured.magic" as well -- but not the rest of "h->sbuf.reply.hdr.structured"!) - take the address of "h->sbuf.reply.hdr.structured.length" -- but not by name; instead, only by virtue of the previous read having filled in "magic", "flags", "type", and "handle", as a *happenstance*, via populating "h->sbuf.reply.hdr.simple" - read as many bytes as necessary in order to complete "h->sbuf.reply.hdr.structured", from that point onward. This smells to me. Optimally, the simple reply and the structured reply should look like this: struct nbd_reply_header { uint32_t magic; union { struct { uint32_t error; uint64_t handle; } simple; struct { uint16_t flags; uint16_t type; uint64_t handle; uint32_t length; } structured; } magic_specific; }; and we should have separate automaton states for reading "magic_specific.simple" and "magic_specific.structured". In REPLY.START, we should only read "magic". We should have a sepate state called REPLY.SIMPLE_REPLY.START, for reading "magic_specific.simple". In REPLY.STRUCTURED_REPLY.START, we should point h->rbuf at "magic_specific.structured", and read "sizeof magic_specific.structured" bytes. I'll stop here because there's already much to discuss. Thanks, Laszlo

On Fri, May 26, 2023 at 05:53:38PM +0200, Laszlo Ersek wrote: Indeed. 'union sbuf' is mostly used as a dumping grounds for "give me enough scratch storage that I can have a place to read in all places of interest across multiple recv() calls; I don't need the resulting storage to match wire format, but any time multiple pieces of information are needed at once, they must reside in non-overlapping portions of sbuf, via a sub-struct. But where we know we will not be accessing disjoint regions of sbuf, we can use further nested unions to compress the overall memory taken by sbuf". While we will never have a simple header used with a reply payload (because the only reply with payload for a simple header is NBD_CMD_READ, and we read that directly into the user's buffer instead of into sbuf), it doesn't hurt that union sbuf has reserved that much space (because we do need that much space for handilng a structured reply, and soon to handle an extended reply). Can do. Looks like I'll be spinning a v4, but I'll let you get further along in the review process to minimize list churn before I start posting updates. Correct, and since option replies are distinct from server replies, I agree with your argument of splitting removal of the attribute((packed)) into a separate patch. Also correct. While it happens to more directly touch the rest of the layout update in this patch, it is still a better fit logically to remove the attribute to both sub-structs at the same time, independently from reflowing the layout of sbuf.simple_reply and sbuf.sr. 'union sbuf' is already a union. If we didn't need or.option_reply and or.payload at the same time, then it would need make sense to just have: union sbuf { ... struct nbd_fixed_new_option_reply option_reply; struct nbd_fixed_ne struct nbd_fixed_new_option_reply_server server; char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */ } __attribute__ ((packed)) or_payload_server; struct nbd_fixed_new_option_reply_info_export or_payload_export; ... }; But because we DO need access to both or.option_reply and or.payload at the same time (for example, see generator/states-newstyle.c:handle_reply_error() called after both pieces of sbuf.or have been populated), we instead packaged the entire option reply into a nested struct, while still recognizing that the distinct payload types can overlap via a nested union: union sbuf { ... struct { struct nbd_fixed_new_option_reply option_reply; union { struct nbd_fixed_ne struct nbd_fixed_new_option_reply_server server; char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */ } __attribute__ ((packed)) server; struct nbd_fixed_new_option_reply_info_export export; ... } payload; } or; ... }; And referring back to your earlier insight, or.payload.server is a nested struct that DOES need __attribute__((packed)), because that is a case where we DO use a single recv() to populate two distinct C types into guaranteed-adjacent memory, and don't want the compiler throwing in any padding that might mess with us. Bingo. On the wire, the two portions of the reply message happen to be adjacent, and we need both portions of the reply in memory to decipher the reply. But there is no reason why our in-memory representation has to be packed to match the wire layout. And allowing the compiler to insert padding can sometimes improve performance. And convinced me of it likewise. A half hour well spent. This line is wrong. Pre-patch, it is 'struct sr', not 'union sr'. I should indeed try harder to follow your useful example of generating specific patches with more than the default 3 lines of context, when it would make review easier. Alas, 'git format-patch' doesn't seem to have an easy way to pick a different context size on a per-patch basis, so this basically implies writing (or finding and reusing existing) wrapper tooling to automate that. It happens to work, but I agree with you that we are probably stretching aliasing rules about memory effective types that it is shaky ground to begin with. Even adding a STATIC_ASSERT that offsetof(struct simple_reply, handle) == offsetof(struct structured_reply, handle) would be helpful to show that we depend on that. While both structs share only a common prefix of uint32_t magic, they intentionally DO share uint64_t handle at the SAME offset of 8 bytes in. The commit message tries to show that visually both pre- and post-patch. (Oh, that reminds me; I need to do a patch to s/handle/cookie/ to match my recent patch to the NBD spec.) Hmm. As long as this resides only in sbuf, it makes sense for libnbd. But if we also touch the structs in lib/nbd-protocol.h, then it is code that is shared with nbdkit and has an even wider ripple effect. My initial problem with THIS proposal is that it REQUIRES the use of multiple __attribute__((packed)) if we intend to read in multiple fields in one recv: for example the struct for nbd_reply_header.simple starts with uint32_t followed by uint64_t, so many ABIs want to align it as: offsets 0-3 error offsets 4-7 padding offsets 8-15 handle and when thrown in the larger nbd_reply_header, you now have a natural alignment that prefers: offsets 0-3 magic offsets 4-7 padding offsets 8-11 error offsets 12-15 padding offsets 16-23 handle Note that in lib/nbd-protocol.h, 'struct nbd_simple_reply' is already marked packed; but even if it weren't, the sizings of the member fields HAPPEN to be naturally aligned, for both simple and structured replies (I can't speak to the design of the simple reply since that pre-dates my involvement in Linux, but as I was the one that added structured replies to the NBD spec, it was intentional on my part to pick sizes that filled in all space assuming alignment matches sizing, and where magic and handle both overlap). But a STATIC_ASSERT wouldn't hurt, showing that we want: offsets 0-3 magic offsets 4-7 error OR offsets 4-5 flags, offsets 6-7 type offsets 8-15 handle (Hmm - another thing I should consider: why do we use NBD_ATTRIBUTE_PACKED in nbd-protocol.h but open-code __attribute__((packed)) in internal.h? Consistent spelling would be nice.) A secondary problem with this proposal is what happens later in the series when adding extended headers. When we added structured replies, we were forced to do (at least) two partial reads (which type of reply packet is on the wire, followed by reading the rest of the packet). But with extended headers, we know a priori that all expected responses will have a 32-byte header, and can do a single recv() for all 32 bytes. And for a server that does not support structured replies, always reading 16 bytes happens to work for simple replies (it is only when we have structured replies that the difference of 16 vs. 20 bytes of recv() is dependent on magic). Thus, having the state machine try to favor as few recv() as possible is still a worthwhile goal. But having said all that, maybe I could live with: struct nbd_reply_header { uint32_t magic; union { uint32_t simple_error; struct { uint16_t flags; uint16_t type; } structured; } middle; uint64_t handle; uint32_t length; /* only used for structured */ }; although once extended headers are in play, it would end up looking more like struct nbd_reply_header { uint32_t magic; union { uint32_t simple_error; struct { uint16_t flags; uint16_t type; } structured; /* including extended */ } middle; uint64_t handle; union { /* unused for simple */ uint32_t structured_length; struct { uint64_t offset; uint64_t length; } extended; } tail; }; At any rate, as written there, natural alignment still works for the struct prefix (no 64-bit entities in struct middle.structured, so the overall union middle has only 4-byte alignment and 4-byte size). Okay, natural alignment says the overall struct nbd_reply_header would be 24 bytes, even though structured replies only take 20 bytes (because of the 8-byte handle, the compiler inserts four bytes of padding after length), so we may still want __attribute__((packed)) or at a bare minimum STATIC_ASSERT to show that we insist on handle being at offset 8. If we weren't guaranteed that 'handle' overlaps between the two types, I might agree. But my argument is that the wire protocol DOES intend for handle to be at the same offset, regardless of the interpretation of the middle bytes, and forcing multiple recv() is just more syscalls for something that we know we'll eventually need to interpret according to magic. Even if we just read into a char[] and then reinterpret into simple or structured, rather than reading a prefix into simple and then reading more bytes into a tail of structured, is at least fewer syscalls, and also less likely to run foul of C's effective type rules (I've recently had to refamiliarize myself with C17 section 6.5 paragraph 7 as part of working on how POSIX should specify intentional aliasing occuring in struct sockaddr and friends, https://austingroupbugs.net/view.php?id=1641). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

Add the magic numbers and new structs necessary to implement the NBD protocol extension of extended headers providing 64-bit lengths. This corresponds to upstream nbd commits 36abf47d and a9384e2f on the extension-ext-header branch[1] (commit e6f3b94a for NBD_FLAG_BLOCK_STATUS_PAYLOAD is saved for a later patch). [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/p... Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- lib/nbd-protocol.h | 66 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h index f4640a98..b6fa9b8a 100644 --- a/lib/nbd-protocol.h +++ b/lib/nbd-protocol.h @@ -124,6 +124,7 @@ struct nbd_fixed_new_option_reply { #define NBD_OPT_STRUCTURED_REPLY 8 #define NBD_OPT_LIST_META_CONTEXT 9 #define NBD_OPT_SET_META_CONTEXT 10 +#define NBD_OPT_EXTENDED_HEADERS 11 #define NBD_REP_ERR(val) (0x80000000 | (val)) #define NBD_REP_IS_ERR(val) (!!((val) & 0x80000000)) @@ -141,6 +142,7 @@ struct nbd_fixed_new_option_reply { #define NBD_REP_ERR_SHUTDOWN NBD_REP_ERR (7) #define NBD_REP_ERR_BLOCK_SIZE_REQD NBD_REP_ERR (8) #define NBD_REP_ERR_TOO_BIG NBD_REP_ERR (9) +#define NBD_REP_ERR_EXT_HEADER_REQD NBD_REP_ERR (10) #define NBD_INFO_EXPORT 0 #define NBD_INFO_NAME 1 @@ -182,16 +184,26 @@ struct nbd_fixed_new_option_reply_meta_context { /* followed by a string */ } NBD_ATTRIBUTE_PACKED; -/* Request (client -> server). */ +/* Compact request (client -> server). */ struct nbd_request { uint32_t magic; /* NBD_REQUEST_MAGIC. */ - uint16_t flags; /* Request flags. */ - uint16_t type; /* Request type. */ + uint16_t flags; /* Request flags: NBD_CMD_FLAG_*. */ + uint16_t type; /* Request type: NBD_CMD_*. */ uint64_t handle; /* Opaque handle. */ uint64_t offset; /* Request offset. */ uint32_t count; /* Request length. */ } NBD_ATTRIBUTE_PACKED; +/* Extended request (client -> server). */ +struct nbd_request_ext { + uint32_t magic; /* NBD_EXTENDED_REQUEST_MAGIC. */ + uint16_t flags; /* Request flags: NBD_CMD_FLAG_*. */ + uint16_t type; /* Request type: NBD_CMD_*. */ + uint64_t handle; /* Opaque handle. */ + uint64_t offset; /* Request offset. */ + uint64_t count; /* Request effect or payload length. */ +} NBD_ATTRIBUTE_PACKED; + /* Simple reply (server -> client). */ struct nbd_simple_reply { uint32_t magic; /* NBD_SIMPLE_REPLY_MAGIC. */ @@ -208,6 +220,16 @@ struct nbd_structured_reply { uint32_t length; /* Length of payload which follows. */ } NBD_ATTRIBUTE_PACKED; +/* Extended reply (server -> client). */ +struct nbd_extended_reply { + uint32_t magic; /* NBD_EXTENDED_REPLY_MAGIC. */ + uint16_t flags; /* NBD_REPLY_FLAG_* */ + uint16_t type; /* NBD_REPLY_TYPE_* */ + uint64_t handle; /* Opaque handle. */ + uint64_t offset; /* Client's offset. */ + uint64_t length; /* Length of payload which follows. */ +} NBD_ATTRIBUTE_PACKED; + struct nbd_structured_reply_offset_data { uint64_t offset; /* offset */ /* Followed by data. */ @@ -224,11 +246,23 @@ struct nbd_block_descriptor { uint32_t status_flags; /* block type (hole etc) */ } NBD_ATTRIBUTE_PACKED; +/* NBD_REPLY_TYPE_BLOCK_STATUS_EXT block descriptor. */ +struct nbd_block_descriptor_ext { + uint64_t length; /* length of block */ + uint64_t status_flags; /* block type (hole etc) */ +} NBD_ATTRIBUTE_PACKED; + struct nbd_structured_reply_block_status_hdr { uint32_t context_id; /* metadata context ID */ /* followed by array of nbd_block_descriptor extents */ } NBD_ATTRIBUTE_PACKED; +struct nbd_structured_reply_block_status_ext_hdr { + uint32_t context_id; /* metadata context ID */ + uint32_t count; /* length of following array */ + /* followed by array of nbd_block_descriptor_ext extents */ +} NBD_ATTRIBUTE_PACKED; + struct nbd_structured_reply_error { uint32_t error; /* NBD_E* error number */ uint16_t len; /* Length of human readable error. */ @@ -236,8 +270,10 @@ struct nbd_structured_reply_error { } NBD_ATTRIBUTE_PACKED; #define NBD_REQUEST_MAGIC 0x25609513 +#define NBD_EXTENDED_REQUEST_MAGIC 0x21e41c71 #define NBD_SIMPLE_REPLY_MAGIC 0x67446698 #define NBD_STRUCTURED_REPLY_MAGIC 0x668e33ef +#define NBD_EXTENDED_REPLY_MAGIC 0x6e8a278c /* Structured reply flags. */ #define NBD_REPLY_FLAG_DONE (1<<0) @@ -246,12 +282,13 @@ struct nbd_structured_reply_error { #define NBD_REPLY_TYPE_IS_ERR(val) (!!((val) & (1<<15))) /* Structured reply types. */ -#define NBD_REPLY_TYPE_NONE 0 -#define NBD_REPLY_TYPE_OFFSET_DATA 1 -#define NBD_REPLY_TYPE_OFFSET_HOLE 2 -#define NBD_REPLY_TYPE_BLOCK_STATUS 5 -#define NBD_REPLY_TYPE_ERROR NBD_REPLY_TYPE_ERR (1) -#define NBD_REPLY_TYPE_ERROR_OFFSET NBD_REPLY_TYPE_ERR (2) +#define NBD_REPLY_TYPE_NONE 0 +#define NBD_REPLY_TYPE_OFFSET_DATA 1 +#define NBD_REPLY_TYPE_OFFSET_HOLE 2 +#define NBD_REPLY_TYPE_BLOCK_STATUS 5 +#define NBD_REPLY_TYPE_BLOCK_STATUS_EXT 6 +#define NBD_REPLY_TYPE_ERROR NBD_REPLY_TYPE_ERR (1) +#define NBD_REPLY_TYPE_ERROR_OFFSET NBD_REPLY_TYPE_ERR (2) /* NBD commands. */ #define NBD_CMD_READ 0 @@ -263,11 +300,12 @@ struct nbd_structured_reply_error { #define NBD_CMD_WRITE_ZEROES 6 #define NBD_CMD_BLOCK_STATUS 7 -#define NBD_CMD_FLAG_FUA (1<<0) -#define NBD_CMD_FLAG_NO_HOLE (1<<1) -#define NBD_CMD_FLAG_DF (1<<2) -#define NBD_CMD_FLAG_REQ_ONE (1<<3) -#define NBD_CMD_FLAG_FAST_ZERO (1<<4) +#define NBD_CMD_FLAG_FUA (1<<0) +#define NBD_CMD_FLAG_NO_HOLE (1<<1) +#define NBD_CMD_FLAG_DF (1<<2) +#define NBD_CMD_FLAG_REQ_ONE (1<<3) +#define NBD_CMD_FLAG_FAST_ZERO (1<<4) +#define NBD_CMD_FLAG_PAYLOAD_LEN (1<<5) /* NBD error codes. */ #define NBD_SUCCESS 0 -- 2.40.1

On Tue, May 30, 2023 at 01:50:59PM +0200, Laszlo Ersek wrote: The actual interpretation of the status flag is up to the spec of the metadata namespace that is being used for this particular request. Some metadata namespaces treat the "flags" field as an unsigned integer, to specify offsets or counters. I that context, the flags field should indeed be extended to 64 bits. This is, as you say, the number of block descriptor messages we are going to send to the client. Each such message is at 8 bytes long. That would mean that by the time you overflow a uint32_t with the number of extents that are to be sent, you'll be promising to send 2^32 * 8 (i.e., 2^36) bytes of data, or 32 GiB. That would be a ridiculously amount of data, and no user is going to wait for a client to finish parsing that amount of data without a hard reboot of their client. The only change that would be reasonable here is to reduce the size of this field 16 to bits, rather than increasing it (but then we lose alignment, so that's also not a good idea) -- w(a)uter.{be,co.za} wouter(a){grep.be,fosdem.org,debian.org} I will have a Tin-Actinium-Potassium mixture, thanks.

On 5/25/23 15:00, Eric Blake wrote: (1) Are there places in the code where we currently assign this "count" field back to a uint32_t object, and assume truncation impossible? What's more, this is a "by the book" common initial sequence! :) Nice -- I wanted to ask for: flags &= ~(uint32_t)LIBNBD_CMD_FLAG_PAYLOAD_LEN; due to LIBNBD_CMD_FLAG_PAYLOAD_LEN having type "int". However: in patch#3, what has type "int" is: +#define NBD_CMD_FLAG_PAYLOAD_LEN (1<<5) and here we have LIBNBD_CMD_FLAG_PAYLOAD_LEN instead -- and the latter has type unsigned int already, from your recent commit 69eecae2c03a ("api: Generate flag values as unsigned", 2022-11-11). And I think we're fine assuming that uint32_t is unsigned int. (2) Incorrect indentation: two spaces rather than one tab. (3) Minor inconsistency with check(): we're not printing "progname" here. (4) Another kind of inconsistency: we could use "progname" here, in place of argv[0]. (This applies to all other fprintf()s below.) Not sure if I understand this check. Per <https://libguestfs.org/nbd_set_strict_mode.3.html>;, I take it that LIBNBD_STRICT_FLAGS should be "on" by default. Are you enforcing that? And if so: is it your intent that, *even with* LIBNBD_STRICT_FLAGS, an invalid PAYLOAD_LEN is not rejected (as seen by the libnbd client app), but fixed up silently? Ah, got it now. We do want APIs other than pwrite to fail. You could contract these into: if (nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION, LIBNBD_CMD_FLAG_PAYLOAD_LEN) == -1 || nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION, 0) == -1) { fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); exit (EXIT_FAILURE); } In general, I think it's good practice to reach nbd_close() whenever nbd_create() succeeds (that is, on error paths as well, after nbd_create() succeeds). For example, if we connected to the server with systemd socket activation in this test, and (say) one of the pwrites failed, then we'd leak the unix domain socket in the filesystem (from the bind() in "generator/states-connect-socket-activation.c"). "sact_sockpath" is unlinked in nbd_close(). (As written, this test should not be affected, because, according to unix(7), unix domain sockets created with socketpair(2) are unnamed.) Thanks Laszlo

On 5/30/23 20:18, Eric Blake wrote: OK. I don't understand. What you describe here (= us fixing up the flag for the caller) applies to *pwrite*, not *pread*. Furthermore, the above check tests pread's behavior, and it expects pread to *fail*. In effect, my understanding of the test code is this: - assume extended headers have not been negotiated - require that the NBD connection be created such that it enforces flag validity on the client side (i.e., "strict mode" including "strict flags") - test that pread fails with PAYLOAD_LEN -- pread should fail *regardless* of extended headers having been negotiated, because (a) if extended headers are not in use, then the flag is altogether invalid, (b) even with extended headers, a read request does not accept the flag. Because we don't add "PAYLOAD_LEN" as a valid flag to pread in the generator code, the check for (b) is always active. - test that pwrite succeeds with PAYLOAD_LEN -- pwrite should succeed *regardless* of extended headers having been negotiated, because we set PAYLOAD_LEN internally, dependent on the extended headers; i.e., ignoring the user's argument. That is, I think I did manage to explain the test to myself, but your most recent answer confuses me again! :) Right, I assume FIXME in the test code might be addressed together with the TODO in nbd_unlocked_aio_pwrite(). Once we know whether the server negotiated extended headers, *and* if the user asks for strictness regarding the PAYLOAD_LEN flag, we can enforce PAYLOAD_LEN's equivalence with extended headers in pwrite calls. Laszlo

Support receiving headers for 64-bit replies if extended headers were negotiated. We already insist that the server not send us too much payload in one reply, so we can exploit that and merge the 64-bit length back into a normalized 32-bit field for the rest of the payload length calculations. The NBD protocol specifically documents that extended mode takes precedence over structured replies, and that there are no simple replies in extended mode. We can also take advantage that the handle field is in the same offset in all the various reply types. Note that if we negotiate extended headers, but a non-compliant server replies with a non-extended header, this patch will stall waiting for the server to send more bytes rather than noticing that the magic number is wrong (for aio operations, you'll get a magic number mismatch once you send a second command that elicits a reply; but for blocking operations, we basically deadlock). The easy alternative would be to read just the first 4 bytes of magic, then determine how many more bytes to expect; but that would require more states and syscalls, and not worth it since the typical server will be compliant. The other alternative is what the next patch implements: teaching REPLY.RECV_REPLY to handle short reads that were at least long enough to transmit magic to specifically look for magic number mismatch. At this point, h->extended_headers is permanently false (we can't enable it until all other aspects of the protocol have likewise been converted). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- lib/internal.h | 1 + generator/states-reply-structured.c | 52 +++++++++++++++++++---------- generator/states-reply.c | 34 ++++++++++++------- 3 files changed, 58 insertions(+), 29 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 8a5f93d4..e4e21a4d 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -243,6 +243,7 @@ struct nbd_handle { union { struct nbd_simple_reply simple; struct nbd_structured_reply structured; + struct nbd_extended_reply extended; } hdr; union { struct nbd_structured_reply_offset_data offset_data; diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index df509216..c53aed7b 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -45,14 +45,20 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length, STATE_MACHINE { REPLY.STRUCTURED_REPLY.START: - /* We've only read the simple_reply. The structured_reply is longer, - * so read the remaining part. + /* If we have extended headers, we've already read the entire header. + * Otherwise, we've only read enough for a simple_reply; since structured + * replies are longer, read the remaining part. */ - h->rbuf = &h->sbuf; - h->rbuf = (char *)h->rbuf + sizeof h->sbuf.reply.hdr.simple; - h->rlen = sizeof h->sbuf.reply.hdr.structured; - h->rlen -= sizeof h->sbuf.reply.hdr.simple; - SET_NEXT_STATE (%RECV_REMAINING); + if (h->extended_headers) { + assert (h->rbuf == sizeof h->sbuf.reply.hdr.extended + (char*)&h->sbuf); + SET_NEXT_STATE (%CHECK); + } + else { + assert (h->rbuf == sizeof h->sbuf.reply.hdr.simple + (char*)&h->sbuf); + h->rlen = sizeof h->sbuf.reply.hdr.structured - + sizeof h->sbuf.reply.hdr.simple; + SET_NEXT_STATE (%RECV_REMAINING); + } return 0; REPLY.STRUCTURED_REPLY.RECV_REMAINING: @@ -69,11 +75,18 @@ REPLY.STRUCTURED_REPLY.RECV_REMAINING: REPLY.STRUCTURED_REPLY.CHECK: struct command *cmd = h->reply_cmd; uint16_t flags, type; - uint32_t length; + uint64_t length; + uint64_t offset = -1; + assert (cmd); flags = be16toh (h->sbuf.reply.hdr.structured.flags); type = be16toh (h->sbuf.reply.hdr.structured.type); - length = be32toh (h->sbuf.reply.hdr.structured.length); + if (h->extended_headers) { + length = be64toh (h->sbuf.reply.hdr.extended.length); + offset = be64toh (h->sbuf.reply.hdr.extended.offset); + } + else + length = be32toh (h->sbuf.reply.hdr.structured.length); /* Reject a server that replies with too much information, but don't * reject a single structured reply to NBD_CMD_READ on the largest @@ -83,13 +96,18 @@ REPLY.STRUCTURED_REPLY.CHECK: * not worth keeping the connection alive. */ if (length > MAX_REQUEST_SIZE + sizeof h->sbuf.reply.payload.offset_data) { - set_error (0, "invalid server reply length %" PRIu32, length); + set_error (0, "invalid server reply length %" PRIu64, length); SET_NEXT_STATE (%.DEAD); return 0; } + /* For convenience, we now normalize extended replies into compact, + * doable since we validated that payload length fits in 32 bits. + */ + h->sbuf.reply.hdr.structured.length = length; /* Skip an unexpected structured reply, including to an unknown cookie. */ - if (cmd == NULL || !h->structured_replies) + if (cmd == NULL || !h->structured_replies || + (h->extended_headers && offset != cmd->offset)) goto resync; switch (type) { @@ -168,7 +186,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); + length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */ assert (length >= sizeof h->sbuf.reply.payload.error.error.error); assert (cmd); @@ -207,7 +225,7 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR_MESSAGE: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); + length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */ msglen = be16toh (h->sbuf.reply.payload.error.error.len); type = be16toh (h->sbuf.reply.hdr.structured.type); @@ -307,7 +325,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); + length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */ offset = be64toh (h->sbuf.reply.payload.offset_data.offset); assert (cmd); /* guaranteed by CHECK */ @@ -346,7 +364,7 @@ REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); + length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */ offset = be64toh (h->sbuf.reply.payload.offset_data.offset); assert (cmd); /* guaranteed by CHECK */ @@ -426,7 +444,7 @@ REPLY.STRUCTURED_REPLY.RECV_BS_HEADER: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); + length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */ assert (cmd); /* guaranteed by CHECK */ assert (cmd->type == NBD_CMD_BLOCK_STATUS); @@ -460,7 +478,7 @@ REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES: SET_NEXT_STATE (%.READY); return 0; case 0: - length = be32toh (h->sbuf.reply.hdr.structured.length); + length = h->sbuf.reply.hdr.structured.length; /* normalized in CHECK */ assert (cmd); /* guaranteed by CHECK */ assert (cmd->type == NBD_CMD_BLOCK_STATUS); diff --git a/generator/states-reply.c b/generator/states-reply.c index 31e4bd2f..4e9f2dde 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -62,15 +62,19 @@ REPLY.START: */ ssize_t r; - /* We read all replies initially as if they are simple replies, but - * check the magic in CHECK_SIMPLE_OR_STRUCTURED_REPLY below. - * This works because the structured_reply header is larger. + /* With extended headers, there is only one size to read, so we can do it + * all in one syscall. But for older structured replies, we don't know if + * we have a simple or structured reply until we read the magic number, + * requiring a two-part read with CHECK_SIMPLE_OR_STRUCTURED_REPLY below. */ assert (h->reply_cmd == NULL); assert (h->rlen == 0); h->rbuf = &h->sbuf.reply.hdr; - h->rlen = sizeof h->sbuf.reply.hdr.simple; + if (h->extended_headers) + h->rlen = sizeof h->sbuf.reply.hdr.extended; + else + h->rlen = sizeof h->sbuf.reply.hdr.simple; r = h->sock->ops->recv (h, h->sock, h->rbuf, h->rlen); if (r == -1) { @@ -116,16 +120,22 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY: uint64_t cookie; magic = be32toh (h->sbuf.reply.hdr.simple.magic); - if (magic == NBD_SIMPLE_REPLY_MAGIC) { + switch (magic) { + case NBD_SIMPLE_REPLY_MAGIC: + if (h->extended_headers) + goto invalid; SET_NEXT_STATE (%SIMPLE_REPLY.START); - } - else if (magic == NBD_STRUCTURED_REPLY_MAGIC) { + break; + case NBD_STRUCTURED_REPLY_MAGIC: + case NBD_EXTENDED_REPLY_MAGIC: + if ((magic == NBD_STRUCTURED_REPLY_MAGIC) == h->extended_headers) + goto invalid; SET_NEXT_STATE (%STRUCTURED_REPLY.START); - } - else { - /* We've probably lost synchronization. */ - SET_NEXT_STATE (%.DEAD); - set_error (0, "invalid reply magic 0x%" PRIx32, magic); + break; + default: + invalid: + SET_NEXT_STATE (%.DEAD); /* We've probably lost synchronization. */ + set_error (0, "invalid or unexpected reply magic 0x%" PRIx32, magic); #if 0 /* uncomment to see desynchronized data */ nbd_internal_hexdump (&h->sbuf.reply.hdr.simple, sizeof (h->sbuf.reply.hdr.simple), -- 2.40.1

On Thu, Jun 01, 2023 at 11:04:05AM +0200, Laszlo Ersek wrote: Yep, I'm already rebasing the series to do that, now that c1df4df9 landed. Yes. If you haven't guessed already, I actually did hit this bug (my initial qemu patches sent the wrong reply type, and it took me several minutes to figure out why libnbd was hung), so I was also pleasantly surprised at how easy the next patch turned out to be, after first trying (and failing) to go down the rabbit hole mentioned here of adding yet more states. But, as you mentioned earlier in the series, it MAY be worth rearranging states anyways, to specifically route extended header parsing differently from structured header parsing. So I'm still playing with that, and seeing what may happen to this patch as fallout. I see what you mean: because of the state hierarchy, it is probably worth tweaking the git orderfile to favor files nearer the top of the state tree first, rather than strict alphabetical ordering of *.c. I may just push a patch for that without needing review, as it doesn't affect library semantics at all. ... Yep, I'm already rebasing onto some additional asserts along those lines, based on your reviews earlier in the series. I still do a double-take every time I see 'a < b < c' in languages (like python) that support it as shorthand for 'a < b && b < c'. Even weirder is when you see 'a < b > c'. But yes, C's non-transitive == can be a surprise for the uninitiated. That part of the patch pre-dates other reviews I've seen from you on the same topic (this patch series has been percolating on my local builds for a long time), so I'm not surprised by your request, and I'm happy to squash in your improvement. I may have copied from other similar code in the generator/states-*.c files, if so, I'll probably do a separate cleanup patch first. Indeed, when rebasing, I need to remember to grep (to cover comments) rather than merely relying on the compiler (which only covers code) for renames. Will fix in the appropriate place(s). Oh cool. I keep forgetting that you can declare a type name usable at the top level even while declaring a nested struct. Yeah, doing that probably has some nice line length improvements. Not necessarily - once I add in more STATIC_ASSERTs, being able to refer to individual nested types without having to go all the way through struct nbd_handle or union sbuf will be shorter. That argument still remains - it's easier to see that we expect a particular offset from sbuf when the use of sbuf in the offset calculation is not hidden several lines away in the intermediate initialization. I'll think about it for this line. I'll have to double-check; I thought that offset was read even for structured replies (where there really isn't an offset read from the wire), but my rebasing efforts may have changed that. Hmmm. Reading ahead, I see what you wrote in (9). This may have been something I copied from other states, but I'll have to double check whether it still makes sense. At one point, I considered normalizing in the opposite direction (take structured replies and widen them into the extended header form); it required touching more lines of code, so I didn't pursue it at the time. But I can see how compressing things down (intentionally throwing away information we will no longer reference) can be confusing without good comments, so at a minimum, I will be adding comments, if not outright going with the widening rather than narrowing approach. Off-hand, I think the assert is correct and I do have a dead condition here; but I'll have to convince myself of that. Since the condition is pre-existing, it may be worth a separate patch adding just the assertion and removing the 'cmd == NULL' check here, if it is correct. I think this all stems from me trying to be as generous as possible with bad servers: if we get a wrong reply type per the protocol but it is a reply type we know how to handle, AND we haven't consumed too many bytes for the size of that reply, then we can tolerate the server's bug. If it is the wrong type, and we already read too many bytes for what the right type would be, we can't (easily) undo the fact that we read too much. If we did not negotiate extended headers, we start by reading the length of a simple header; if the magic says it was instead an extended header, we can tolerate the server's mistake by reading the rest of that packet. But I definitely see your point about consolidating the decisions to be local to one state, rather than split across two files, and aiming for consistent handling. I'm liking your idea earlier in the series about reworking the state engine to FULLY parse the header (whether simple, structured, or extended) in states-reply.c, and only then move to states-reply-structured.c if the header was structured or extended, rather than splitting the header parse across two files. The evolution of that field: in my first draft, I wanted power-of-2 sizing to the reply header (both request and reply having a header of exactly 32 bytes); this left dead space in the reply packet, which I originally tried to mandate to be zero-filled. But when looking at the types again, I noticed that if the 'offset' field occupies the same relative place in the request and reply struct, a server can rewrite the reply in the same memory as it had saved for the request (I don't know that I actually implemented it that way in qemu as server, but it is possible for some other server). There's also NBD_REPLY_TYPE_OFFSET_DATA and NBD_REPLY_TYPE_OFFSET_HOLE (used in response to NBD_CMD_READ), which each return an offset field in order to handle the case where a single client read request was split across multiple server replies. When we first specified structured replies, the spec was ambiguous: if I issue NBD_CMD_READ(length=1k, offset=0) but the server replies with the pair OFFSET_DATA(length=512, offset=0) and OFFSET_HOLE(length=512, offset=512), it's obvious how to reconstruct the original buffer (read into the first 512 bytes, memset the second 512 bytes). But if I issue NBD_CMD_READ(length=1k, offset=512), should the server reply OFFSET_DATA(length=512, offset=512) and OFFSET_HOLE(length=512, offset=1k) (that is, the reply offsets are absolute to the overall image), or with OFFSET_DATA(length=512, offset=0) and OFFSET_HOLE(length=512, offset=512) (that is, the reply offsets are relative to the start of the 1k buffer of the read request)? The initial implementations of qemu and nbdkit disagreed, and the spec ended up settling in favor of the former (see nbd.git commit 587ba722). As a result, when receiving an absolute offset in a data reply, the client has to place the data into buffer + (reply_offset - request_offset) (that is, convert the server's absolute offset back into a buffer-relative offset), which requires knowing what the reqeust offset was; having the server return this offset in extended replies may make it easier to implement a client that doesn't have to store its request offset separately. Since libnbd also handles non-extended headers (and has to hang on to the request offset for those), you are right that we end up merely validating that the server's reply offset was expected, and then throwing it away as it didn't add anything further than a validation that we haven't lost sync with the server. The benefit is that all later code only has to deal with nbd_structured_reply (rather than adding yet more 'if (h->extended_reply)' in all subsequent states). Conversely, if I had widened instead of narrowed, then all later code would only need to doea with nbd_extended_reply. You may be on to something. Leaving the wire reply ALWAYS in network endian order, and adding a few bytes to nbd_reply to stash a normalized host-endian decoded value, would certainly be less confusing than having to figure out "which parts of my wire reply are still network-endian vs. natively converted". It may make the overall structure a few bytes larger, but that's probably in the noise (since our nbd_handle already accomodatese maximum-length NBD strings of 4k, adding up to another 32 bytes for fully-normalized host-endian form isn't bad). I'll keep that in mind for v4 (this is not the only place that needs normalization; handling 32- vs. 64-bit block status replies also needs normalization; the tradeoff between minimal storage by normalizing in place vs. maintainability by normalizing into a copy has interesting implications). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Thu, Jun 01, 2023 at 08:00:55AM -0500, Eric Blake wrote: In fact, doing it in place is bad. Later code in this function can trigger a transition to state RESYNC, which assumes sbuf.reply.hdr.structured.length still holds the wire value, not the normalized value. I'm fixing it up by creating h->payload_left, initialized in START to the endian-corrected wire value, and then decremented as various states peel off parts of the payload, so that FINISH can then assert all payload has been accounted for. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On 6/7/23 16:55, Richard W.M. Jones wrote: Good point; that reminds me we used to encounter that issue specifically in IA32 edk2 builds, but only when using old gcc (RHEL-7 era, gcc-4.8.5). Of course it might still happen today. If that's the case, can we comment the code that we initialize "offset" for shutting up the compiler? Thanks! Laszlo

On 5/25/23 15:00, Eric Blake wrote: (Totally tangential comment: recvmsg() could scatter the incoming traffic into non-contiguous fields of a non-packed structure. But I don't know if TLS has anything similar. The current "linear receive" approach is probably the least demanding of the underlying socket abstractions. At the same time it requires us to use packed structs, and/or multiple syscalls.) Haha, so this is where it's coming from! :) I can totally relate. (1) s/simply/simple/ (2) At this point my English parser gets thrown off: resync here: (4) Slight inconsistency between commit message and code comment: the former mentions "20 bytes", but the latter doesn't mention "structured reply". (.) I wonder if (address-of-magic + size-of magic) might be more readable / greppable. Just in case. Feel free to ignore. (5) This could be factored out to a helper function, to share the "invalid:" label logic with the previous patch. (6) Commencing a dump from "flags" onward looks questionable. At this point, the "flags" field need not to be complete, or even started -- all we know is that the "magic" field *before" "flags" is complete. I think we should dump "h->sbuf.reply.hdr.simple", like in patch#5. (.) Side comment (so no bullet number assigned): because we can take multiple iterations on RECV_REPLY, we may end up checking the "magic" field multiple times. Not very costly, just something to be aware of. (7) Assume that we have a short read first, but then complete REPLY.RECV_REPLY successfully, and move to REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY. Then, the following condition will have been caught (excluded) under RECV_REPLY: (extended_headers && magic != NBD_EXTENDED_REPLY_MAGIC) [1] Consequently, the same condition will never hold in REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY. Now consider REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY, from the previous patch: magic = be32toh (h->sbuf.reply.hdr.simple.magic); switch (magic) { case NBD_SIMPLE_REPLY_MAGIC: if (h->extended_headers) goto invalid; This branch becomes dead code. The condition h->extended_headers && magic == NBD_SIMPLE_REPLY_MAGIC would satisfy [1], therefore it can't happen here. Further: SET_NEXT_STATE (%SIMPLE_REPLY.START); break; case NBD_STRUCTURED_REPLY_MAGIC: case NBD_EXTENDED_REPLY_MAGIC: if ((magic == NBD_STRUCTURED_REPLY_MAGIC) == h->extended_headers) goto invalid; While this lovely condition is satisfied by "true == true", that is, by: h->extended_headers && magic == NBD_STRUCTURED_REPLY_MAGIC [2] the same condition [2] satisfies [1] too, and therefore it can never occur. The only way for satisfying this (magic == NBD_STRUCTURED_REPLY_MAGIC) == h->extended_headers condition would be the "false == false" case, that is: !h->extended_headers && magic == NBD_EXTENDED_REPLY_MAGIC [3] (note that (magic != NBD_STRUCTURED_REPLY_MAGIC) is equivalent to (magic == NBD_EXTENDED_REPLY_MAGIC) due to how the switch statement is structured,) and then we should use that expression, i.e., [3], because it is more readable than the double-equality (i.e., equivalence) form. *HOWEVER*! :) If we do *not* see some short-but-together-long-enough reads first, but see a full read at once, then our "pre-check" in RECV_REPLY does not get activated at all! And then all the conditions in CHECK_SIMPLE_OR_STRUCTURED_REPLY remain necessary. So this is my long-winded way to ask that: you please consider adding a comment here, saying that, while the short read special case *does* short-circuit a number of checks under CHECK_SIMPLE_OR_STRUCTURED_REPLY, all the checks in CHECK_SIMPLE_OR_STRUCTURED_REPLY remain required if under RECV_REPLY we get a full read immediately. Thanks! Laszlo

On 7/21/23 14:50, Eric Blake wrote: My mistake, probably due to over-editing! :) Thanks Laszlo

On 5/25/23 15:00, Eric Blake wrote: ( so the terminology is: - we have "len" uint32_t elements in the array, - one extent is one uint32_t pair, - hence we have len/2 extents in the array ) The term "extent pairs" is inconsistent with the above terminology. (1) We should say: - "len/2 extents", or - "len/2 uint64_t pairs". (2) s/or with/or go with/ ? This sounds OK; if the element type of the array is a struct, then one struct describes one extent, and then the "len" elements of the array describe "len" extents. This is a good argument, but I needed to chew on the "but" conjunction for a good while until I understood the point. The "but" seemed out of place at first; only later did I realize it was actually the core of the argument ("easier to map"). The point is that for each extent, we have two properties that are same in size but that differ in meaning (hence in type), and *therefore* a struct is better than two adjacent, raw uint64_t elements. We're going to have two uint64_t fields, but conceptually, that's one off_t field (just serialized as a uint64_t) and a genuine uint64_t field, for status. (3) Can you append a sentence, "That is, conceptually, the fields have different types, so a structure is best to express them"? Or something similar? (4) Can you say "We need to insert '__attribute__ ((unused))' a number of times, temporarily, to keep the compiler happy..."? "Unused attributes" sounds as if we needed some attributes that were not used. Confusing to me. What we need instead is to mark some variables or functions with an attribute that says those vars/funcs are unused. Could be OK or fishy; I should be able to tell later :) OK. This extends "name_of_arg". This extends "arg_attr_nonnull". This extends "print_arg_list". This extends "print_cbarg_list". Pointing a bit into the future, but fine / unavoidable. 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. This extends "print_trace_enter" (near the top). And this extends "print_format_string" within "print_trace_enter". Now here's why I've inserted all those "this extends ..." remarks: I think your assertion is valid here, *but* it is inconsistent with the extensions to: - name_of_arg - arg_attr_nonnull - print_arg_list - print_trace_enter We only deal with Extent64 via the "penetrating pattern match" CBArrayAndLen (Extent64 n, len), in "print_cbarg_list". (5) Therefore, for consistency, *all* other Extent64 pattern matches should assert that they are unreachable -- not only in "print_format_string", but in those four functions too that I list above. Clearly, I could be missing some code paths leading to those functions / patterns; I've not tested my proposition. The idea is that the narrowest necessary subset of patterns should be handled. Even if it is easy to "handle" a pattern with just (), it makes the wrong impression. This extends "print_args" within "print_trace_enter". The assertion is OK. (6) This is near the end of "print_trace_enter", and again, we should assert here that "Extent64 _" will never match. (7) I care pretty little for the Go binding, but again: the hunk above that is the first for "GoLang.ml", we extend "go_name_of_arg", and I'd argue we should insert an "assert false" there. 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. Regarding the new copy_extent_array(), my additional complaints are: - whitespace usage before opening parens is inconsistent -- there is no space after "make" and "Pointer". - 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): (This passes make and make-check, when the tree is checked out at your patch#7, and this is applied on top.) Back to your patch: On 5/25/23 15:00, Eric Blake wrote: 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): OK. OK, this is as announced in the commit message. OK, this is responsible for generating: PyObject *py_entries = NULL; (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; producing (in the preexistent "extent_wrapper" function): if (!py_entries) { PyErr_PrintEx (0); goto out; } We have two issues here: (10) We should call PyLong_FromUnsignedLongLong(), not PyLong_FromUnsignedLong(). On an ILP32 platform, the latter will not do the right thing. (11) PyLong_FromWhatever() returns a new reference, and so it can fail due to memory exhaustion (minimally, I think). That means zero, one, or two of our PyLong_FromUnsignedLongLong() calls can fail, and return NULL. We call Py_BuildValue() with an "OO" specification. The documentation says: Now, I don't think PyLong_FromUnsignedLongLong() sets an exception when it fails (and returns NULL), but that should be fine -- at least Py_BuildValue() will raise SystemError. The problem is that we have three possible ways to leak memory: - If we pass two NULLs to "OO" (after both our PyLong_FromUnsignedLongLong() calls fail), there's no leak. Py_BuildValue() fails in response too, and nothing is leaked. - If we pass exactly one NULL to "OO" (after one PyLong_FromUnsignedLongLong() succeeds, but the other fails), then Py_BuildValue() fails again (as expected), but we leak the PyLongObject that we *did* successfully create. - If we pass two non-NULLs to "OO" (after both PyLong_FromUnsignedLongLong() succeed), and Py_BuildValue() fails because *it* cannot allocate further memory, then we leak *both* PyLongObjects. - If we pass two non-NULLs to "OO" (after both PyLong_FromUnsignedLongLong() succeed), and Py_BuildValue() *succeds*, then we leak both PyLongObjects again: PyLong_FromUnsignedLongLong() returns a new reference, but when Py_BuildValue() succeeds, it adds a *new* reference (it does not only take ownership). The actual leak will happen later, when the object just constructed with Py_BuildValue() is torn down. So, we should implement this in a much more "staged" way. Shown as a diff on top of the code generated by your patch, covering points (9) through (11) (even (12) below): In this variant, if at least one PyLong_FromUnsignedLongLong() call fails, then Py_BuildValue() catches that, fails itself, and at the "out" label, we release precisely those PyLongObjects that we managed to construct (zero or one objects). If both PyLongObjects get allocated, but Py_BuildValue() fails, then at the "out" label, we release both PyLongObjects. If even the Py_BuildValue() call succeeds, then we reach PyList_SET_ITEM(). PyList_SET_ITEM() "steals" a reference (i.e., takes ownership of one reference), it does not add one. Therefore the just constructed "py_e_entries" object is now owned by the "py_entries" list -- and we want the new "py_e_entries" entry in that list too to own our PyLongObjects. Therefore, we release our temporary references to the PyLongObjects (see the xdecref's in the loop body). We also NULL out the variables, reflecting the ownership transfer. This is generally good practice, but it even matters, for example, when this is the last iteration of the loop, and the subsequent nbd_internal_py_wrap_errptr() call fails. Then we jump to the "out" label, and at that point, the xdecref's *there* must be no-ops. The PyLongObjects will be released via "py_entries". (In theory we could NULL out the variables right after the loop body, rather than at the end of the loop body, given that the top of the loop body immediately overwrites both variables -- but as written, the code is more understandable.) The xdecref's and the NULL'ing happen in reverse, relative to order of construction (i.e., the inverse order is intentional). Back to your patch: On 5/25/23 15:00, Eric Blake wrote: Right, this corresponds to "py_entries" being a list object. (12) We do need this Py_XDECREF (py_entries); we just need the Py_XDECREF (py_e_flags); Py_XDECREF (py_e_length); calls before it, too, so I think ultimately you'll want to handle this pattern separately. Yes, these are an "easy sell". (13) However I would turn these into assertions of "false", as well. They too are in "print_python_binding". OK. OK. (14) Could we use "assert false" with these too? (15) Can we add a note here that negative values are not permitted? (See my further comments below.) This is inside "print_ocaml_closure_wrapper"; OK. (16) You can contract the new pattern as follows: | CBArrayAndLen ((UInt32 n | Extent64 n), _) Seems OK superficially; matches the 'CBArrayAndLen (UInt32 n, count)' case, with only the helper function differing. (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. OK, announced in the commit message. OK, this is in "print_ocaml_binding". 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. (19) I'd suggest the following addition: *However*, considering the nbd_internal_ocaml_alloc_extent64_array() call site, in the generated extent64_wrapper_locked() function [ocaml/nbd-c.c]: 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. Again, this is a huge patch; I've now spent almost a full workday on reviewing it. I suggest splitting each language to its own patch; thus, we'd get at least 5 patches (core generator changes + 4 languages). If necessary to placate the OCaml compiler, I'd try introducing bare-bones place-holder code at first in the generator for each language, in the first ("core") patch, and then elaborate on each language in a separate patch (replacing the placeholders with actual code). Laszlo

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

On Mon, Jul 24, 2023 at 07:04:15PM -0500, Eric Blake wrote: On the other hand, that would be reverting Nir's patch 6725fa0e, which introduced that notation in the first place, but also without benchmarking. Nir, any thoughts before we touch this code? -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org

Fair warning: this patch has made me both very excited and very uncomfortable; that's the reason my style might come across as needlessly "combative". I don't mean to be combative. It's just how my thoughts have come out, and (unfortunately) I need to go pick up my son now, and don't have time for further rounds of polishing my style. I apologize in advance. Again, it's a consequence of me caring a bit too much perhaps, and getting worked up. On 5/25/23 15:00, Eric Blake wrote: Yes. Per spec, negotiating NBD_OPT_EXTENDED_HEADERS is equivalent to the server sending NBD_REPLY_TYPE_BLOCK_STATUS_EXT, as opposed to NBD_REPLY_TYPE_BLOCK_STATUS. I agree that a 64-bit-only internal representation makes sense. Once corresponding code part (quoting out of order for discussion's sake) is: and that looks good to me. I absolutely don't follow this. The spec is clear. NBD_OPT_EXTENDED_HEADERS corresponds to NBD_REPLY_TYPE_BLOCK_STATUS_EXT, and (!NBD_OPT_EXTENDED_HEADERS) corresponds to NBD_REPLY_TYPE_BLOCK_STATUS. If NBD_OPT_EXTENDED_HEADERS is negotiated (which means both client and server agree to it), then the client interface is 64-bit clean, so there's no need to truncate anything. If NBD_OPT_EXTENDED_HEADERS is *not* negotiated, then our internal 64-bit clean representation will never be filled with information that doesn't fit into 32-bits (due to the reply type being restricted to NBD_REPLY_TYPE_BLOCK_STATUS), so there is again no need to (observably) truncate anything (technically there would be a uint64_t to uint32_t conversion, but it would be value-preserving). The spec also says that, if a client wants to query a particular metadata context that is known to use 64-bit values, then the client must first negotiate NBD_OPT_EXTENDED_HEADERS. Again, I don't follow. Whenever the client issues an NBD_CMD_BLOCK_STATUS command (which can only occur after negotiation): - the client must have set at least one metacontext with NBD_OPT_SET_META_CONTEXT during negotiation, - the client may or may not have agreed upon with the server on NBD_OPT_EXTENDED_HEADERS. Therefore, when the server receives the NBD_CMD_BLOCK_STATUS command, it can immediately decide whether there is *any* configured metacontext for which *lack* of extended headers could be too restrictive. If only 32-bit metacontexts have been requested, this is not a problem. If ext-hdrs have been negotiated, also not a problem. If there is at least one 64-bit metacontext (regarding *either* status flags *or* lengths) *and* ext-hdrs have not been negotiated, the server should immediately fail (reject) the request. Now, peeking a bit forward, I can see patch#10: "api: Add [aio_]nbd_block_status_64". That means we can reject a 32-bit-only nbd_block_status() API call on the client side immediately, if the client has negotiated ext-hdrs. That is, if the client is smart enough to ask for extended headers, it can be smart enough to use the 64-bit API, and then it can accept everything the server throws at it. If the client never even asks for extended headers, then it is permitted to stick with the 32-bit-only API, but then the server must never respond with NBD_REPLY_TYPE_BLOCK_STATUS_EXT [*], so actually 64-bit values are never going to be present in our internal representation. [*] If it still does, then the server is buggy, and we should realize that as soon as we parse the generic reply magic, or the specific reply type code. I simply fail to see the *possibility* truncating the internal 64-bit representation to 32-bit such that it would make a difference for the caller. Speaking in terms of code: Side question: what guarantees that the multiplication won't overflow? If we have a guarantee, we should document it, and (preferably) assert it. Yes, and we should absolutely enforce that. Don't we break off the connection (or at least abort the request) when we get the wrong reply magic from the server, or else if we do get the right reply magic, but an NBD_REPLY_TYPE_BLOCK_STATUS_EXT reply type code inside, without having negotiated extended headers? This compatibility goo creates a big maintenance and debugging burden. Back to the commit message: I'm not worried about the speed penalty, but at this point (and looking at the rest of the patch) I'm actually changing my mind, and questioning whether we are better off with the unified, 64-bit only internal representation. It's not just about converting back and forth (and hence code complexity): we're introducing further malloc()s, and those are all new failure points. Even if we do handle memory allocation failures everywhere correctly, they still introduce pontential *late* breaking points for the client. Right, and I happen to disagree with your implementation; more on that below. Now this could be a good approach in my opinion; however, I disagree with how it's being used (the in-place conversion below, which IMO arguably runs afoul of the effective type rules in C). This division will work as expected, but it's hard to see why. After my request under patch#1, for introducing bs_reply_length_ok(), and asserting it here, it will be easier to understand. I do dislike the open-coded "sizeof (uint32_t)" though, we should say "sizeof *h->bs_entries.narrow". So I find this way too clever; worse, it almost certainly breaks the effective type rules in C. First, I would be OK with storing pointers in the bs_entries union, if our only purpose with the union were to only handle one of those pointers at any given time. However, we're type punning the pointers back and forth, and relying on their representation being identical. (Pointer to structure vs. pointer to char vs. pointer to uint32_t.) Not catastrophic, but not nice (and IMO not even necessary, see below). Second, after reading the effective type rules very carefully, the code might break them minimally (I think) when "i" equals zero, and the "length" field is assigned. In that instance, you are reading the same memory location as a uint32_t and writing it as a uint64_t, and not going through a union. I'd prefer one of the two following options: - bs_entries should indeed be the union that you recommend, but used differently. First, "storage" should be allocated, and filled. When the reading completes, a new uint32_t array should be allocated, populated via byte-swapping, "storage" should be released, and the new (now byte-order-correct) array should be recorded in "narrow". Same approach for the future "normal" population. The advantages of this approach are: (a) not reading a pointer-to-type-X value as a pointer-to-type-Y value (reinterpreting the bit pattern) via the union, as we should always know what reply type we're dealing with, (b) any allocated array would only be accessed with a singular element type at any point in time. A kind of "double buffering". - Better yet: drop the "storage" field from the union, and only have "narrow" and "normal" in it. Treat them entirely separately. For the 32-bit case, simply rename the field references (from bs_normal to bs_normal.narrow), no need to change the existent logic. For the 64-bit case, duplicate and widen the 32-bit case (the byte swapping etc), using bs_entries.normal. [empty line here due to earlier out-of-order quoting] For better readability, "data" should be "user_data" IMO, and "cb" should be "shim". I disagree with this order of operations. (Note, I'm not saying there are leaks or double free's.) Consider the nbd_unlocked_aio_block_status() call site in nbd_aio_block_status(): If that call succeeds (returns a value different from (-1)), then we expect nbd_unlocked_aio_block_status() to have taken ownership of both "extent_callback" and "completion_callback", by emptying them. That way, the FREE_CALLBACK()s at the end of the function decay to NOPs. If the call fails OTOH, we expect the call to have left "extent_callback" and "completion_callback" untouched, and the FREE_CALLBACK()s at the end to have "fangs" -- perform actual freeing. Pre-patch, nbd_unlocked_aio_block_status() conforms to this interface contract quite well. It first goes through all possible local error branches, and if all of those pass, only then does it zero the callback structures (i.e., takes ownership of them). This patch changes that; we take ownershop of "extent_callback" (by copying it to "shim" and nulling it) before we check our local error conditions, such as structured_replies and metadata context validity. That breaks the interface contract, and so we cannot rely on nbd_aio_block_status() for freeing "extent_callback" when we fail in one of those local error conditions -- this is why we have to go through FREE_CALLBACK (cb.fn.extent) on each of those error paths, for freeing *both* "shim" and the "extent_callback" internals. And (most importantly to me) then we return to the caller, namely nbd_aio_block_status(), in such a way that (a) we fail, yet (b) we have already destroyed "extent_callback" (but not "completion_callback"). The caller copes with this (due to how FREE_CALLBACK() works), but it's unclean. If nbd_unlocked_aio_block_status() fails, it shouldn't have messed with "extent_callback". (I notice a *slight* inconsistency with this requirement of mine in the pre-patch code as well, BTW: pre-patch, we null both callback structures *before* calling nbd_internal_command_common(); if the latter fails, we get into the same situation. So I actually tend to disagree with that too.) Anyway, I would suggest (on top of your patch): Which, after squashing, would result in a patch like: But, to repeat myself -- I'm not convinced any more we benefit from the 64-bit-only internal representation. We should always be able to tell precisely which one of the 32-bit and the 64-bit internal representations we need. We should handle each separately, and then we don't need the conversions + shim. Laszlo

Overcome the inherent 32-bit limitation of our existing nbd_block_status command by adding a 64-bit variant. The command sent to the server does not change, but the user's callback is now handed 64-bit information regardless of whether the server replies with 32- or 64-bit extents. Unit tests prove that the new API works in each of C, Python, OCaml, and Go bindings. We can also get rid of the temporary hack added to appease the compiler in an earlier patch. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/libnbd.pod | 18 +-- sh/nbdsh.pod | 2 +- generator/API.ml | 146 +++++++++++++++++++--- generator/OCaml.ml | 1 - generator/Python.ml | 1 - lib/rw.c | 48 +++++-- python/t/465-block-status-64.py | 56 +++++++++ ocaml/tests/Makefile.am | 1 + ocaml/tests/test_465_block_status_64.ml | 58 +++++++++ tests/meta-base-allocation.c | 104 +++++++++++++-- fuzzing/libnbd-fuzz-wrapper.c | 20 ++- golang/Makefile.am | 1 + golang/libnbd_465_block_status_64_test.go | 119 ++++++++++++++++++ 13 files changed, 527 insertions(+), 48 deletions(-) create mode 100644 python/t/465-block-status-64.py create mode 100644 ocaml/tests/test_465_block_status_64.ml create mode 100644 golang/libnbd_465_block_status_64_test.go diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 72f74053..ddbb09f3 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -661,14 +661,14 @@ In order to utilize block status, the client must call L<nbd_add_meta_context(3)> prior to connecting, for each meta context in which it is interested, then check L<nbd_can_meta_context(3)> after connection to see which contexts the server actually supports. If a -context is supported, the client can then use L<nbd_block_status(3)> -with a callback function that will receive an array of 32-bit integer -pairs describing consecutive extents within a context. In each pair, -the first integer is the length of the extent, the second is a bitmask -description of that extent (for the "base:allocation" context, the -bitmask may include C<LIBNBD_STATE_HOLE> for unallocated portions of -the file, and/or C<LIBNBD_STATE_ZERO> for portions of the file known -to read as zero). +context is supported, the client can then use +L<nbd_block_status_64(3)> with a callback function that will receive +an array of structs describing consecutive extents within a context. +Each struct gives the length of the extent, then a bitmask description +of that extent (for the "base:allocation" context, the bitmask may +include C<LIBNBD_STATE_HOLE> for unallocated portions of the file, +and/or C<LIBNBD_STATE_ZERO> for portions of the file known to read as +zero). There is a full example of requesting meta context and using block status available at @@ -917,7 +917,7 @@ will tie up resources until L<nbd_close(3)> is eventually reached). =head2 Callbacks with C<int *error> parameter Some of the high-level commands (L<nbd_pread_structured(3)>, -L<nbd_block_status(3)>) involve the use of a callback function invoked +L<nbd_block_status_64(3)>) involve the use of a callback function invoked by the state machine at appropriate points in the server's reply before the overall command is complete. These callback functions, along with all of the completion callbacks, include a parameter diff --git a/sh/nbdsh.pod b/sh/nbdsh.pod index d50b738a..d222c0ff 100644 --- a/sh/nbdsh.pod +++ b/sh/nbdsh.pod @@ -59,7 +59,7 @@ Display brief command line help and exit. =item B<--base-allocation> Request the use of the "base:allocation" meta context, which is the -most common context used with L<nbd_block_status(3)>. This is +most common context used with L<nbd_block_status_64(3)>. This is equivalent to calling S<C<h.set_meta_context(nbd.CONTEXT_BASE_ALLOCATION)>> in the shell prior to connecting, and works even when combined with C<--uri> (while diff --git a/generator/API.ml b/generator/API.ml index 1da1ec53..f04f4b79 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -1565,7 +1565,7 @@ "add_meta_context", { During connection libnbd can negotiate zero or more metadata contexts with the server. Metadata contexts are features (such as C<\"base:allocation\">) which describe information returned -by the L<nbd_block_status(3)> command (for C<\"base:allocation\"> +by the L<nbd_block_status_64(3)> command (for C<\"base:allocation\"> this is whether blocks of data are allocated, zero or sparse). This call adds one metadata context to the list to be negotiated. @@ -1592,7 +1592,7 @@ "add_meta_context", { Other metadata contexts are server-specific, but include C<\"qemu:dirty-bitmap:...\"> and C<\"qemu:allocation-depth\"> for qemu-nbd (see qemu-nbd I<-B> and I<-A> options)."; - see_also = [Link "block_status"; Link "can_meta_context"; + see_also = [Link "block_status_64"; Link "can_meta_context"; Link "get_nr_meta_contexts"; Link "get_meta_context"; Link "clear_meta_contexts"]; }; @@ -1605,14 +1605,14 @@ "get_nr_meta_contexts", { During connection libnbd can negotiate zero or more metadata contexts with the server. Metadata contexts are features (such as C<\"base:allocation\">) which describe information returned -by the L<nbd_block_status(3)> command (for C<\"base:allocation\"> +by the L<nbd_block_status_64(3)> command (for C<\"base:allocation\"> this is whether blocks of data are allocated, zero or sparse). This command returns how many meta contexts have been added to the list to request from the server via L<nbd_add_meta_context(3)>. The server is not obligated to honor all of the requests; to see what it actually supports, see L<nbd_can_meta_context(3)>."; - see_also = [Link "block_status"; Link "can_meta_context"; + see_also = [Link "block_status_64"; Link "can_meta_context"; Link "add_meta_context"; Link "get_meta_context"; Link "clear_meta_contexts"]; }; @@ -1625,13 +1625,13 @@ "get_meta_context", { During connection libnbd can negotiate zero or more metadata contexts with the server. Metadata contexts are features (such as C<\"base:allocation\">) which describe information returned -by the L<nbd_block_status(3)> command (for C<\"base:allocation\"> +by the L<nbd_block_status_64(3)> command (for C<\"base:allocation\"> this is whether blocks of data are allocated, zero or sparse). This command returns the i'th meta context request, as added by L<nbd_add_meta_context(3)>, and bounded by L<nbd_get_nr_meta_contexts(3)>."; - see_also = [Link "block_status"; Link "can_meta_context"; + see_also = [Link "block_status_64"; Link "can_meta_context"; Link "add_meta_context"; Link "get_nr_meta_contexts"; Link "clear_meta_contexts"]; }; @@ -1645,7 +1645,7 @@ "clear_meta_contexts", { During connection libnbd can negotiate zero or more metadata contexts with the server. Metadata contexts are features (such as C<\"base:allocation\">) which describe information returned -by the L<nbd_block_status(3)> command (for C<\"base:allocation\"> +by the L<nbd_block_status_64(3)> command (for C<\"base:allocation\"> this is whether blocks of data are allocated, zero or sparse). This command resets the list of meta contexts to request back to @@ -1654,7 +1654,7 @@ "clear_meta_contexts", { negotiation mode is selected (see L<nbd_set_opt_mode(3)>), for altering the list of attempted contexts between subsequent export queries."; - see_also = [Link "block_status"; Link "can_meta_context"; + see_also = [Link "block_status_64"; Link "can_meta_context"; Link "add_meta_context"; Link "get_nr_meta_contexts"; Link "get_meta_context"; Link "set_opt_mode"]; }; @@ -2281,7 +2281,7 @@ "can_meta_context", { ^ non_blocking_test_call_description; see_also = [SectionLink "Flag calls"; Link "opt_info"; Link "add_meta_context"; - Link "block_status"; Link "aio_block_status"; + Link "block_status_64"; Link "aio_block_status_64"; Link "set_request_meta_context"; Link "opt_set_meta_context"]; }; @@ -2712,7 +2712,7 @@ "block_status", { optargs = [ OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; ret = RErr; permitted_states = [ Connected ]; - shortdesc = "send block status command to the NBD server"; + shortdesc = "send block status command, with 32-bit callback"; longdesc = "\ Issue the block status command to the NBD server. If supported by the server, this causes metadata context @@ -2727,7 +2727,15 @@ "block_status", { The NBD protocol does not yet have a way for a client to learn if the server will enforce an even smaller maximum block status size, although a future extension may add a constraint visible in -L<nbd_get_block_size(3)>. +L<nbd_get_block_size(3)>. Furthermore, this function is inherently +limited to 32-bit values. If the server replies with a larger +extent, the length of that extent will be truncated to just +below 32 bits and any further extents from the server will be +ignored. If the server replies with a status value larger than +32 bits (only possible when extended headers are in use), the +callback function will be passed an C<EOVERFLOW> error. To get +the full extent information from a server that supports 64-bit +extents, you must use L<nbd_block_status_64(3)>. Depending on which metadata contexts were enabled before connecting (see L<nbd_add_meta_context(3)>) and which are @@ -2747,7 +2755,7 @@ "block_status", { array is an array of pairs of integers with the first entry in each pair being the length (in bytes) of the block and the second entry being a status/flags field which is specific to the metadata context. -(The number of pairs passed to the function is C<nr_entries/2>.) The +The number of pairs passed to the function is C<nr_entries/2>. The NBD protocol document in the section about C<NBD_REPLY_TYPE_BLOCK_STATUS> describes the meaning of this array; for contexts known to libnbd, B<E<lt>libnbd.hE<gt>> contains constants @@ -2771,10 +2779,79 @@ "block_status", { does not exceed C<count> bytes; however, libnbd does not validate that the server obeyed the flag." ^ strict_call_description; - see_also = [Link "add_meta_context"; Link "can_meta_context"; + see_also = [Link "block_status_64"; + Link "add_meta_context"; Link "can_meta_context"; Link "aio_block_status"; Link "set_strict_mode"]; }; + "block_status_64", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; Closure extent64_closure ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; + ret = RErr; + permitted_states = [ Connected ]; + shortdesc = "send block status command, with 64-bit callback"; + longdesc = "\ +Issue the block status command to the NBD server. If +supported by the server, this causes metadata context +information about blocks beginning from the specified +offset to be returned. The C<count> parameter is a hint: the +server may choose to return less status, or the final block +may extend beyond the requested range. If multiple contexts +are supported, the number of blocks and cumulative length +of those blocks need not be identical between contexts. + +Note that not all servers can support a C<count> of 4GiB or larger. +The NBD protocol does not yet have a way for a client to learn if +the server will enforce an even smaller maximum block status size, +although a future extension may add a constraint visible in +L<nbd_get_block_size(3)>. + +Depending on which metadata contexts were enabled before +connecting (see L<nbd_add_meta_context(3)>) and which are +supported by the server (see L<nbd_can_meta_context(3)>) this call +returns information about extents by calling back to the +C<extent64> function. The callback cannot call C<nbd_*> APIs on the +same handle since it holds the handle lock and will +cause a deadlock. If the callback returns C<-1>, and no earlier +error has been detected, then the overall block status command +will fail with any non-zero value stored into the callback's +C<error> parameter (with a default of C<EPROTO>); but any further +contexts will still invoke the callback. + +The C<extent64> function is called once per type of metadata available, +with the C<user_data> passed to this function. The C<metacontext> +parameter is a string such as C<\"base:allocation\">. The C<entries> +array is an array of B<nbd_extent> structs, containing length (in bytes) +of the block and a status/flags field which is specific to the metadata +context. The number of array entries passed to the function is +C<nr_entries>. The NBD protocol document in the section about +C<NBD_REPLY_TYPE_BLOCK_STATUS> describes the meaning of this array; +for contexts known to libnbd, B<E<lt>libnbd.hE<gt>> contains constants +beginning with C<LIBNBD_STATE_> that may help decipher the values. +On entry to the callback, the C<error> parameter contains the errno +value of any previously detected error. + +It is possible for the extent function to be called +more times than you expect (if the server is buggy), +so always check the C<metacontext> field to ensure you +are receiving the data you expect. It is also possible +that the extent function is not called at all, even for +metadata contexts that you requested. This indicates +either that the server doesn't support the context +or for some other reason cannot return the data. + +The C<flags> parameter may be C<0> for no flags, or may contain +C<LIBNBD_CMD_FLAG_REQ_ONE> meaning that the server should +return only one extent per metadata context where that extent +does not exceed C<count> bytes; however, libnbd does not +validate that the server obeyed the flag." +^ strict_call_description; + see_also = [Link "block_status"; + Link "add_meta_context"; Link "can_meta_context"; + Link "aio_block_status_64"; Link "set_strict_mode"]; + }; + "poll", { default_call with args = [ Int "timeout" ]; ret = RInt; @@ -3369,7 +3446,7 @@ "aio_block_status", { OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; ret = RCookie; permitted_states = [ Connected ]; - shortdesc = "send block status command to the NBD server"; + shortdesc = "send block status command, with 32-bit callback"; longdesc = "\ Send the block status command to the NBD server. @@ -3377,13 +3454,48 @@ "aio_block_status", { Or supply the optional C<completion_callback> which will be invoked as described in L<libnbd(3)/Completion callbacks>. -Other parameters behave as documented in L<nbd_block_status(3)>." +Other parameters behave as documented in L<nbd_block_status(3)>. + +This function is inherently limited to 32-bit values. If the +server replies with a larger extent, the length of that extent +will be truncated to just below 32 bits and any further extents +from the server will be ignored. If the server replies with a +status value larger than 32 bits (only possible when extended +headers are in use), the callback function will be passed an +C<EOVERFLOW> error. To get the full extent information from a +server that supports 64-bit extents, you must use +L<nbd_aio_block_status_64(3)>. +" ^ strict_call_description; see_also = [SectionLink "Issuing asynchronous commands"; + Link "aio_block_status_64"; Link "can_meta_context"; Link "block_status"; Link "set_strict_mode"]; }; + "aio_block_status_64", { + default_call with + args = [ UInt64 "count"; UInt64 "offset"; Closure extent64_closure ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; + ret = RCookie; + permitted_states = [ Connected ]; + shortdesc = "send block status command, with 64-bit callback"; + longdesc = "\ +Send the block status command to the NBD server. + +To check if the command completed, call L<nbd_aio_command_completed(3)>. +Or supply the optional C<completion_callback> which will be invoked +as described in L<libnbd(3)/Completion callbacks>. + +Other parameters behave as documented in L<nbd_block_status_64(3)>." +^ strict_call_description; + see_also = [SectionLink "Issuing asynchronous commands"; + Link "aio_block_status"; + Link "can_meta_context"; Link "block_status_64"; + Link "set_strict_mode"]; + }; + "aio_get_fd", { default_call with args = []; ret = RFd; @@ -3909,6 +4021,10 @@ let first_version = "set_socket_activation_name", (1, 16); "get_socket_activation_name", (1, 16); + (* Added in 1.17.x development cycle, will be stable and supported in 1.18. *) + "block_status_64", (1, 18); + "aio_block_status_64", (1, 18); + (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. "get_tls_certificates", (1, ??); diff --git a/generator/OCaml.ml b/generator/OCaml.ml index ea3b044a..b05cd1d2 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -593,7 +593,6 @@ 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"; diff --git a/generator/Python.ml b/generator/Python.ml index ccac5fed..6f87b0cf 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -158,7 +158,6 @@ 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"; diff --git a/lib/rw.c b/lib/rw.c index 58b05a4b..bea55fa1 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -205,7 +205,7 @@ nbd_unlocked_zero (struct nbd_handle *h, return wait_for_command (h, cookie); } -/* Issue a block status command and wait for the reply. */ +/* Issue a block status command and wait for the reply, 32-bit callback. */ int nbd_unlocked_block_status (struct nbd_handle *h, uint64_t count, uint64_t offset, @@ -223,6 +223,25 @@ nbd_unlocked_block_status (struct nbd_handle *h, return wait_for_command (h, cookie); } +/* Issue a block status command and wait for the reply, 64-bit callback. */ +int +nbd_unlocked_block_status_64 (struct nbd_handle *h, + uint64_t count, uint64_t offset, + nbd_extent64_callback *extent64, + uint32_t flags) +{ + int64_t cookie; + nbd_completion_callback c = NBD_NULL_COMPLETION; + + cookie = nbd_unlocked_aio_block_status_64 (h, count, offset, extent64, &c, + flags); + if (cookie == -1) + return -1; + + assert (CALLBACK_IS_NULL (*extent64)); + return wait_for_command (h, cookie); +} + /* count_err represents the errno to return if bounds check fail */ int64_t nbd_internal_command_common (struct nbd_handle *h, @@ -543,10 +562,10 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, uint32_t flags) { nbd_extent_callback *shim = malloc (sizeof *shim); - struct command_cb cb = { .fn.extent.callback = nbd_convert_extent, - .fn.extent.user_data = shim, - .fn.extent.free = nbd_convert_extent_free, - .completion = *completion }; + nbd_extent64_callback wrapper = { .callback = nbd_convert_extent, + .user_data = shim, + .free = nbd_convert_extent_free, }; + int ret; if (shim == NULL) { set_error (errno, "malloc"); @@ -555,10 +574,25 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, *shim = *extent; SET_CALLBACK_TO_NULL (*extent); + ret = nbd_unlocked_aio_block_status_64 (h, count, offset, &wrapper, + completion, flags); + FREE_CALLBACK (wrapper); + return ret; +} + +int64_t +nbd_unlocked_aio_block_status_64 (struct nbd_handle *h, + uint64_t count, uint64_t offset, + nbd_extent64_callback *extent64, + nbd_completion_callback *completion, + uint32_t flags) +{ + struct command_cb cb = { .fn.extent = *extent64, + .completion = *completion }; + if (h->strict & LIBNBD_STRICT_COMMANDS) { if (!h->structured_replies) { set_error (ENOTSUP, "server does not support structured replies"); - FREE_CALLBACK (cb.fn.extent); return -1; } @@ -566,11 +600,11 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, set_error (ENOTSUP, "did not negotiate any metadata contexts, " "either you did not call nbd_add_meta_context before " "connecting or the server does not support it"); - FREE_CALLBACK (cb.fn.extent); return -1; } } + SET_CALLBACK_TO_NULL (*extent64); SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS, offset, count, EINVAL, NULL, &cb); diff --git a/python/t/465-block-status-64.py b/python/t/465-block-status-64.py new file mode 100644 index 00000000..a8a8eaea --- /dev/null +++ b/python/t/465-block-status-64.py @@ -0,0 +1,56 @@ +# libnbd Python bindings +# Copyright Red Hat +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import os + +import nbd + +script = "%s/../tests/meta-base-allocation.sh" % os.getenv("srcdir", ".") + +h = nbd.NBD() +h.add_meta_context("base:allocation") +h.connect_command(["nbdkit", "-s", "--exit-with-parent", "-v", "sh", script]) + +entries = [] + + +def f(user_data, metacontext, offset, e, err): + global entries + assert user_data == 42 + assert err.value == 0 + if metacontext != "base:allocation": + return + entries = e + + +h.block_status_64(65536, 0, lambda *args: f(42, *args)) +print("entries = %r" % entries) +assert entries == [(8192, 0), + (8192, 1), + (16384, 3), + (16384, 2), + (16384, 0)] + +h.block_status_64(1024, 32256, lambda *args: f(42, *args)) +print("entries = %r" % entries) +assert entries == [(512, 3), + (16384, 2)] + +h.block_status_64(1024, 32256, lambda *args: f(42, *args), + nbd.CMD_FLAG_REQ_ONE) +print("entries = %r" % entries) +assert entries == [(512, 3)] diff --git a/ocaml/tests/Makefile.am b/ocaml/tests/Makefile.am index 20546350..4ddd0f9f 100644 --- a/ocaml/tests/Makefile.am +++ b/ocaml/tests/Makefile.am @@ -40,6 +40,7 @@ ML_TESTS = \ test_405_pread_structured.ml \ test_410_pwrite.ml \ test_460_block_status.ml \ + test_465_block_status_64.ml \ test_500_aio_pread.ml \ test_505_aio_pread_structured_callback.ml \ test_510_aio_pwrite.ml \ diff --git a/ocaml/tests/test_465_block_status_64.ml b/ocaml/tests/test_465_block_status_64.ml new file mode 100644 index 00000000..0634a025 --- /dev/null +++ b/ocaml/tests/test_465_block_status_64.ml @@ -0,0 +1,58 @@ +(* hey emacs, this is OCaml code: -*- tuareg -*- *) +(* libnbd OCaml test case + * Copyright Red Hat + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + *) + +open Printf + +let script = + try + let srcdir = Sys.getenv "srcdir" in + sprintf "%s/../../tests/meta-base-allocation.sh" srcdir + with + Not_found -> failwith "error: srcdir is not defined" + +let entries = ref [||] +let f user_data metacontext offset e err = + assert (user_data = 42); + assert (!err = 0); + if metacontext = "base:allocation" then + entries := e; + 0 + +let () = + let nbd = NBD.create () in + NBD.add_meta_context nbd "base:allocation"; + NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "-v"; + "sh"; script]; + + NBD.block_status_64 nbd 65536_L 0_L (f 42); + assert (!entries = [| 8192_L, 0_L; + 8192_L, 1_L; + 16384_L, 3_L; + 16384_L, 2_L; + 16384_L, 0_L |]); + + NBD.block_status_64 nbd 1024_L 32256_L (f 42); + assert (!entries = [| 512_L, 3_L; + 16384_L, 2_L |]); + + let flags = let open NBD.CMD_FLAG in [REQ_ONE] in + NBD.block_status_64 nbd 1024_L 32256_L (f 42) ~flags; + assert (!entries = [| 512_L, 3_L |]) + +let () = Gc.compact () diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c index a7b3af09..7697b5da 100644 --- a/tests/meta-base-allocation.c +++ b/tests/meta-base-allocation.c @@ -32,10 +32,13 @@ #define BOGUS_CONTEXT "x-libnbd:nosuch" -static int check_extent (void *data, - const char *metacontext, - uint64_t offset, - uint32_t *entries, size_t nr_entries, int *error); +static int check_extent32 (void *data, const char *metacontext, + uint64_t offset, + uint32_t *entries, size_t nr_entries, int *error); + +static int check_extent64 (void *data, const char *metacontext, + uint64_t offset, + nbd_extent *entries, size_t nr_entries, int *error); int main (int argc, char *argv[]) @@ -43,8 +46,10 @@ main (int argc, char *argv[]) struct nbd_handle *nbd; char plugin_path[256]; int id; - nbd_extent_callback extent_callback = { .callback = check_extent, - .user_data = &id }; + nbd_extent_callback extent32_callback = { .callback = check_extent32, + .user_data = &id }; + nbd_extent64_callback extent64_callback = { .callback = check_extent64, + .user_data = &id }; int r; const char *s; char *tmp; @@ -150,23 +155,36 @@ main (int argc, char *argv[]) /* Read the block status. */ id = 1; - if (nbd_block_status (nbd, 65536, 0, extent_callback, 0) == -1) { + if (nbd_block_status (nbd, 65536, 0, extent32_callback, 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_block_status_64 (nbd, 65536, 0, extent64_callback, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } id = 2; - if (nbd_block_status (nbd, 1024, 32768-512, extent_callback, 0) == -1) { + if (nbd_block_status (nbd, 1024, 32768-512, extent32_callback, 0) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_block_status_64 (nbd, 1024, 32768-512, extent64_callback, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } id = 3; - if (nbd_block_status (nbd, 1024, 32768-512, extent_callback, + if (nbd_block_status (nbd, 1024, 32768-512, extent32_callback, LIBNBD_CMD_FLAG_REQ_ONE) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } + if (nbd_block_status_64 (nbd, 1024, 32768-512, extent64_callback, + LIBNBD_CMD_FLAG_REQ_ONE) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } if (nbd_shutdown (nbd, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); @@ -178,10 +196,8 @@ main (int argc, char *argv[]) } static int -check_extent (void *data, - const char *metacontext, - uint64_t offset, - uint32_t *entries, size_t nr_entries, int *error) +check_extent32 (void *data, const char *metacontext, uint64_t offset, + uint32_t *entries, size_t nr_entries, int *error) { size_t i; int id; @@ -235,3 +251,65 @@ check_extent (void *data, return 0; } + +static int +check_extent64 (void *data, const char *metacontext, uint64_t offset, + nbd_extent *entries, size_t nr_entries, int *error) +{ + size_t i; + int id; + + id = * (int *)data; + + printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ", " + "nr_entries=%zu, error=%d\n", + id, metacontext, offset, nr_entries, *error); + + assert (*error == 0); + if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) { + for (i = 0; i < nr_entries; i++) { + printf ("\t%zu\tlength=%" PRIu64 ", status=%" PRIu64 "\n", + i, entries[i].length, entries[i].flags); + } + fflush (stdout); + + switch (id) { + case 1: + assert (nr_entries == 5); + assert (entries[0].length == 8192); + assert (entries[0].flags == 0); + assert (entries[1].length == 8192); + assert (entries[1].flags == LIBNBD_STATE_HOLE); + assert (entries[2].length == 16384); + assert (entries[2].flags == (LIBNBD_STATE_HOLE|LIBNBD_STATE_ZERO)); + assert (entries[3].length == 16384); + assert (entries[3].flags == LIBNBD_STATE_ZERO); + assert (entries[4].length == 16384); + assert (entries[4].flags == 0); + break; + + case 2: + assert (nr_entries == 2); + assert (entries[0].length == 512); + assert (entries[0].flags == (LIBNBD_STATE_HOLE|LIBNBD_STATE_ZERO)); + assert (entries[1].length == 16384); + assert (entries[1].flags == LIBNBD_STATE_ZERO); + break; + + case 3: + assert (nr_entries == 1); + assert (entries[0].length == 512); + assert (entries[0].flags == (LIBNBD_STATE_HOLE|LIBNBD_STATE_ZERO)); + break; + + default: + abort (); + } + + } + else + fprintf (stderr, "warning: ignored unexpected meta context %s\n", + metacontext); + + return 0; +} diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c index e111866f..cbd55380 100644 --- a/fuzzing/libnbd-fuzz-wrapper.c +++ b/fuzzing/libnbd-fuzz-wrapper.c @@ -165,6 +165,17 @@ extent_callback (void *user_data, return 0; } +/* Block status (extents64) callback, does nothing. */ +static int +extent64_callback (void *user_data, + const char *metacontext, + uint64_t offset, nbd_extent *entries, + size_t nr_entries, int *error) +{ + //fprintf (stderr, "extent64 called, nr_entries = %zu\n", nr_entries); + return 0; +} + /* This is the client (parent process) running libnbd. */ static char buf[512]; static char prbuf[65536]; @@ -213,7 +224,7 @@ client (int sock) }, 0); - /* Test block status. */ + /* Test both sizes of block status. */ nbd_block_status (nbd, length, 0, (nbd_extent_callback) { .callback = extent_callback, @@ -221,6 +232,13 @@ client (int sock) .free = NULL }, 0); + nbd_block_status_64 (nbd, length, 0, + (nbd_extent64_callback) { + .callback = extent64_callback, + .user_data = NULL, + .free = NULL + }, + 0); nbd_shutdown (nbd, 0); } diff --git a/golang/Makefile.am b/golang/Makefile.am index fac65248..b4151250 100644 --- a/golang/Makefile.am +++ b/golang/Makefile.am @@ -43,6 +43,7 @@ source_files = \ libnbd_405_pread_structured_test.go \ libnbd_410_pwrite_test.go \ libnbd_460_block_status_test.go \ + libnbd_465_block_status_64_test.go \ libnbd_500_aio_pread_test.go \ libnbd_510_aio_pwrite_test.go \ libnbd_590_aio_copy_test.go \ diff --git a/golang/libnbd_465_block_status_64_test.go b/golang/libnbd_465_block_status_64_test.go new file mode 100644 index 00000000..7659a21d --- /dev/null +++ b/golang/libnbd_465_block_status_64_test.go @@ -0,0 +1,119 @@ +/* libnbd golang tests + * Copyright Red Hat + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +package libnbd + +import ( + "fmt" + "os" + "strings" + "testing" +) + +var entries64 []LibnbdExtent + +func mcf64(metacontext string, offset uint64, e []LibnbdExtent, error *int) int { + if *error != 0 { + panic("expected *error == 0") + } + if metacontext == "base:allocation" { + entries64 = e + } + return 0 +} + +// Seriously WTF? +func mc64_compare(a1 []LibnbdExtent, a2 []LibnbdExtent) bool { + if len(a1) != len(a2) { + return false + } + for i := 0; i < len(a1); i++ { + if a1[i] != a2[i] { + return false + } + } + return true +} + +func mc64_to_string(a []LibnbdExtent) string { + ss := make([]string, len(a)) + for i := 0; i < len(a); i++ { + ss[i] = fmt.Sprintf("%#v", a[i]) + } + return strings.Join(ss, ", ") +} + +func Test465BlockStatus64(t *testing.T) { + srcdir := os.Getenv("abs_top_srcdir") + script := srcdir + "/tests/meta-base-allocation.sh" + + h, err := Create() + if err != nil { + t.Fatalf("could not create handle: %s", err) + } + defer h.Close() + + err = h.AddMetaContext("base:allocation") + if err != nil { + t.Fatalf("%s", err) + } + err = h.ConnectCommand([]string{ + "nbdkit", "-s", "--exit-with-parent", "-v", + "sh", script, + }) + if err != nil { + t.Fatalf("%s", err) + } + + err = h.BlockStatus64(65536, 0, mcf64, nil) + if err != nil { + t.Fatalf("%s", err) + } + if !mc64_compare(entries64, []LibnbdExtent{ + {8192, 0}, + {8192, 1}, + {16384, 3}, + {16384, 2}, + {16384, 0}, + }) { + t.Fatalf("unexpected entries (1): %s", mc64_to_string(entries64)) + } + + err = h.BlockStatus64(1024, 32256, mcf64, nil) + if err != nil { + t.Fatalf("%s", err) + } + if !mc64_compare(entries64, []LibnbdExtent{ + {512, 3}, + {16384, 2}, + }) { + t.Fatalf("unexpected entries (2): %s", mc64_to_string(entries64)) + } + + var optargs BlockStatus64Optargs + optargs.FlagsSet = true + optargs.Flags = CMD_FLAG_REQ_ONE + err = h.BlockStatus64(1024, 32256, mcf64, &optargs) + if err != nil { + t.Fatalf("%s", err) + } + if !mc64_compare(entries64, []LibnbdExtent{{512, 3}}) { + t.Fatalf("unexpected entries (3): %s", mc64_to_string(entries64)) + } + +} -- 2.40.1

Although our use of "base:allocation" doesn't require the use of the 64-bit API for flags, we might perform slightly faster for a server that does give us 64-bit extent lengths and honors larger nbd_zero lengths. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- copy/nbd-ops.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c index f3b3bed3..71ee0a3b 100644 --- a/copy/nbd-ops.c +++ b/copy/nbd-ops.c @@ -428,7 +428,7 @@ nbd_ops_asynch_notify_write (struct rw *rw, size_t index) * request for extents in a single round trip. */ static int add_extent (void *vp, const char *metacontext, - uint64_t offset, uint32_t *entries, size_t nr_entries, + uint64_t offset, nbd_extent *entries, size_t nr_entries, int *error); static void @@ -449,11 +449,11 @@ nbd_ops_get_extents (struct rw *rw, size_t index, size_t i; exts.len = 0; - if (nbd_block_status (nbd, count, offset, - (nbd_extent_callback) { - .user_data = &exts, - .callback = add_extent - }, 0) == -1) { + if (nbd_block_status_64 (nbd, count, offset, + (nbd_extent64_callback) { + .user_data = &exts, + .callback = add_extent + }, 0) == -1) { /* XXX We could call default_get_extents, but unclear if it's * the right thing to do if the server is returning errors. */ @@ -496,7 +496,7 @@ nbd_ops_get_extents (struct rw *rw, size_t index, static int add_extent (void *vp, const char *metacontext, - uint64_t offset, uint32_t *entries, size_t nr_entries, + uint64_t offset, nbd_extent *entries, size_t nr_entries, int *error) { extent_list *ret = vp; @@ -505,25 +505,25 @@ add_extent (void *vp, const char *metacontext, if (strcmp (metacontext, "base:allocation") != 0 || *error) return 0; - for (i = 0; i < nr_entries; i += 2) { + for (i = 0; i < nr_entries; i++) { struct extent e; e.offset = offset; - e.length = entries[i]; + e.length = entries[i].length; /* Note we deliberately don't care about the HOLE flag. There is * no need to read extent that reads as zeroes. We will convert * to it to a hole or allocated extents based on the command line * arguments. */ - e.zero = (entries[i+1] & LIBNBD_STATE_ZERO) != 0; + e.zero = (entries[i].flags & LIBNBD_STATE_ZERO) != 0; if (extent_list_append (ret, e) == -1) { perror ("realloc"); exit (EXIT_FAILURE); } - offset += entries[i]; + offset += entries[i].length; } return 0; -- 2.40.1

Although our use of "base:allocation" doesn't require the use of the 64-bit API for flags, we might perform slightly faster for a server that does give us 64-bit extent lengths. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- dump/dump.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/dump/dump.c b/dump/dump.c index b4aebe84..71053277 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -38,7 +38,7 @@ #include "version.h" #include "vector.h" -DEFINE_VECTOR_TYPE (uint32_vector, uint32_t); +DEFINE_VECTOR_TYPE (uint64_vector, uint64_t); static const char *progname; static struct nbd_handle *nbd; @@ -262,10 +262,10 @@ catch_signal (int sig) static int extent_callback (void *user_data, const char *metacontext, uint64_t offset, - uint32_t *entries, size_t nr_entries, + nbd_extent *entries, size_t nr_entries, int *error) { - uint32_vector *list = user_data; + uint64_vector *list = user_data; size_t i; if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) != 0) @@ -273,7 +273,8 @@ extent_callback (void *user_data, const char *metacontext, /* Just append the entries we got to the list. */ for (i = 0; i < nr_entries; ++i) { - if (uint32_vector_append (list, entries[i]) == -1) { + if (uint64_vector_append (list, entries[i].length) == -1 || + uint64_vector_append (list, entries[i].flags) == -1) { perror ("realloc"); exit (EXIT_FAILURE); } @@ -284,7 +285,7 @@ extent_callback (void *user_data, const char *metacontext, static bool test_all_zeroes (uint64_t offset, size_t count) { - uint32_vector entries = empty_vector; + uint64_vector entries = empty_vector; size_t i; uint64_t count_read; @@ -296,22 +297,22 @@ test_all_zeroes (uint64_t offset, size_t count) * false, causing the main code to do a full read. We could be * smarter and keep asking the server (XXX). */ - if (nbd_block_status (nbd, count, offset, - (nbd_extent_callback) { - .callback = extent_callback, - .user_data = &entries }, - 0) == -1) { + if (nbd_block_status_64 (nbd, count, offset, + (nbd_extent64_callback) { + .callback = extent_callback, + .user_data = &entries }, + 0) == -1) { fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); exit (EXIT_FAILURE); } count_read = 0; for (i = 0; i < entries.len; i += 2) { - uint32_t len = entries.ptr[i]; - uint32_t type = entries.ptr[i+1]; + uint64_t len = entries.ptr[i]; + uint64_t type = entries.ptr[i+1]; count_read += len; - if (!(type & 2)) /* not zero */ + if (!(type & LIBNBD_STATE_ZERO)) /* not zero */ return false; } -- 2.40.1

Add another bit of overall server information, as well as a '--can extended-headers' silent query. For now, the testsuite is written assuming that when nbdkit finally adds extended headers support, it will also add a --no-eh kill switch comparable to its existing --no-sr switch. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- info/nbdinfo.pod | 11 ++++++++++- info/can.c | 9 +++++++++ info/info-can.sh | 27 +++++++++++++++++++++++++++ info/info-packets.sh | 17 ++++++++++++++++- info/main.c | 7 ++++++- 5 files changed, 68 insertions(+), 3 deletions(-) diff --git a/info/nbdinfo.pod b/info/nbdinfo.pod index 7eb3c1a0..9ea4a278 100644 --- a/info/nbdinfo.pod +++ b/info/nbdinfo.pod @@ -86,6 +86,7 @@ the I<--json> parameter: "protocol": "newstyle-fixed", "TLS": false, "structured": true, + "extended": false, "exports": [ { "export-name": "", @@ -165,6 +166,11 @@ Test if the NBD URI connection is using TLS. Test if server can respond with structured replies (a prerequisite for supporting block status commands). +=item nbdinfo --can extended-headers URI + +Test if server supports extended headers (a prerequisite for +supporting 64-bit commands; implies structured replies as well). + =item nbdinfo --is rotational URI Test if the server export is backed by something which behaves like a @@ -312,6 +318,8 @@ Display brief command line help and exit. =item B<--can df> +=item B<--can extended-headers> + =item B<--can fast-zero> =item B<--can flush> @@ -341,7 +349,8 @@ and the following libnbd functions: L<nbd_can_cache(3)>, L<nbd_can_df(3)>, L<nbd_can_fast_zero(3)>, L<nbd_can_flush(3)>, L<nbd_can_fua(3)>, L<nbd_can_multi_conn(3)>, L<nbd_can_trim(3)>, L<nbd_can_zero(3)>, L<nbd_is_read_only(3)>, -L<nbd_get_structured_replies_negotiated(3)>. +L<nbd_get_structured_replies_negotiated(3)>, +L<nbd_get_extended_headers_negotiated(3)>. =item B<--color> diff --git a/info/can.c b/info/can.c index 01ab4806..31c4a1ca 100644 --- a/info/can.c +++ b/info/can.c @@ -50,6 +50,15 @@ do_can (void) strcasecmp (can, "structured_replies") == 0) feature = nbd_get_structured_replies_negotiated (nbd); + else if (strcasecmp (can, "eh") == 0 || + strcasecmp (can, "extended header") == 0 || + strcasecmp (can, "extended-header") == 0 || + strcasecmp (can, "extended_header") == 0 || + strcasecmp (can, "extended headers") == 0 || + strcasecmp (can, "extended-headers") == 0 || + strcasecmp (can, "extended_headers") == 0) + feature = nbd_get_extended_headers_negotiated (nbd); + else if (strcasecmp (can, "readonly") == 0 || strcasecmp (can, "read-only") == 0 || strcasecmp (can, "read_only") == 0) diff --git a/info/info-can.sh b/info/info-can.sh index 6cc8cbf4..8154d1ce 100755 --- a/info/info-can.sh +++ b/info/info-can.sh @@ -61,6 +61,33 @@ esac EOF test $st = 2 +# --can extended-headers cannot be positively tested until nbdkit gains +# --no-eh support. Otherwise, it is similar to --can structured-reply. + +no_eh= +if nbdkit --no-eh --help >/dev/null 2>/dev/null; then + no_eh=--no-eh + nbdkit -v -U - sh - \ + --run '$VG nbdinfo --can extended-headers "nbd+unix:///?socket=$unixsocket"' <<'EOF' +case "$1" in + get_size) echo 1024 ;; + pread) ;; + *) exit 2 ;; +esac +EOF +fi + +st=0 +nbdkit -v -U - $no_eh sh - \ + --run '$VG nbdinfo --can extended-headers "nbd+unix:///?socket=$unixsocket"' <<'EOF' || st=$? +case "$1" in + get_size) echo 1024 ;; + pread) ;; + *) exit 2 ;; +esac +EOF +test $st = 2 + # --can cache and --can fua require special handling because in # nbdkit-sh-plugin we must print "native" or "none". Also the can_fua # flag is only sent if the export is writable (hence can_write below). diff --git a/info/info-packets.sh b/info/info-packets.sh index 2460052e..410faef8 100755 --- a/info/info-packets.sh +++ b/info/info-packets.sh @@ -27,12 +27,27 @@ requires nbdkit --no-sr memory --version out=info-packets.out cleanup_fn rm -f $out +# Older nbdkit does not support extended headers; --no-eh is a reliable +# witness of whether nbdkit is new enough. + +no_eh= +if nbdkit --no-eh --help >/dev/null 2>/dev/null; then + no_eh=--no-eh +fi + nbdkit --no-sr -U - memory size=1M \ --run '$VG nbdinfo "nbd+unix:///?socket=$unixsocket"' > $out cat $out grep "protocol: .*using simple packets" $out -nbdkit -U - memory size=1M \ +nbdkit $no_eh -U - memory size=1M \ --run '$VG nbdinfo "nbd+unix:///?socket=$unixsocket"' > $out cat $out grep "protocol: .*using structured packets" $out + +if test x != "x$no_eh"; then + nbdkit -U - memory size=1M \ + --run '$VG nbdinfo "nbd+unix:///?socket=$unixsocket"' > $out + cat $out + grep "protocol: .*using extended packets" $out +fi diff --git a/info/main.c b/info/main.c index 1b99e089..8c923266 100644 --- a/info/main.c +++ b/info/main.c @@ -302,11 +302,13 @@ main (int argc, char *argv[]) const char *protocol; int tls_negotiated; int sr_negotiated; + int eh_negotiated; /* Print per-connection fields. */ protocol = nbd_get_protocol (nbd); tls_negotiated = nbd_get_tls_negotiated (nbd); sr_negotiated = nbd_get_structured_replies_negotiated (nbd); + eh_negotiated = nbd_get_extended_headers_negotiated (nbd); if (!json_output) { if (protocol) { @@ -314,8 +316,9 @@ main (int argc, char *argv[]) fprintf (fp, "protocol: %s", protocol); if (tls_negotiated >= 0) fprintf (fp, " %s TLS", tls_negotiated ? "with" : "without"); - if (sr_negotiated >= 0) + if (eh_negotiated >= 0 && sr_negotiated >= 0) fprintf (fp, ", using %s packets", + eh_negotiated ? "extended" : sr_negotiated ? "structured" : "simple"); fprintf (fp, "\n"); ansi_restore (fp); @@ -333,6 +336,8 @@ main (int argc, char *argv[]) fprintf (fp, "\"TLS\": %s,\n", tls_negotiated ? "true" : "false"); if (sr_negotiated >= 0) fprintf (fp, "\"structured\": %s,\n", sr_negotiated ? "true" : "false"); + if (eh_negotiated >= 0) + fprintf (fp, "\"extended\": %s,\n", eh_negotiated ? "true" : "false"); } if (!list_all) -- 2.40.1

On Thu, May 25, 2023 at 08:01:01AM -0500, Eric Blake wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- 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