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

[Laszlo Ersek]

On 5/31/23 17:48, Eric Blake wrote:
> 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
> 

Thanks!