Re: [Qemu-devel] [PATCH RFT 2/5] virtio: Convert VirtioDevice to QOM realize/unrealize

[Peter Crosthwaite]

Hi Andreas,

On Wed, Jun 12, 2013 at 7:15 PM, Andreas Färber <address@hidden> wrote:
> Am 10.06.2013 04:08, schrieb Anthony Liguori:
>> Peter Crosthwaite <address@hidden> writes:
>>> On Sat, Jun 8, 2013 at 7:55 PM, Andreas Färber <address@hidden> wrote:
>>>> Am 08.06.2013 04:22, schrieb Peter Crosthwaite:
>>>>> On Sat, Jun 8, 2013 at 4:18 AM, Andreas Färber <address@hidden> 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
>