Re: [Libguestfs] [libnbd PATCH] internal: Tweak use of attribute packed in union sbuf

On 5/31/23 04:10, Eric Blake wrote: > While analyzing 'union sbuf' in preparation to add more members to the > union, I noticed several things related to __attribute__((packed)) > that can be improved. It helps to note that that the bulk of the > members of 'union sbuf' are already marked packed structures from > nbd-protocol.h, where we don't need to repeat that annotation in > internal.h but where it does factor into sbuf alignment. > > First,... > > Second,... > > Finally, there are benefits to naturally aligning uint64_t members,... In my opinion, this should have been three patches: (1) replace "__attribute__ ((packed)" with NBD_ATTRIBUTE_PACKED, (2) eliminate the packing for "sr" and "or", (3) introduce the "align_" fields.

This structuring is actually perfectly reflected by the commit message (compare "first", "second", "finally"). All these concepts are mutually independent, so squashing them together makes for a needlessly complicated commit message and patch body. I make an honest effort to internalize every detail of the commit message before starting to read the code, and lumping together unrelated concepts does not help with that -- I get overloaded for no good reason. That said: Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;