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
