Libguestfs - May 2023 - [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.

Laszlo

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org