On 5/26/23 23:26, Eric Blake wrote: > On Fri, May 26, 2023 at 06:09:00PM +0200, Laszlo Ersek wrote: >> 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. > > It is not completely arbitrary: when structured replies were added to > the NBD spec, the choice of having handle at the same offset was > intentional. Similarly, extended replies have it at the same offset > as well. But a STATIC_ASSERT proving that would go a long way to > proving our intent, more than just a comment in the code.
Right, a STATIC_ASSERT would be great. Thanks! Laszlo _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs