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

["Andreas Färber" <afaerber@suse.de>]

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.

Andreas

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