[Libguestfs] [libnbd PATCH] internal: Tweak use of attribute packed in union sbuf
Eric Blake
eblake at redhat.com
Wed May 31 15:48:08 UTC 2023
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 <lersek at 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
More information about the Libguestfs
mailing list