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

[Laszlo Ersek]

On 5/26/23 23:06, Eric Blake wrote:

> I should indeed try harder to follow your useful example of generating
> specific patches with more than the default 3 lines of context, when
> it would make review easier.  Alas, 'git format-patch' doesn't seem to
> have an easy way to pick a different context size on a per-patch
> basis, so this basically implies writing (or finding and reusing
> existing) wrapper tooling to automate that.

If it's of any help, please see my script attached. It lets me put
"context:-W" and "context:-U5" style hints in the Notes sections of the
patches (with git-notes).

>>> diff --git a/generator/states-reply-structured.c 
>>> b/generator/states-reply-structured.c
>>> index 96182222..6f96945a 100644
>>> --- a/generator/states-reply-structured.c
>>> +++ b/generator/states-reply-structured.c
>>> @@ -49,9 +49,9 @@  REPLY.STRUCTURED_REPLY.START:
>>>     * so read the remaining part.
>>>     */
>>>    h->rbuf = &h->sbuf;
>>> -  h->rbuf = (char *)h->rbuf + sizeof h->sbuf.simple_reply;
>>> -  h->rlen = sizeof h->sbuf.sr.structured_reply;
>>> -  h->rlen -= sizeof h->sbuf.simple_reply;
>>> +  h->rbuf = (char *)h->rbuf + sizeof h->sbuf.reply.hdr.simple;
>>> +  h->rlen = sizeof h->sbuf.reply.hdr.structured;
>>> +  h->rlen -= sizeof h->sbuf.reply.hdr.simple;
>>>    SET_NEXT_STATE (%RECV_REMAINING);
>>>    return 0;
>>>
>>
>> Here I disagree with the mechanical approach.
>>
>> I even take issue with the pre-patch code. Pre-patch, we fill in
>> "h->sbuf.simple_reply" (in "generator/states-reply.c"), i.e., one member
>> of the top-level "sbuf" union, but then continue filling a member of a
>> *different* member (i.e., "sr.structured_reply") of the "sbuf" union
>> here. This looks undefined by the C standard, but even if it's not
>> undefined, it's supremely confusing.
> 
> It happens to work, but I agree with you that we are probably
> stretching aliasing rules about memory effective types that it is
> shaky ground to begin with.  Even adding a STATIC_ASSERT that
> offsetof(struct simple_reply, handle) == offsetof(struct
> structured_reply, handle) would be helpful to show that we depend on
> that.

After I sent my comments last week, I kept pondering this topic. I now
believe my approach to unions in this instance was too rigid. The code
is not trivial to read for sure, but there are code comments that help
with that. If you can introduce that STATIC_ASSERT, IMO that will
suffice, for sticking with this patch.

(

Given that we already depend on (non-standard) packing, and depend on
the accesses to those packed fields not to fault, it's mostly irrelevant
how exactly we calculate the byte offsets.

It's really difficult to use unions effectively, if one doesn't go
beyond what the standard guarantees. :/

)

Thanks,
Laszlo

git-format-series
Description: Text document