|
From: | Eric Blake |
Subject: | Re: [Qemu-devel] [PATCH] nbd: Don't take address of fields in packed structs |
Date: | Thu, 27 Sep 2018 13:16:33 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 9/27/18 1:03 PM, Peter Maydell wrote:
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).
Aha - I'm not the only one that does half-baked things like that. It's fun until you get caught. :)
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.
Okay, I am also in favor of the complete conversion. Want me to squash in the remaining 3 spots as part of queuing my patch, so you don't have to send a v2?
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...
Indeed, I suspected we might be headed towards removal of the *_to_*s variants, even on non-packed struct, because the syntactic sugar it provides has to be balanced against its chance for misuse.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Prev in Thread] | Current Thread | [Next in Thread] |