Re: [Libguestfs] [libnbd PATCH v3 02/22] internal: Refactor layout of replies in sbuf

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