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

[Laszlo Ersek]

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.

Laszlo