Hi Andreas,
On Wed, Jun 12, 2013 at 7:15 PM, Andreas Färber <afaer...@suse.de> wrote:
> Am 10.06.2013 04:08, schrieb Anthony Liguori:
>> Peter Crosthwaite <peter.crosthwa...@xilinx.com> writes:
>>> On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <afaer...@suse.de> wrote:
>>>> Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
>>>>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <afaer...@suse.de> wrote:
>>>>>> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
>>>>>> index dc6f4e4..409d315 100644
>>>>>> --- a/hw/9pfs/virtio-9p-device.c
>>>>>> +++ b/hw/9pfs/virtio-9p-device.c
>>>> [...]
>>>>>> @@ -136,12 +138,16 @@ static Property virtio_9p_properties[] = {
>>>>>> DEFINE_PROP_END_OF_LIST(),
>>>>>> };
>>>>>>
>>>>>> -static void virtio_9p_class_init(ObjectClass *klass, void *data)
>>>>>> +static void virtio_9p_class_init(ObjectClass *oc, void *data)
>>>>>> {
>>>>>> - DeviceClass *dc = DEVICE_CLASS(klass);
>>>>>> - VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
>>>>>> + DeviceClass *dc = DEVICE_CLASS(oc);
>>>>>> + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(oc);
>>>>>> + V9fsClass *v9c = VIRTIO_9P_CLASS(oc);
>>>>>> +
>>>>>> + v9c->parent_realize = dc->realize;
>>>>>> + dc->realize = virtio_9p_device_realize;
>>>>>> +
>>>>>> dc->props = virtio_9p_properties;
>>>>>> - vdc->init = virtio_9p_device_init;
>>>>>> vdc->get_features = virtio_9p_get_features;
>>>>>> vdc->get_config = virtio_9p_get_config;
>>>>>> }
>>>>>> @@ -151,6 +157,7 @@ static const TypeInfo virtio_device_info = {
>>>>>> .parent = TYPE_VIRTIO_DEVICE,
>>>>>> .instance_size = sizeof(V9fsState),
>>>>>> .class_init = virtio_9p_class_init,
>>>>>> + .class_size = sizeof(V9fsClass),
>>>>>> };
>>>>>>
>>>>>> static void virtio_9p_register_types(void)
>>>>>> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
>>>>>> index 1d6eedb..85699a7 100644
>>>>>> --- a/hw/9pfs/virtio-9p.h
>>>>>> +++ b/hw/9pfs/virtio-9p.h
>>>>>> @@ -227,6 +227,15 @@ typedef struct V9fsState
>>>>>> V9fsConf fsconf;
>>>>>> } V9fsState;
>>>>>>
>>>>>> +typedef struct V9fsClass {
>>>>>> + /*< private >*/
>>>>>> + VirtioDeviceClass parent_class;
>>>>>> + /*< public >*/
>>>>>> +
>>>>>> + DeviceRealize parent_realize;
>>>>>> +} V9fsClass;
>>>>>> +
>>>>>> +
>>>>>
>>>>> If applied tree-wide this change pattern is going to add a lot of
>>>>> boiler-plate to all devices. There is capability in QOM to access the
>>>>> overridden parent class functions already, so it can be made to work
>>>>> without every class having to do this save-and-call trick with
>>>>> overridden realize (and friends). How about this:
>>>>>
>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>>> index 9190a7e..696702a 100644
>>>>> --- a/hw/core/qdev.c
>>>>> +++ b/hw/core/qdev.c
>>>>> @@ -37,6 +37,18 @@ int qdev_hotplug = 0;
>>>>> static bool qdev_hot_added = false;
>>>>> static bool qdev_hot_removed = false;
>>>>>
>>>>> +void device_parent_realize(DeviceState *dev, Error **errp)
>>>>> +{
>>>>> + ObjectClass *class = object_get_class(dev);
>>>>> + DeviceClass *dc;
>>>>> +
>>>>> + class = object_class_get_parent(dc);
>>>>> + assert(class);
>>>>> + dc = DEVICE_CLASS(class);
>>>>> +
>>>>> + dc->realize(dev, errp);
>>>>> +}
>>
>> What's weird about this is that you aren't necessarily calling
>> Device::realize() here, you're really calling super()::realize().
>
> We can certainly fix that by passing ObjectClass *oc argument instead of
> DeviceState *dev and using object_class_get_parent(oc)
That is slightly more defensive but does cost you a line of code on
every usage (to do the DEVICE_GET_CLASS(dev)). Im happy either way as
its the need for the FooClass definition that really bloats the code.
Under this modified proposal its a two line change rather than the one
which is not so bad.
> - dc is used
> uninitialized above.
>
Typo on my part. should be "class"
>> I really don't know whether it's a better approach or not.
>
> It does save LOCs and should work with sane class_inits.
>
>> Another option is to have a VirtioDevice::realize() that virtio devices
>> overload such that you don't have to explicitly call the super()
>> version. The advantage of this approach is that you don't have to
>> explicitly call the super version.
>
> The disadvantage is that we then have no chance to solve Jesse's
> virtio-net issue this way (cf. cover letter), the only improvement would
> be Error propagation.
>
+1. I thought we were trying to get away from abstractions
(SYS_BUS_DEVICE, I2C_DEVICE etc, and friends) defining their own
abstract inititialization fns (init or realize) so this would be a
step backwards. It also makes the code base very inconsistent with
itself. Devices which inherit from an abstraction that already have a
realize need to use the specific realize while those that dont would
use the generic one (DeviceClass::realize). Also makes life very hard
when someone wants to add common realize functionality to an
abstraction (e.g. implemenent a non-trivial SysBusDevice::Realize) as
all the concrete classes would then need conversion.
> Just let me know which path to pursue here. We could start by converting
> *::init to realize signature and then follow up with either conversion.
> Would that be acceptable to move forward?
Sounds good to me, and makes it easier to disentangle this particular
issue with the rest of your changes.
Regards,
Peter
> Long-term a VirtioDevice::realize() would be blurring semantics though.
>
> Partially affects pending ISA series as well (PIT/PIC).
>
> 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
>
