On Wed, May 31, 2023 at 12:45:16PM +0200, Laszlo Ersek wrote: > 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.
Concur, although I swapped (2) and (1) to minimize churn. > > 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 <ler...@redhat.com> I've split the patch then added your R-b (the end result to code is the same, and I think the commit messages are better); now in as commits c1df4df9..03de4514 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
