Re: [Libguestfs] [libnbd PATCH v3 05/22] states: Prepare to receive 64-bit replies

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