On 27 September 2018 at 18:30, Eric Blake <ebl...@redhat.com> 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