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

[Laszlo Ersek]

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