From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42046) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SPYiA-0002h0-Rl for qemu-devel@nongnu.org; Wed, 02 May 2012 08:31:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SPYi0-00011X-UL for qemu-devel@nongnu.org; Wed, 02 May 2012 08:31:14 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:40123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SPYi0-00011I-Jy for qemu-devel@nongnu.org; Wed, 02 May 2012 08:31:04 -0400 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 2 May 2012 06:30:58 -0600 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 8BE8B19D8050 for ; Wed, 2 May 2012 06:29:48 -0600 (MDT) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q42CTjeP214388 for ; Wed, 2 May 2012 06:29:48 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q42CULcD014528 for ; Wed, 2 May 2012 06:30:22 -0600 Message-ID: <4FA128B7.8090402@us.ibm.com> Date: Wed, 02 May 2012 07:29:43 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1335958273-769-1-git-send-email-pbonzini@redhat.com> <1335958273-769-12-git-send-email-pbonzini@redhat.com> In-Reply-To: <1335958273-769-12-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/21] qdev: move bus properties to abstract superclasses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, afaerber@suse.de, liwp@linux.vnet.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 > --- > 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 : ""); > 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