Re: [Qemu-devel] [PATCH] nbd: Don't take address of fields in packed structs

[Peter Maydell]

On 27 September 2018 at 18:30, Eric Blake <address@hidden> wrote:
> On 9/27/18 11:42 AM, Peter Maydell wrote:
>>
>> Taking the address of a field in a packed struct is a bad idea, because
>> it might not be actually aligned enough for that pointer type (and
>> thus cause a crash on dereference on some host architectures). Newer
>> versions of clang warn about this. Avoid the bug by not using the
>> "modify in place" byte swapping functions.
>>
>> This patch was produced with the following spatch script:
>> @@
>> expression E;
>> @@
>> -be16_to_cpus(&E);
>> +E = be16_to_cpu(E);
>
>
> I'm a bit confused. After applying your patch (and rebasing it to my pending
> pull request), I still found instances of be16_to_cpus() and others.  Were
> you only flipping instances that were members of a packed struct, while
> leaving other instances unchanged (in which case the commit message should
> be amended to mention post-filtering on the Coccinelle results)?  Can the
> Coccinelle script be tightened to only catch expressions of the form a.b or
> a->b, or where we guarantee a packed struct was involved?

I produced the patch by applying the coccinelle script to
nbd/client.c and nbd/server.c, and didn't make any by-hand changes.
So it will have changed all occurrences of these functions.
The thing that's causing what you list below is that
I started off with a script that didn't have the stanza for
the 16-bit case, and found that made server.c compile but
not client.c. So I added the extra entries but only reran the
script on client.c, not server.c (an error on my part).
As you note since they're not packed-struct fields we don't need
to change them, but I think we're probably better off doing the
complete conversion.

My rationale for preferring full conversion is that I suspect
that once we've fixed all the uses of the in-place byteswap
functions that are doing it on packed struct fields we'll
find there are very few uses left; and at that point it
might well be less confusing to have only one set of byteswap
functions rather than two. Also it will make the commit message's
claim about how the patch was produced actually correct...

thanks
-- PMM