On 5 October 2018 at 13:17, Laszlo Ersek <ler...@redhat.com> wrote: > On 10/05/18 12:49, Philippe Mathieu-Daudé wrote: >> Is the comment added in 8e4a424b305 still valid? >> >> /* >> * A helper function for the _utterly broken_ virtio device model to >> find out if >> * it's running on a big endian machine. Don't do this at home kids! >> */ >> bool target_words_bigendian(void); > > Yeah, that comment irks me too. > > Not that it isn't valid, necessarily. "hw/virtio/virtio.c" still > features an embedded declaration of this function. Just above > virtio_default_endian(), which calls it. And the latter function is > called in several places (mostly from commit 616a655219a92). > > So I can't really tell what's "utterly broken" here -- the device model > (i.e., the virtio implementation in QEMU), or the legacy *spec* that > required QEMU to jump through such hoops?
AIUI, the brokenness here is that the virtio specification requires that a device knows the endianness of the CPU, which is an awkward layering violation [*]. The point of the comment is really to say "virtio needs this, but that is a special weird case, think twice or three times before using this function anywhere else" (which I suspect is also why the declaration is not in a header file, to make it less easily accessible). [*] Actually it's not even the endianness of the CPU, because bi-endian CPUs like Arm can be in big-endian mode even though TARGET_WORDS_BIGENDIAN is false... I entirely agree that we should clean up the comment to be clearer about what it's trying to say. > BTW, I don't think angry comments are useful in *code*. They look & feel > great for about a week. In a year, they just look unprofessional. (NB, > this is not about "political correctness"; it's just that, when someone > reads such a comment, they receive the anger *before* they understand, > or recall, the underlying problem. Agreed 100%. thanks -- PMM
