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.
Laszlo