Re: [Qemu-devel] [PATCH 11/21] qdev: move bus properties to abstract superclasses

[Anthony Liguori <aliguori@us.ibm.com>]

On 05/02/2012 06:31 AM, Paolo Bonzini wrote:
> In qdev, each bus in practice identified an abstract superclass, but
> this was mostly hidden.  In QOM, instead, these abstract classes are
> explicit so we can move bus properties there.
>
> All bus property walks are removed, and all device property walks
> are changed to look along the class hierarchy instead.
>
> This breaks global bus properties, an obscure feature when used
> with the command-line which is actually useful and used when used by
> backwards-compatible machine types.  So this patch also adjust the
> global bus properties in hw/pc_piix.c to refer to the abstract class.
>
> Globals and other properties must be modified in the same patch to
> avoid complications related to initialization ordering.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/i2c.c                      |    2 +-
>   hw/ide/qdev.c                 |    2 +-
>   hw/intel-hda.c                |    2 +-
>   hw/pc_piix.c                  |    5 +++--
>   hw/pci.c                      |    2 +-
>   hw/qdev-monitor.c             |   41 ++++++++++++++++++-----------------------
>   hw/qdev-properties.c          |   40 ++++++++++++++++++++++------------------
>   hw/qdev.c                     |   36 ++++++++++--------------------------
>   hw/qdev.h                     |    5 -----
>   hw/scsi-bus.c                 |    2 +-
>   hw/spapr_vio.c                |    2 +-
>   hw/usb/bus.c                  |    2 +-
>   hw/usb/dev-smartcard-reader.c |    2 +-
>   hw/virtio-serial-bus.c        |    2 +-
>   14 files changed, 62 insertions(+), 83 deletions(-)
>
> diff --git a/hw/i2c.c b/hw/i2c.c
> index cb10b1d..af5979e 100644
> --- a/hw/i2c.c
> +++ b/hw/i2c.c
> @@ -25,7 +25,6 @@ static Property i2c_props[] = {
>   static struct BusInfo i2c_bus_info = {
>       .name = "I2C",
>       .size = sizeof(i2c_bus),
> -    .props = i2c_props,
>   };
>
>   static void i2c_bus_pre_save(void *opaque)
> @@ -221,6 +220,7 @@ static void i2c_slave_class_init(ObjectClass *klass, void *data)
>       DeviceClass *k = DEVICE_CLASS(klass);
>       k->init = i2c_slave_qdev_init;
>       k->bus_info =&i2c_bus_info;
> +    k->props = i2c_props;
>   }
>
>   static TypeInfo i2c_slave_type_info = {
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index b67df3d..a91e878 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -36,7 +36,6 @@ static struct BusInfo ide_bus_info = {
>       .name  = "IDE",
>       .size  = sizeof(IDEBus),
>       .get_fw_dev_path = idebus_get_fw_dev_path,
> -    .props = ide_props,
>   };
>
>   void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id)
> @@ -251,6 +250,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data)
>       DeviceClass *k = DEVICE_CLASS(klass);
>       k->init = ide_qdev_init;
>       k->bus_info =&ide_bus_info;
> +    k->props = ide_props;
>   }
>
>   static TypeInfo ide_device_type_info = {
> diff --git a/hw/intel-hda.c b/hw/intel-hda.c
> index 0994f6b..e2bd41e 100644
> --- a/hw/intel-hda.c
> +++ b/hw/intel-hda.c
> @@ -37,7 +37,6 @@ static Property hda_props[] = {
>   static struct BusInfo hda_codec_bus_info = {
>       .name      = "HDA",
>       .size      = sizeof(HDACodecBus),
> -    .props     = hda_props,
>   };
>
>   void hda_codec_bus_init(DeviceState *dev, HDACodecBus *bus,
> @@ -1278,6 +1277,7 @@ static void hda_codec_device_class_init(ObjectClass *klass, void *data)
>       k->init = hda_codec_dev_init;
>       k->exit = hda_codec_dev_exit;
>       k->bus_info =&hda_codec_bus_info;
> +    k->props = hda_props;
>   }
>
>   static TypeInfo hda_codec_device_type_info = {
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 6a75718..ef6afb1 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -29,6 +29,7 @@
>   #include "apic.h"
>   #include "pci.h"
>   #include "pci_ids.h"
> +#include "usb.h"
>   #include "net.h"
>   #include "boards.h"
>   #include "ide.h"
> @@ -378,7 +379,7 @@ static QEMUMachine pc_machine_v1_1 = {
>               .property = "vapic",\
>               .value    = "off",\
>           },{\
> -            .driver   = "USB",\
> +            .driver   = TYPE_USB_DEVICE,\
>               .property = "full-path",\
>               .value    = "no",\
>           }
> @@ -451,7 +452,7 @@ static QEMUMachine pc_machine_v0_14 = {
>   #define PC_COMPAT_0_13 \
>           PC_COMPAT_0_14,\
>           {\
> -            .driver   = "PCI",\
> +            .driver   = TYPE_PCI_DEVICE,\
>               .property = "command_serr_enable",\
>               .value    = "off",\
>           },{\
> diff --git a/hw/pci.c b/hw/pci.c
> index 403651f..6319f4d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -62,7 +62,6 @@ struct BusInfo pci_bus_info = {
>       .get_dev_path = pcibus_get_dev_path,
>       .get_fw_dev_path = pcibus_get_fw_dev_path,
>       .reset      = pcibus_reset,
> -    .props      = pci_props,
>   };
>
>   static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
> @@ -2004,6 +2003,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
>       k->unplug = pci_unplug_device;
>       k->exit = pci_unregister_device;
>       k->bus_info =&pci_bus_info;
> +    k->props = pci_props;
>   }
>
>   static TypeInfo pci_device_type_info = {
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index eed781d..d9c6adc 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -123,7 +123,6 @@ int qdev_device_help(QemuOpts *opts)
>       const char *driver;
>       Property *prop;
>       ObjectClass *klass;
> -    DeviceClass *info;
>
>       driver = qemu_opt_get(opts, "driver");
>       if (driver&&  !strcmp(driver, "?")) {
> @@ -149,30 +148,22 @@ int qdev_device_help(QemuOpts *opts)
>       if (!klass) {
>           return 0;
>       }
> -    info = DEVICE_CLASS(klass);
> -
> -    for (prop = info->props; prop&&  prop->name; prop++) {
> -        /*
> -         * TODO Properties without a parser are just for dirty hacks.
> -         * qdev_prop_ptr is the only such PropertyInfo.  It's marked
> -         * for removal.  This conditional should be removed along with
> -         * it.
> -         */
> -        if (!prop->info->set) {
> -            continue;           /* no way to set it, don't show */
> -        }
> -        error_printf("%s.%s=%s\n", driver, prop->name,
> -                     prop->info->legacy_name ?: prop->info->name);
> -    }
> -    if (info->bus_info) {
> -        for (prop = info->bus_info->props; prop&&  prop->name; prop++) {
> +    do {
> +        for (prop = DEVICE_CLASS(klass)->props; prop&&  prop->name; prop++) {
> +            /*
> +             * TODO Properties without a parser are just for dirty hacks.
> +             * qdev_prop_ptr is the only such PropertyInfo.  It's marked
> +             * for removal.  This conditional should be removed along with
> +             * it.
> +             */
>               if (!prop->info->set) {
>                   continue;           /* no way to set it, don't show */
>               }
>               error_printf("%s.%s=%s\n", driver, prop->name,
>                            prop->info->legacy_name ?: prop->info->name);
>           }
> -    }
> +        klass = object_class_get_parent(klass);
> +    } while (klass != object_class_by_name(TYPE_DEVICE));
>       return 1;
>   }
>
> @@ -482,7 +473,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>   static void qbus_print(Monitor *mon, BusState *bus, int indent);
>
>   static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
> -                             const char *prefix, int indent)
> +                             int indent)
>   {
>       if (!props)
>           return;
> @@ -501,7 +492,7 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>               error_free(err);
>               continue;
>           }
> -        qdev_printf("%s-prop: %s = %s\n", prefix, props->name,
> +        qdev_printf("%s = %s\n", props->name,
>                       value&&  *value ? value : "<null>");
>           g_free(value);
>       }
> @@ -509,6 +500,7 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>
>   static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>   {
> +    ObjectClass *class;
>       BusState *child;
>       qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
>                   dev->id ? dev->id : "");
> @@ -519,8 +511,11 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>       if (dev->num_gpio_out) {
>           qdev_printf("gpio-out %d\n", dev->num_gpio_out);
>       }
> -    qdev_print_props(mon, dev, qdev_get_props(dev), "dev", indent);
> -    qdev_print_props(mon, dev, dev->parent_bus->info->props, "bus", indent);
> +    class = object_get_class(OBJECT(dev));
> +    do {
> +        qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
> +        class = object_class_get_parent(class);
> +    } while (class != object_class_by_name(TYPE_DEVICE));
>       if (dev->parent_bus->info->print_dev)
>           dev->parent_bus->info->print_dev(mon, dev, indent);
>       QLIST_FOREACH(child,&dev->child_bus, sibling) {
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 98dd06a..572b83c 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -939,17 +939,18 @@ static Property *qdev_prop_walk(Property *props, const char *name)
>
>   static Property *qdev_prop_find(DeviceState *dev, const char *name)
>   {
> +    ObjectClass *class;
>       Property *prop;
>
>       /* device properties */
> -    prop = qdev_prop_walk(qdev_get_props(dev), name);
> -    if (prop)
> -        return prop;
> -
> -    /* bus properties */
> -    prop = qdev_prop_walk(dev->parent_bus->info->props, name);
> -    if (prop)
> -        return prop;
> +    class = object_get_class(OBJECT(dev));
> +    do {
> +        prop = qdev_prop_walk(DEVICE_CLASS(class)->props, name);
> +        if (prop) {
> +            return prop;
> +        }
> +        class = object_class_get_parent(class);
> +    } while (class != object_class_by_name(TYPE_DEVICE));
>
>       return NULL;
>   }
> @@ -1169,17 +1170,20 @@ void qdev_prop_register_global_list(GlobalProperty *props)
>
>   void qdev_prop_set_globals(DeviceState *dev)
>   {
> -    GlobalProperty *prop;
> -
> -    QTAILQ_FOREACH(prop,&global_props, next) {
> -        if (strcmp(object_get_typename(OBJECT(dev)), prop->driver) != 0&&
> -            strcmp(qdev_get_bus_info(dev)->name, prop->driver) != 0) {
> -            continue;
> -        }
> -        if (qdev_prop_parse(dev, prop->property, prop->value) != 0) {
> -            exit(1);
> +    ObjectClass *class = object_get_class(OBJECT(dev));
> +
> +    do {
> +        GlobalProperty *prop;
> +        QTAILQ_FOREACH(prop,&global_props, next) {
> +            if (strcmp(object_class_get_name(class), prop->driver) != 0) {
> +                continue;
> +            }
> +            if (qdev_prop_parse(dev, prop->property, prop->value) != 0) {
> +                exit(1);
> +            }
>           }
> -    }
> +        class = object_class_get_parent(class);
> +    } while (class);
>   }
>
>   static int qdev_add_one_global(QemuOpts *opts, void *opaque)
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 67d7770..98efc8b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -45,18 +45,6 @@ const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>       return dc->vmsd;
>   }
>
> -BusInfo *qdev_get_bus_info(DeviceState *dev)
> -{
> -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -    return dc->bus_info;
> -}
> -
> -Property *qdev_get_props(DeviceState *dev)
> -{
> -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -    return dc->props;
> -}
> -
>   const char *qdev_fw_name(DeviceState *dev)
>   {
>       DeviceClass *dc = DEVICE_GET_CLASS(dev);
> @@ -78,20 +66,12 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>
>   void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>   {
> -    Property *prop;
> -
>       if (qdev_hotplug) {
>           assert(bus->allow_hotplug);
>       }
>
>       dev->parent_bus = bus;
>       QTAILQ_INSERT_HEAD(&bus->children, dev, sibling);
> -
> -    for (prop = qdev_get_bus_info(dev)->props; prop&&  prop->name; prop++) {
> -        qdev_property_add_legacy(dev, prop, NULL);
> -        qdev_property_add_static(dev, prop, NULL);
> -    }
> -    qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
>   }
>
>   /* Create a new device.  This only initializes the device state structure
> @@ -615,6 +595,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
>   static void device_initfn(Object *obj)
>   {
>       DeviceState *dev = DEVICE(obj);
> +    ObjectClass *class;
>       Property *prop;
>
>       if (qdev_hotplug) {
> @@ -625,12 +606,15 @@ static void device_initfn(Object *obj)
>       dev->instance_id_alias = -1;
>       dev->state = DEV_STATE_CREATED;
>
> -    for (prop = qdev_get_props(dev); prop&&  prop->name; prop++) {
> -        qdev_property_add_legacy(dev, prop, NULL);
> -        qdev_property_add_static(dev, prop, NULL);
> -    }
> -
> -    qdev_prop_set_defaults(dev, qdev_get_props(dev));
> +    class = object_get_class(OBJECT(dev));
> +    do {
> +        for (prop = DEVICE_CLASS(class)->props; prop&&  prop->name; prop++) {
> +            qdev_property_add_legacy(dev, prop, NULL);
> +            qdev_property_add_static(dev, prop, NULL);
> +        }
> +        qdev_prop_set_defaults(dev, DEVICE_CLASS(class)->props);
> +        class = object_class_get_parent(class);
> +    } while (class != object_class_by_name(TYPE_DEVICE));
>   }


This little bit of magic is a bit too magical for my taste.

Polymorphism relies on the idea that a subclass overloads base class 
members/methods.  From the base classes perspective, it's unaware if a subclass 
has overloaded something (that's allowed to be overloaded).

This code doesn't get the current class *as* the base class but rather gets the 
non-overloaded form of the base class.  This isn't really something that most OO 
systems allow and I think it could lead to major ugliness in the future.

I much prefer moving property installation to a function call that is invoked 
during base class init.

Regards,

Anthony Liguori