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

On 5/26/23 17:53, Laszlo Ersek wrote: > 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. This (pre-patch) part: /* NB: This works for both simple and structured replies because the * handle (our cookie) is stored at the same offset. */ [...] cookie = be64toh (h->sbuf.simple_reply.handle); is disconcerting as well. I think it's well-defined C, but a hack nonetheless. IMO, unions are justified for two purposes: - deliberately reinterpreting one object representation as another - saving space, when at most one of N objects is expected to exist at any given time. Both of those uses follow from intentional elements of a design. But the fact that "handle" is at the same offset in both "simple" and "structured" is totally arbitrary. IMO this is a hack.