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

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.

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