Re: [Qemu-devel] [PATCH for-1.2 v5 14/14] pci: Tidy up PCI host bridges

[Anthony Liguori <anthony@codemonkey.ws>]

"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);