| Message ID | 20120813131659.GE16801@redhat.com |
|---|---|
| State | New |
| Headers | show |
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Thu, Aug 02, 2012 at 03:47:06AM +0200, Andreas Färber wrote: >> Uglify the parent field to enforce QOM-style access via casts. >> Don't just typedef PCIHostState, either use it directly or embed it. >> >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > > IMHO only one chunk from this patch should be applied (below). > Below it is split out but needs to be rebased on top of patches > 1-13. I understand what your objection is but it's unreasonable IMHO. The purpose of QOM is to bring consistency across large swaths of code in QEMU that have historically done things there own way. This means expressing concepts like inheritence and casting in the same way across the board. The common way (the QOM way) is to make the parent type the first member of the struct (typically named parent or parent_obj) and then to use cast macros to upcast and downcast. This patch is 100% correct in that regard and I'm going to apply it once Andreas makes the change I requested. For my part, I'm long over due in writing up a device authoring style guide that I promised a few weeks ago. I'll write that up this afternoon and send it out today. We can debate the merits of this sort of thing in the style guide. Regards, Anthony Liguori > > --> > > From: Andreas Färber <andreas.faerber@web.de> > > piix: minor code simplification > > There's no need to deal with qdev internals in piix - we get device > state from qdev_create so just use that. > > Signed-off-by: Andreas Färber <andreas.faerber@web.de> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index c497a01..18554a6 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -274,7 +274,7 @@ static PCIBus *i440fx_common_init(const char *device_name, > dev = qdev_create(NULL, "i440FX-pcihost"); > s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev)); > s->address_space = address_space_mem; > - b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space, > + b = pci_bus_new(&dev, NULL, pci_address_space, > address_space_io, 0); > s->bus = b; > object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
On 13/08/12 15:16, Anthony Liguori wrote: > I understand what your objection is but it's unreasonable IMHO. The > purpose of QOM is to bring consistency across large swaths of code in > QEMU that have historically done things there own way. > > This means expressing concepts like inheritence and casting in the same > way across the board. The common way (the QOM way) is to make the > parent type the first member of the struct (typically named parent or > parent_obj) and then to use cast macros to upcast and downcast. > > This patch is 100% correct in that regard and I'm going to apply it once > Andreas makes the change I requested. > > For my part, I'm long over due in writing up a device authoring style > guide that I promised a few weeks ago. I'll write that up this > afternoon and send it out today. > > We can debate the merits of this sort of thing in the style guide. Yes please! I've spent quite a fair bit of time over the past couple of weeks in this area, and have found the lack of documentation related to trying to create a new hardware device fairly frustrating. The hardest part is not the coding, but figuring which of the following I should be using out of: qom qdev qmp hmp sysbus VMState (I think that half of these are now obsolete interfaces, but a general document which explains how these things are related and which ones I should be using would be very welcome) ATB, Mark.
diff --git a/hw/piix_pci.c b/hw/piix_pci.c index c497a01..18554a6 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -274,7 +274,7 @@ static PCIBus *i440fx_common_init(const char *device_name, dev = qdev_create(NULL, "i440FX-pcihost"); s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev)); s->address_space = address_space_mem; - b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space, + b = pci_bus_new(&dev, NULL, pci_address_space, address_space_io, 0); s->bus = b; object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);