Re: [Qemu-devel] [PATCH] virtio: make bindings typesafe

[Andreas Färber]

Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
> On Mon, Dec 17, 2012 at 11:08:43PM +0100, Andreas Färber wrote:
>> Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
>>> On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
>>>> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
>>>>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
>>>>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
>>>>>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
>>>>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
>>>>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>>>>>>> index 3ea4140..63ae888 100644
>>>>>>>>> --- a/hw/virtio-pci.c
>>>>>>>>> +++ b/hw/virtio-pci.c
>>>>>>>>> @@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);
>>>>>>>>>  
>>>>>>>>>  /* virtio device */
>>>>>>>>>  
>>>>>>>>> -static void virtio_pci_notify(void *opaque, uint16_t vector)
>>>>>>>>> +static void virtio_pci_notify(DeviceState *d, uint16_t vector)
>>>>>>>>>  {
>>>>>>>>> -    VirtIOPCIProxy *proxy = opaque;
>>>>>>>>> +    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, 
>>>>>>>>> pci_dev.qdev);
>>>>>>>>
>>>>>>>> Nack. This is going the wrong direction QOM-wise and you among all
>>>>>>>> others know that from PCI host bridges!
>>>>>>>
>>>>>>> Well, that's just a difference of VIRTIO_PCI_PROXY(d) vs. container_of.
>>>>>>
>>>>>> VIRTIO_PCI_PROXY(d) would be acceptable, sure. But as-is this patch just
>>>>>> pushes unnecessary work on Fred, me, you or anyone else who works with 
>>>>>> QOM.
>>>>>
>>>>> What's VIRTIO_PCI_PROXY? Note this is data path we do not want extra
>>>>> code.
>>>>
>>>> My complaint is the direct access of pci_dev, qdev, etc. parent fields
>>>> in many places as the main change of this patch. Those mean more places
>>>> to touch in a future patch.
>>>>
>>>> Use of any new-style macro hiding these - wherever the particular one
>>>> suggested may be defined or whether it needs to be added - is better.
>>>>
>>>> If performance of dynamic_cast is an issue - something I'd leave you to
>>>> discuss with Anthony - you can just do a C cast directly. Just don't
>>>> spread this qdev paradigm further please.
>>>
>>> OK so just
>>>
>>> #define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_dev.qdev)
>>>
>>> is OK with you?
>>
>> Well, at least it's better than inlining it...
>>
>> I would've expected to see VIRTIO_PCI_PROXY(obj) defined as
>> OBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something) somewhere.
>>
>> If, as you imply with "data path", this were a problem, you could just
>> do VirtIOPCIProxy *proxy = (VirtIOPCIProxy *)d inline to allow for
>> VIRTIO_PCI_PROXY() to be used in the QOM sense elsewhere.
> 
> I don't get it - where?
> Since we don't do runtime checks we need container_of -
> safer than a plain cast.
>
> Anyway, when you start doing your QOM conversions it will be
> easy to do what you like.

I don't get what you don't get - QOM has been around for nearly a year
now and it's neither my invention nor my conversion, it's ours! It's an
object-orientation framework for C to avoid a flag day for rewriting
everything in a new language, while allowing us to get a number of new
features.

In object-oriented languages like C++, C# or Java there is no concept of
accessing parent state by name with or without container_of(); that is a
QOM implementation detail as long as we don't generate the parent's
fields in the actual struct via some QIDL'ish preprocessing or by
switching to a language that supports it. Does this explain better?

Every maintainer should review patches to conform to QOM, just like I
review for Coding Style issues even if I don't personally agree to every
bit of what became the consensus. That has exactly nothing to do with
me, I may be many things but not QOM maintainer. Having Anthony or any
single maintainer follow-up everyone's devices with fixes for QOM simply
doesn't scale. And when I do refactorings to get something particular
accomplished, I can't do any major benchmarking of each device, so I
need an easy way to tell if someone was merely sloppy or utterly clever
about the choice of macros.

Late rebellions against QOM are just as tiring as the constant Coding
Style wars in the audio code. If you have an idea to improve QOM cast
performance (maybe disable some checks for non-debug builds?) that would
benefit everyone rather than just having PCI/virtio be different from
the rest of the code base.

Having said that, yes, container_of() assures that VirtIOPCIProxy has a
PCIDevice field somewhere, but when you pass (Device *)sth_not_a_device
in the caller, that check becomes totally moot. Having a typed argument
is not moot, of course (I believe Fred was proposing the more specific
VirtioDevice* for that). But since there are only two callers passing in
DeviceState* to the bind function today (maybe four soon) I fail to see
the benefit of container_of() you are trying to sell me here...

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg