Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges

[Michael S. Tsirkin]

On Wed, Jul 04, 2012 at 04:38:03PM -0500, Anthony Liguori wrote:
> On 07/04/2012 04:26 PM, Michael S. Tsirkin wrote:
> >On Thu, Jul 05, 2012 at 12:17:17AM +0300, Michael S. Tsirkin wrote:
> >>On Wed, Jul 04, 2012 at 07:19:33PM +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<address@hidden>
> >>>---
> >>>  hw/alpha_typhoon.c |    4 ++--
> >>>  hw/dec_pci.c       |    2 +-
> >>>  hw/grackle_pci.c   |    2 +-
> >>>  hw/gt64xxx.c       |   26 ++++++++++++++++----------
> >>>  hw/piix_pci.c      |    6 ++++--
> >>>  hw/ppc4xx_pci.c    |    8 +++++---
> >>>  hw/ppce500_pci.c   |    2 +-
> >>>  hw/prep_pci.c      |    8 +++++---
> >>>  hw/spapr_pci.c     |   12 +++++++-----
> >>>  hw/spapr_pci.h     |    2 +-
> >>>  hw/unin_pci.c      |   14 +++++++-------
> >>>  11 files changed, 50 insertions(+), 36 deletions(-)
> >>>
> >>>diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
> >>>index 58025a3..955d628 100644
> >>>--- a/hw/alpha_typhoon.c
> >>>+++ b/hw/alpha_typhoon.c
> >>>@@ -46,7 +46,7 @@ typedef struct TyphoonPchip {
> >>>      OBJECT_CHECK(TyphoonState, (obj), TYPE_TYPHOON_PCI_HOST_BRIDGE)
> >>>
> >>>  typedef struct TyphoonState {
> >>>-    PCIHostState host;
> >>>+    PCIHostState parent_obj;
> >>>
> >>>      TyphoonCchip cchip;
> >>>      TyphoonPchip pchip;
> >>>@@ -770,7 +770,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus 
> >>>**isa_bus,
> >>>      b = pci_register_bus(dev, "pci",
> >>>                           typhoon_set_irq, sys_map_irq, s,
> >>>                           &s->pchip.reg_mem, addr_space_io, 0, 64);
> >>>-    s->host.bus = b;
> >>>+    PCI_HOST_BRIDGE(s)->bus = b;
> >>>
> >>>      /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB.  
> >>> */
> >>>      memory_region_init_io(&s->pchip.reg_iack,&alpha_pci_iack_ops, b,
> >>
> >>Sorry I don't understand.
> >>why are we making code ugly apparently intentionally?
> >
> >Just to clarify: replacing upcasts which are always safe
> >with downcasts which can fail is what I consider especially ugly.
> 
> I'm not a huge fan of using the cast operation like this.  I'd much prefer:
> 
> PCIHostState *pci_host = PCI_HOST_BRIDGE(s);
> 
> pci_host->bus = b;
> 
> But using the macro is absolutely the right thing to do.
> 
> Macro casts never fail.  If there is a user error, then it will cause an 
> abort().

Field accesses are better. If there is a user error, code does not
compile. They are also self-documenting to some level: you look
at a struct you see all its fields. How do you know which casts
will succeed on a given type? There's no easy way.

> Using a macro has a lot of advantages as demonstrated by this patch.
> It makes refactoring a hell of a lot easier.  If you look at my
> early QOM patches, it involved a lot of "change complex touching of
> fields" with wrapping functions/macros to have better encapsulation.
> Having to touch a bunch of files just to rename 'host' to
> 'parent_obj' is ugly.
> 
> Regards,
> 
> Anthony Liguori

It still seems wrong to wrap all field accesses in macros just in case
we need to rename them later. We can pay that cost when the need arises.

-- 
MST