[v3,14/14] pci: Tidy up PCI host bridges

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>
---
 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(-)

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 <afaerber@suse.de>
> ---
>  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?

> diff --git a/hw/dec_pci.c b/hw/dec_pci.c
> index de16361..c30ade3 100644
> --- a/hw/dec_pci.c
> +++ b/hw/dec_pci.c
> @@ -43,7 +43,7 @@
>  #define DEC_21154(obj) OBJECT_CHECK(DECState, (obj), TYPE_DEC_21154)
>  
>  typedef struct DECState {
> -    PCIHostState host_state;
> +    PCIHostState parent_obj;
>  } DECState;
>  
>  static int dec_map_irq(PCIDevice *pci_dev, int irq_num)
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 066f6e1..67da307 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -41,7 +41,7 @@
>      OBJECT_CHECK(GrackleState, (obj), TYPE_GRACKLE_PCI_HOST_BRIDGE)
>  
>  typedef struct GrackleState {
> -    PCIHostState host_state;
> +    PCIHostState parent_obj;
>  
>      MemoryRegion pci_mmio;
>      MemoryRegion pci_hole;
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index 857758e..e95e664 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -235,7 +235,7 @@
>      OBJECT_CHECK(GT64120State, (obj), TYPE_GT64120_PCI_HOST_BRIDGE)
>  
>  typedef struct GT64120State {
> -    PCIHostState pci;
> +    PCIHostState parent_obj;
>  
>      uint32_t regs[GT_REGS];
>      PCI_MAPPING_ENTRY(PCI0IO);
> @@ -315,6 +315,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>                              uint64_t val, unsigned size)
>  {
>      GT64120State *s = opaque;
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>      uint32_t saddr;
>  
>      if (!(s->regs[GT_CPU] & 0x00001000))
> @@ -535,13 +536,15 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>          /* not implemented */
>          break;
>      case GT_PCI0_CFGADDR:
> -        s->pci.config_reg = val & 0x80fffffc;
> +        phb->config_reg = val & 0x80fffffc;
>          break;
>      case GT_PCI0_CFGDATA:
> -        if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & 0x00fff800))
> +        if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
>              val = bswap32(val);
> -        if (s->pci.config_reg & (1u << 31))
> -            pci_data_write(s->pci.bus, s->pci.config_reg, val, 4);
> +        }
> +        if (phb->config_reg & (1u << 31)) {
> +            pci_data_write(phb->bus, phb->config_reg, val, 4);
> +        }
>          break;
>  
>      /* Interrupts */
> @@ -594,6 +597,7 @@ static uint64_t gt64120_readl (void *opaque,
>                                 target_phys_addr_t addr, unsigned size)
>  {
>      GT64120State *s = opaque;
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>      uint32_t val;
>      uint32_t saddr;
>  
> @@ -775,15 +779,17 @@ static uint64_t gt64120_readl (void *opaque,
>  
>      /* PCI Internal */
>      case GT_PCI0_CFGADDR:
> -        val = s->pci.config_reg;
> +        val = phb->config_reg;
>          break;
>      case GT_PCI0_CFGDATA:
> -        if (!(s->pci.config_reg & (1 << 31)))
> +        if (!(phb->config_reg & (1 << 31))) {
>              val = 0xffffffff;
> -        else
> -            val = pci_data_read(s->pci.bus, s->pci.config_reg, 4);
> -        if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & 0x00fff800))
> +        } else {
> +            val = pci_data_read(phb->bus, phb->config_reg, 4);
> +        }
> +        if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
>              val = bswap32(val);
> +        }
>          break;
>  
>      case GT_PCI0_CMD:
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 0523d81..9522a27 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -36,7 +36,9 @@
>   * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>   */
>  
> -typedef PCIHostState I440FXState;
> +typedef struct I440FXState {
> +    PCIHostState parent_obj;
> +} I440FXState;
>  
>  #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
>  #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
> @@ -273,7 +275,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
>      dev = qdev_create(NULL, "i440FX-pcihost");
>      s = PCI_HOST_BRIDGE(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);
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 5583321..a14fd42 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -52,7 +52,7 @@ struct PCITargetMap {
>  #define PPC4xx_PCI_NR_PTMS 2
>  
>  struct PPC4xxPCIState {
> -    PCIHostState pci_state;
> +    PCIHostState parent_obj;
>  
>      struct PCIMasterMap pmm[PPC4xx_PCI_NR_PMMS];
>      struct PCITargetMap ptm[PPC4xx_PCI_NR_PTMS];
> @@ -96,16 +96,18 @@ static uint64_t pci4xx_cfgaddr_read(void *opaque, target_phys_addr_t addr,
>                                      unsigned size)
>  {
>      PPC4xxPCIState *ppc4xx_pci = opaque;
> +    PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci);
>  
> -    return ppc4xx_pci->pci_state.config_reg;
> +    return phb->config_reg;
>  }
>  
>  static void pci4xx_cfgaddr_write(void *opaque, target_phys_addr_t addr,
>                                    uint64_t value, unsigned size)
>  {
>      PPC4xxPCIState *ppc4xx_pci = opaque;
> +    PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci);
>  
> -    ppc4xx_pci->pci_state.config_reg = value & ~0x3;
> +    phb->config_reg = value & ~0x3;
>  }
>  
>  static const MemoryRegionOps pci4xx_cfgaddr_ops = {
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 3333967..92b1dc0 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -78,7 +78,7 @@ struct pci_inbound {
>      OBJECT_CHECK(PPCE500PCIState, (obj), TYPE_PPC_E500_PCI_HOST_BRIDGE)
>  
>  struct PPCE500PCIState {
> -    PCIHostState pci_state;
> +    PCIHostState parent_obj;
>  
>      struct pci_outbound pob[PPCE500_PCI_NR_POBS];
>      struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 35cb9b2..cc44e61 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -34,7 +34,7 @@
>      OBJECT_CHECK(PREPPCIState, (obj), TYPE_RAVEN_PCI_HOST_BRIDGE)
>  
>  typedef struct PRePPCIState {
> -    PCIHostState host_state;
> +    PCIHostState parent_obj;
>  
>      MemoryRegion intack;
>      qemu_irq irq[4];
> @@ -60,14 +60,16 @@ static void ppc_pci_io_write(void *opaque, target_phys_addr_t addr,
>                               uint64_t val, unsigned int size)
>  {
>      PREPPCIState *s = opaque;
> -    pci_data_write(s->host_state.bus, PPC_PCIIO_config(addr), val, size);
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    pci_data_write(phb->bus, PPC_PCIIO_config(addr), val, size);
>  }
>  
>  static uint64_t ppc_pci_io_read(void *opaque, target_phys_addr_t addr,
>                                  unsigned int size)
>  {
>      PREPPCIState *s = opaque;
> -    return pci_data_read(s->host_state.bus, PPC_PCIIO_config(addr), size);
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    return pci_data_read(phb->bus, PPC_PCIIO_config(addr), size);
>  }
>  
>  static const MemoryRegionOps PPC_PCIIO_ops = {
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 7d84801..9231e0e 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -36,16 +36,18 @@ static PCIDevice *find_dev(sPAPREnvironment *spapr,
>                             uint64_t buid, uint32_t config_addr)
>  {
>      int devfn = (config_addr >> 8) & 0xFF;
> -    sPAPRPHBState *phb;
> +    sPAPRPHBState *sphb;
>  
> -    QLIST_FOREACH(phb, &spapr->phbs, list) {
> +    QLIST_FOREACH(sphb, &spapr->phbs, list) {
> +        PCIHostState *phb;
>          BusChild *kid;
>  
> -        if (phb->buid != buid) {
> +        if (sphb->buid != buid) {
>              continue;
>          }
>  
> -        QTAILQ_FOREACH(kid, &phb->host_state.bus->qbus.children, sibling) {
> +        phb = PCI_HOST_BRIDGE(sphb);
> +        QTAILQ_FOREACH(kid, &BUS(phb->bus)->children, sibling) {
>              PCIDevice *dev = (PCIDevice *)kid->child;
>              if (dev->devfn == devfn) {
>                  return dev;
> @@ -319,7 +321,7 @@ static int spapr_phb_init(SysBusDevice *s)
>                             pci_spapr_set_irq, pci_spapr_map_irq, phb,
>                             &phb->memspace, &phb->iospace,
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS);
> -    phb->host_state.bus = bus;
> +    PCI_HOST_BRIDGE(phb)->bus = bus;
>  
>      liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16);
>      phb->dma = spapr_tce_new_dma_context(liobn, 0x40000000);
> diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
> index 06e2742..6840814 100644
> --- a/hw/spapr_pci.h
> +++ b/hw/spapr_pci.h
> @@ -33,7 +33,7 @@
>      OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>  
>  typedef struct sPAPRPHBState {
> -    PCIHostState host_state;
> +    PCIHostState parent_obj;
>  
>      uint64_t buid;
>      char *busname;
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 0db7c1f..d3aaa5a 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -53,7 +53,7 @@ static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
>      OBJECT_CHECK(UNINState, (obj), TYPE_U3_AGP_HOST_BRIDGE)
>  
>  typedef struct UNINState {
> -    PCIHostState host_state;
> +    PCIHostState parent_obj;
>  
>      MemoryRegion pci_mmio;
>      MemoryRegion pci_hole;
> @@ -114,22 +114,22 @@ static uint32_t unin_get_config_reg(uint32_t reg, uint32_t addr)
>  static void unin_data_write(void *opaque, target_phys_addr_t addr,
>                              uint64_t val, unsigned len)
>  {
> -    UNINState *s = opaque;
> +    PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
>      UNIN_DPRINTF("write addr %" TARGET_FMT_plx " len %d val %"PRIx64"\n",
>                   addr, len, val);
> -    pci_data_write(s->host_state.bus,
> -                   unin_get_config_reg(s->host_state.config_reg, addr),
> +    pci_data_write(phb->bus,
> +                   unin_get_config_reg(phb->config_reg, addr),
>                     val, len);
>  }
>  
>  static uint64_t unin_data_read(void *opaque, target_phys_addr_t addr,
>                                 unsigned len)
>  {
> -    UNINState *s = opaque;
> +    PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
>      uint32_t val;
>  
> -    val = pci_data_read(s->host_state.bus,
> -                        unin_get_config_reg(s->host_state.config_reg, addr),
> +    val = pci_data_read(phb->bus,
> +                        unin_get_config_reg(phb->config_reg, addr),
>                          len);
>      UNIN_DPRINTF("read addr %" TARGET_FMT_plx " len %d val %x\n",
>                   addr, len, val);
> -- 
> 1.7.7

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 <afaerber@suse.de>
> > ---
> >  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.

> > diff --git a/hw/dec_pci.c b/hw/dec_pci.c
> > index de16361..c30ade3 100644
> > --- a/hw/dec_pci.c
> > +++ b/hw/dec_pci.c
> > @@ -43,7 +43,7 @@
> >  #define DEC_21154(obj) OBJECT_CHECK(DECState, (obj), TYPE_DEC_21154)
> >  
> >  typedef struct DECState {
> > -    PCIHostState host_state;
> > +    PCIHostState parent_obj;
> >  } DECState;
> >  
> >  static int dec_map_irq(PCIDevice *pci_dev, int irq_num)
> > diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> > index 066f6e1..67da307 100644
> > --- a/hw/grackle_pci.c
> > +++ b/hw/grackle_pci.c
> > @@ -41,7 +41,7 @@
> >      OBJECT_CHECK(GrackleState, (obj), TYPE_GRACKLE_PCI_HOST_BRIDGE)
> >  
> >  typedef struct GrackleState {
> > -    PCIHostState host_state;
> > +    PCIHostState parent_obj;
> >  
> >      MemoryRegion pci_mmio;
> >      MemoryRegion pci_hole;
> > diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> > index 857758e..e95e664 100644
> > --- a/hw/gt64xxx.c
> > +++ b/hw/gt64xxx.c
> > @@ -235,7 +235,7 @@
> >      OBJECT_CHECK(GT64120State, (obj), TYPE_GT64120_PCI_HOST_BRIDGE)
> >  
> >  typedef struct GT64120State {
> > -    PCIHostState pci;
> > +    PCIHostState parent_obj;
> >  
> >      uint32_t regs[GT_REGS];
> >      PCI_MAPPING_ENTRY(PCI0IO);
> > @@ -315,6 +315,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
> >                              uint64_t val, unsigned size)
> >  {
> >      GT64120State *s = opaque;
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> >      uint32_t saddr;
> >  
> >      if (!(s->regs[GT_CPU] & 0x00001000))
> > @@ -535,13 +536,15 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
> >          /* not implemented */
> >          break;
> >      case GT_PCI0_CFGADDR:
> > -        s->pci.config_reg = val & 0x80fffffc;
> > +        phb->config_reg = val & 0x80fffffc;
> >          break;
> >      case GT_PCI0_CFGDATA:
> > -        if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & 0x00fff800))
> > +        if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
> >              val = bswap32(val);
> > -        if (s->pci.config_reg & (1u << 31))
> > -            pci_data_write(s->pci.bus, s->pci.config_reg, val, 4);
> > +        }
> > +        if (phb->config_reg & (1u << 31)) {
> > +            pci_data_write(phb->bus, phb->config_reg, val, 4);
> > +        }
> >          break;
> >  
> >      /* Interrupts */
> > @@ -594,6 +597,7 @@ static uint64_t gt64120_readl (void *opaque,
> >                                 target_phys_addr_t addr, unsigned size)
> >  {
> >      GT64120State *s = opaque;
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> >      uint32_t val;
> >      uint32_t saddr;
> >  
> > @@ -775,15 +779,17 @@ static uint64_t gt64120_readl (void *opaque,
> >  
> >      /* PCI Internal */
> >      case GT_PCI0_CFGADDR:
> > -        val = s->pci.config_reg;
> > +        val = phb->config_reg;
> >          break;
> >      case GT_PCI0_CFGDATA:
> > -        if (!(s->pci.config_reg & (1 << 31)))
> > +        if (!(phb->config_reg & (1 << 31))) {
> >              val = 0xffffffff;
> > -        else
> > -            val = pci_data_read(s->pci.bus, s->pci.config_reg, 4);
> > -        if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & 0x00fff800))
> > +        } else {
> > +            val = pci_data_read(phb->bus, phb->config_reg, 4);
> > +        }
> > +        if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) {
> >              val = bswap32(val);
> > +        }
> >          break;
> >  
> >      case GT_PCI0_CMD:
> > diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> > index 0523d81..9522a27 100644
> > --- a/hw/piix_pci.c
> > +++ b/hw/piix_pci.c
> > @@ -36,7 +36,9 @@
> >   * http://download.intel.com/design/chipsets/datashts/29054901.pdf
> >   */
> >  
> > -typedef PCIHostState I440FXState;
> > +typedef struct I440FXState {
> > +    PCIHostState parent_obj;
> > +} I440FXState;
> >  
> >  #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> >  #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
> > @@ -273,7 +275,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
> >      dev = qdev_create(NULL, "i440FX-pcihost");
> >      s = PCI_HOST_BRIDGE(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);
> > diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> > index 5583321..a14fd42 100644
> > --- a/hw/ppc4xx_pci.c
> > +++ b/hw/ppc4xx_pci.c
> > @@ -52,7 +52,7 @@ struct PCITargetMap {
> >  #define PPC4xx_PCI_NR_PTMS 2
> >  
> >  struct PPC4xxPCIState {
> > -    PCIHostState pci_state;
> > +    PCIHostState parent_obj;
> >  
> >      struct PCIMasterMap pmm[PPC4xx_PCI_NR_PMMS];
> >      struct PCITargetMap ptm[PPC4xx_PCI_NR_PTMS];
> > @@ -96,16 +96,18 @@ static uint64_t pci4xx_cfgaddr_read(void *opaque, target_phys_addr_t addr,
> >                                      unsigned size)
> >  {
> >      PPC4xxPCIState *ppc4xx_pci = opaque;
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci);
> >  
> > -    return ppc4xx_pci->pci_state.config_reg;
> > +    return phb->config_reg;
> >  }
> >  
> >  static void pci4xx_cfgaddr_write(void *opaque, target_phys_addr_t addr,
> >                                    uint64_t value, unsigned size)
> >  {
> >      PPC4xxPCIState *ppc4xx_pci = opaque;
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci);
> >  
> > -    ppc4xx_pci->pci_state.config_reg = value & ~0x3;
> > +    phb->config_reg = value & ~0x3;
> >  }
> >  
> >  static const MemoryRegionOps pci4xx_cfgaddr_ops = {
> > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> > index 3333967..92b1dc0 100644
> > --- a/hw/ppce500_pci.c
> > +++ b/hw/ppce500_pci.c
> > @@ -78,7 +78,7 @@ struct pci_inbound {
> >      OBJECT_CHECK(PPCE500PCIState, (obj), TYPE_PPC_E500_PCI_HOST_BRIDGE)
> >  
> >  struct PPCE500PCIState {
> > -    PCIHostState pci_state;
> > +    PCIHostState parent_obj;
> >  
> >      struct pci_outbound pob[PPCE500_PCI_NR_POBS];
> >      struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> > index 35cb9b2..cc44e61 100644
> > --- a/hw/prep_pci.c
> > +++ b/hw/prep_pci.c
> > @@ -34,7 +34,7 @@
> >      OBJECT_CHECK(PREPPCIState, (obj), TYPE_RAVEN_PCI_HOST_BRIDGE)
> >  
> >  typedef struct PRePPCIState {
> > -    PCIHostState host_state;
> > +    PCIHostState parent_obj;
> >  
> >      MemoryRegion intack;
> >      qemu_irq irq[4];
> > @@ -60,14 +60,16 @@ static void ppc_pci_io_write(void *opaque, target_phys_addr_t addr,
> >                               uint64_t val, unsigned int size)
> >  {
> >      PREPPCIState *s = opaque;
> > -    pci_data_write(s->host_state.bus, PPC_PCIIO_config(addr), val, size);
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> > +    pci_data_write(phb->bus, PPC_PCIIO_config(addr), val, size);
> >  }
> >  
> >  static uint64_t ppc_pci_io_read(void *opaque, target_phys_addr_t addr,
> >                                  unsigned int size)
> >  {
> >      PREPPCIState *s = opaque;
> > -    return pci_data_read(s->host_state.bus, PPC_PCIIO_config(addr), size);
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> > +    return pci_data_read(phb->bus, PPC_PCIIO_config(addr), size);
> >  }
> >  
> >  static const MemoryRegionOps PPC_PCIIO_ops = {
> > diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> > index 7d84801..9231e0e 100644
> > --- a/hw/spapr_pci.c
> > +++ b/hw/spapr_pci.c
> > @@ -36,16 +36,18 @@ static PCIDevice *find_dev(sPAPREnvironment *spapr,
> >                             uint64_t buid, uint32_t config_addr)
> >  {
> >      int devfn = (config_addr >> 8) & 0xFF;
> > -    sPAPRPHBState *phb;
> > +    sPAPRPHBState *sphb;
> >  
> > -    QLIST_FOREACH(phb, &spapr->phbs, list) {
> > +    QLIST_FOREACH(sphb, &spapr->phbs, list) {
> > +        PCIHostState *phb;
> >          BusChild *kid;
> >  
> > -        if (phb->buid != buid) {
> > +        if (sphb->buid != buid) {
> >              continue;
> >          }
> >  
> > -        QTAILQ_FOREACH(kid, &phb->host_state.bus->qbus.children, sibling) {
> > +        phb = PCI_HOST_BRIDGE(sphb);
> > +        QTAILQ_FOREACH(kid, &BUS(phb->bus)->children, sibling) {
> >              PCIDevice *dev = (PCIDevice *)kid->child;
> >              if (dev->devfn == devfn) {
> >                  return dev;
> > @@ -319,7 +321,7 @@ static int spapr_phb_init(SysBusDevice *s)
> >                             pci_spapr_set_irq, pci_spapr_map_irq, phb,
> >                             &phb->memspace, &phb->iospace,
> >                             PCI_DEVFN(0, 0), PCI_NUM_PINS);
> > -    phb->host_state.bus = bus;
> > +    PCI_HOST_BRIDGE(phb)->bus = bus;
> >  
> >      liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16);
> >      phb->dma = spapr_tce_new_dma_context(liobn, 0x40000000);
> > diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
> > index 06e2742..6840814 100644
> > --- a/hw/spapr_pci.h
> > +++ b/hw/spapr_pci.h
> > @@ -33,7 +33,7 @@
> >      OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
> >  
> >  typedef struct sPAPRPHBState {
> > -    PCIHostState host_state;
> > +    PCIHostState parent_obj;
> >  
> >      uint64_t buid;
> >      char *busname;
> > diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> > index 0db7c1f..d3aaa5a 100644
> > --- a/hw/unin_pci.c
> > +++ b/hw/unin_pci.c
> > @@ -53,7 +53,7 @@ static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
> >      OBJECT_CHECK(UNINState, (obj), TYPE_U3_AGP_HOST_BRIDGE)
> >  
> >  typedef struct UNINState {
> > -    PCIHostState host_state;
> > +    PCIHostState parent_obj;
> >  
> >      MemoryRegion pci_mmio;
> >      MemoryRegion pci_hole;
> > @@ -114,22 +114,22 @@ static uint32_t unin_get_config_reg(uint32_t reg, uint32_t addr)
> >  static void unin_data_write(void *opaque, target_phys_addr_t addr,
> >                              uint64_t val, unsigned len)
> >  {
> > -    UNINState *s = opaque;
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
> >      UNIN_DPRINTF("write addr %" TARGET_FMT_plx " len %d val %"PRIx64"\n",
> >                   addr, len, val);
> > -    pci_data_write(s->host_state.bus,
> > -                   unin_get_config_reg(s->host_state.config_reg, addr),
> > +    pci_data_write(phb->bus,
> > +                   unin_get_config_reg(phb->config_reg, addr),
> >                     val, len);
> >  }
> >  
> >  static uint64_t unin_data_read(void *opaque, target_phys_addr_t addr,
> >                                 unsigned len)
> >  {
> > -    UNINState *s = opaque;
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
> >      uint32_t val;
> >  
> > -    val = pci_data_read(s->host_state.bus,
> > -                        unin_get_config_reg(s->host_state.config_reg, addr),
> > +    val = pci_data_read(phb->bus,
> > +                        unin_get_config_reg(phb->config_reg, addr),
> >                          len);
> >      UNIN_DPRINTF("read addr %" TARGET_FMT_plx " len %d val %x\n",
> >                   addr, len, val);
> > -- 
> > 1.7.7

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<afaerber@suse.de>
>>> ---
>>>   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().

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

>
>>> diff --git a/hw/dec_pci.c b/hw/dec_pci.c
>>> index de16361..c30ade3 100644
>>> --- a/hw/dec_pci.c
>>> +++ b/hw/dec_pci.c
>>> @@ -43,7 +43,7 @@
>>>   #define DEC_21154(obj) OBJECT_CHECK(DECState, (obj), TYPE_DEC_21154)
>>>
>>>   typedef struct DECState {
>>> -    PCIHostState host_state;
>>> +    PCIHostState parent_obj;
>>>   } DECState;
>>>
>>>   static int dec_map_irq(PCIDevice *pci_dev, int irq_num)
>>> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
>>> index 066f6e1..67da307 100644
>>> --- a/hw/grackle_pci.c
>>> +++ b/hw/grackle_pci.c
>>> @@ -41,7 +41,7 @@
>>>       OBJECT_CHECK(GrackleState, (obj), TYPE_GRACKLE_PCI_HOST_BRIDGE)
>>>
>>>   typedef struct GrackleState {
>>> -    PCIHostState host_state;
>>> +    PCIHostState parent_obj;
>>>
>>>       MemoryRegion pci_mmio;
>>>       MemoryRegion pci_hole;
>>> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
>>> index 857758e..e95e664 100644
>>> --- a/hw/gt64xxx.c
>>> +++ b/hw/gt64xxx.c
>>> @@ -235,7 +235,7 @@
>>>       OBJECT_CHECK(GT64120State, (obj), TYPE_GT64120_PCI_HOST_BRIDGE)
>>>
>>>   typedef struct GT64120State {
>>> -    PCIHostState pci;
>>> +    PCIHostState parent_obj;
>>>
>>>       uint32_t regs[GT_REGS];
>>>       PCI_MAPPING_ENTRY(PCI0IO);
>>> @@ -315,6 +315,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>>>                               uint64_t val, unsigned size)
>>>   {
>>>       GT64120State *s = opaque;
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>>>       uint32_t saddr;
>>>
>>>       if (!(s->regs[GT_CPU]&  0x00001000))
>>> @@ -535,13 +536,15 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>>>           /* not implemented */
>>>           break;
>>>       case GT_PCI0_CFGADDR:
>>> -        s->pci.config_reg = val&  0x80fffffc;
>>> +        phb->config_reg = val&  0x80fffffc;
>>>           break;
>>>       case GT_PCI0_CFGDATA:
>>> -        if (!(s->regs[GT_PCI0_CMD]&  1)&&  (s->pci.config_reg&  0x00fff800))
>>> +        if (!(s->regs[GT_PCI0_CMD]&  1)&&  (phb->config_reg&  0x00fff800)) {
>>>               val = bswap32(val);
>>> -        if (s->pci.config_reg&  (1u<<  31))
>>> -            pci_data_write(s->pci.bus, s->pci.config_reg, val, 4);
>>> +        }
>>> +        if (phb->config_reg&  (1u<<  31)) {
>>> +            pci_data_write(phb->bus, phb->config_reg, val, 4);
>>> +        }
>>>           break;
>>>
>>>       /* Interrupts */
>>> @@ -594,6 +597,7 @@ static uint64_t gt64120_readl (void *opaque,
>>>                                  target_phys_addr_t addr, unsigned size)
>>>   {
>>>       GT64120State *s = opaque;
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>>>       uint32_t val;
>>>       uint32_t saddr;
>>>
>>> @@ -775,15 +779,17 @@ static uint64_t gt64120_readl (void *opaque,
>>>
>>>       /* PCI Internal */
>>>       case GT_PCI0_CFGADDR:
>>> -        val = s->pci.config_reg;
>>> +        val = phb->config_reg;
>>>           break;
>>>       case GT_PCI0_CFGDATA:
>>> -        if (!(s->pci.config_reg&  (1<<  31)))
>>> +        if (!(phb->config_reg&  (1<<  31))) {
>>>               val = 0xffffffff;
>>> -        else
>>> -            val = pci_data_read(s->pci.bus, s->pci.config_reg, 4);
>>> -        if (!(s->regs[GT_PCI0_CMD]&  1)&&  (s->pci.config_reg&  0x00fff800))
>>> +        } else {
>>> +            val = pci_data_read(phb->bus, phb->config_reg, 4);
>>> +        }
>>> +        if (!(s->regs[GT_PCI0_CMD]&  1)&&  (phb->config_reg&  0x00fff800)) {
>>>               val = bswap32(val);
>>> +        }
>>>           break;
>>>
>>>       case GT_PCI0_CMD:
>>> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
>>> index 0523d81..9522a27 100644
>>> --- a/hw/piix_pci.c
>>> +++ b/hw/piix_pci.c
>>> @@ -36,7 +36,9 @@
>>>    * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>>>    */
>>>
>>> -typedef PCIHostState I440FXState;
>>> +typedef struct I440FXState {
>>> +    PCIHostState parent_obj;
>>> +} I440FXState;
>>>
>>>   #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
>>>   #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
>>> @@ -273,7 +275,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
>>>       dev = qdev_create(NULL, "i440FX-pcihost");
>>>       s = PCI_HOST_BRIDGE(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);
>>> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
>>> index 5583321..a14fd42 100644
>>> --- a/hw/ppc4xx_pci.c
>>> +++ b/hw/ppc4xx_pci.c
>>> @@ -52,7 +52,7 @@ struct PCITargetMap {
>>>   #define PPC4xx_PCI_NR_PTMS 2
>>>
>>>   struct PPC4xxPCIState {
>>> -    PCIHostState pci_state;
>>> +    PCIHostState parent_obj;
>>>
>>>       struct PCIMasterMap pmm[PPC4xx_PCI_NR_PMMS];
>>>       struct PCITargetMap ptm[PPC4xx_PCI_NR_PTMS];
>>> @@ -96,16 +96,18 @@ static uint64_t pci4xx_cfgaddr_read(void *opaque, target_phys_addr_t addr,
>>>                                       unsigned size)
>>>   {
>>>       PPC4xxPCIState *ppc4xx_pci = opaque;
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci);
>>>
>>> -    return ppc4xx_pci->pci_state.config_reg;
>>> +    return phb->config_reg;
>>>   }
>>>
>>>   static void pci4xx_cfgaddr_write(void *opaque, target_phys_addr_t addr,
>>>                                     uint64_t value, unsigned size)
>>>   {
>>>       PPC4xxPCIState *ppc4xx_pci = opaque;
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci);
>>>
>>> -    ppc4xx_pci->pci_state.config_reg = value&  ~0x3;
>>> +    phb->config_reg = value&  ~0x3;
>>>   }
>>>
>>>   static const MemoryRegionOps pci4xx_cfgaddr_ops = {
>>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>>> index 3333967..92b1dc0 100644
>>> --- a/hw/ppce500_pci.c
>>> +++ b/hw/ppce500_pci.c
>>> @@ -78,7 +78,7 @@ struct pci_inbound {
>>>       OBJECT_CHECK(PPCE500PCIState, (obj), TYPE_PPC_E500_PCI_HOST_BRIDGE)
>>>
>>>   struct PPCE500PCIState {
>>> -    PCIHostState pci_state;
>>> +    PCIHostState parent_obj;
>>>
>>>       struct pci_outbound pob[PPCE500_PCI_NR_POBS];
>>>       struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
>>> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
>>> index 35cb9b2..cc44e61 100644
>>> --- a/hw/prep_pci.c
>>> +++ b/hw/prep_pci.c
>>> @@ -34,7 +34,7 @@
>>>       OBJECT_CHECK(PREPPCIState, (obj), TYPE_RAVEN_PCI_HOST_BRIDGE)
>>>
>>>   typedef struct PRePPCIState {
>>> -    PCIHostState host_state;
>>> +    PCIHostState parent_obj;
>>>
>>>       MemoryRegion intack;
>>>       qemu_irq irq[4];
>>> @@ -60,14 +60,16 @@ static void ppc_pci_io_write(void *opaque, target_phys_addr_t addr,
>>>                                uint64_t val, unsigned int size)
>>>   {
>>>       PREPPCIState *s = opaque;
>>> -    pci_data_write(s->host_state.bus, PPC_PCIIO_config(addr), val, size);
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>>> +    pci_data_write(phb->bus, PPC_PCIIO_config(addr), val, size);
>>>   }
>>>
>>>   static uint64_t ppc_pci_io_read(void *opaque, target_phys_addr_t addr,
>>>                                   unsigned int size)
>>>   {
>>>       PREPPCIState *s = opaque;
>>> -    return pci_data_read(s->host_state.bus, PPC_PCIIO_config(addr), size);
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>>> +    return pci_data_read(phb->bus, PPC_PCIIO_config(addr), size);
>>>   }
>>>
>>>   static const MemoryRegionOps PPC_PCIIO_ops = {
>>> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
>>> index 7d84801..9231e0e 100644
>>> --- a/hw/spapr_pci.c
>>> +++ b/hw/spapr_pci.c
>>> @@ -36,16 +36,18 @@ static PCIDevice *find_dev(sPAPREnvironment *spapr,
>>>                              uint64_t buid, uint32_t config_addr)
>>>   {
>>>       int devfn = (config_addr>>  8)&  0xFF;
>>> -    sPAPRPHBState *phb;
>>> +    sPAPRPHBState *sphb;
>>>
>>> -    QLIST_FOREACH(phb,&spapr->phbs, list) {
>>> +    QLIST_FOREACH(sphb,&spapr->phbs, list) {
>>> +        PCIHostState *phb;
>>>           BusChild *kid;
>>>
>>> -        if (phb->buid != buid) {
>>> +        if (sphb->buid != buid) {
>>>               continue;
>>>           }
>>>
>>> -        QTAILQ_FOREACH(kid,&phb->host_state.bus->qbus.children, sibling) {
>>> +        phb = PCI_HOST_BRIDGE(sphb);
>>> +        QTAILQ_FOREACH(kid,&BUS(phb->bus)->children, sibling) {
>>>               PCIDevice *dev = (PCIDevice *)kid->child;
>>>               if (dev->devfn == devfn) {
>>>                   return dev;
>>> @@ -319,7 +321,7 @@ static int spapr_phb_init(SysBusDevice *s)
>>>                              pci_spapr_set_irq, pci_spapr_map_irq, phb,
>>>                              &phb->memspace,&phb->iospace,
>>>                              PCI_DEVFN(0, 0), PCI_NUM_PINS);
>>> -    phb->host_state.bus = bus;
>>> +    PCI_HOST_BRIDGE(phb)->bus = bus;
>>>
>>>       liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus)<<  16);
>>>       phb->dma = spapr_tce_new_dma_context(liobn, 0x40000000);
>>> diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
>>> index 06e2742..6840814 100644
>>> --- a/hw/spapr_pci.h
>>> +++ b/hw/spapr_pci.h
>>> @@ -33,7 +33,7 @@
>>>       OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
>>>
>>>   typedef struct sPAPRPHBState {
>>> -    PCIHostState host_state;
>>> +    PCIHostState parent_obj;
>>>
>>>       uint64_t buid;
>>>       char *busname;
>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
>>> index 0db7c1f..d3aaa5a 100644
>>> --- a/hw/unin_pci.c
>>> +++ b/hw/unin_pci.c
>>> @@ -53,7 +53,7 @@ static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
>>>       OBJECT_CHECK(UNINState, (obj), TYPE_U3_AGP_HOST_BRIDGE)
>>>
>>>   typedef struct UNINState {
>>> -    PCIHostState host_state;
>>> +    PCIHostState parent_obj;
>>>
>>>       MemoryRegion pci_mmio;
>>>       MemoryRegion pci_hole;
>>> @@ -114,22 +114,22 @@ static uint32_t unin_get_config_reg(uint32_t reg, uint32_t addr)
>>>   static void unin_data_write(void *opaque, target_phys_addr_t addr,
>>>                               uint64_t val, unsigned len)
>>>   {
>>> -    UNINState *s = opaque;
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
>>>       UNIN_DPRINTF("write addr %" TARGET_FMT_plx " len %d val %"PRIx64"\n",
>>>                    addr, len, val);
>>> -    pci_data_write(s->host_state.bus,
>>> -                   unin_get_config_reg(s->host_state.config_reg, addr),
>>> +    pci_data_write(phb->bus,
>>> +                   unin_get_config_reg(phb->config_reg, addr),
>>>                      val, len);
>>>   }
>>>
>>>   static uint64_t unin_data_read(void *opaque, target_phys_addr_t addr,
>>>                                  unsigned len)
>>>   {
>>> -    UNINState *s = opaque;
>>> +    PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
>>>       uint32_t val;
>>>
>>> -    val = pci_data_read(s->host_state.bus,
>>> -                        unin_get_config_reg(s->host_state.config_reg, addr),
>>> +    val = pci_data_read(phb->bus,
>>> +                        unin_get_config_reg(phb->config_reg, addr),
>>>                           len);
>>>       UNIN_DPRINTF("read addr %" TARGET_FMT_plx " len %d val %x\n",
>>>                    addr, len, val);
>>> --
>>> 1.7.7

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<afaerber@suse.de>
> >>>---
> >>>  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.

Il 05/07/2012 10:59, Michael S. Tsirkin ha scritto:
> 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.
> 

I agree.

Paolo

Il 05/07/2012 00:34, Andreas Färber ha scritto:
>> > Just to clarify: replacing upcasts which are always safe
>> > with downcasts which can fail is what I consider especially ugly.
> As per Anthony the parent field in the QOM instance structs is not
> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
> that it doesn't even show up in gtk-doc documentation. If it is unused,
> its name becomes irrelevant and could even be "reserved" if we so
> wanted. Renaming it to whatever proves that all old references are gone.

I disagree with removing static checks whenever possible.

> Background is that qdev and QOM work differently with regards to
> inheritance: as mentioned in the preceding patch, for qdev the parent
> was (had to be) identified by name and could be anywhere in the struct;

Not entirely true, being at the beginning of the struct is already
enforced by using DO_UPCAST (which is admittedly a strange name for a
downcast macro) instead of container_of.

> for QOM the parent is a subset of the struct from the start and it's
> supposed to be accessed through the struct type that provides the
> fields, the usual way to get such a pointer is through
> OBJECT_CHECK()-derived cast macros.

Paolo

Am 05.07.2012 11:53, schrieb Paolo Bonzini:
> Il 05/07/2012 00:34, Andreas Färber ha scritto:
>>>> Just to clarify: replacing upcasts which are always safe
>>>> with downcasts which can fail is what I consider especially ugly.
>> As per Anthony the parent field in the QOM instance structs is not
>> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
>> that it doesn't even show up in gtk-doc documentation. If it is unused,
>> its name becomes irrelevant and could even be "reserved" if we so
>> wanted. Renaming it to whatever proves that all old references are gone.
> 
> I disagree with removing static checks whenever possible.
> 
>> Background is that qdev and QOM work differently with regards to
>> inheritance: as mentioned in the preceding patch, for qdev the parent
>> was (had to be) identified by name and could be anywhere in the struct;
> 
> Not entirely true, being at the beginning of the struct is already
> enforced by using DO_UPCAST (which is admittedly a strange name for a
> downcast macro) instead of container_of.

If you look at the patchset you will find that it was not properly enforced!

Andreas

> > Not entirely true, being at the beginning of the struct is already
> > enforced by using DO_UPCAST (which is admittedly a strange name for
> > a downcast macro) instead of container_of.
> 
> If you look at the patchset you will find that it was not properly
> enforced!

Well, what *is* enforced? :)

Paolo

On Thu, Jul 05, 2012 at 12:15:04PM +0200, Andreas Färber wrote:
> Am 05.07.2012 11:53, schrieb Paolo Bonzini:
> > Il 05/07/2012 00:34, Andreas Färber ha scritto:
> >>>> Just to clarify: replacing upcasts which are always safe
> >>>> with downcasts which can fail is what I consider especially ugly.
> >> As per Anthony the parent field in the QOM instance structs is not
> >> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
> >> that it doesn't even show up in gtk-doc documentation. If it is unused,
> >> its name becomes irrelevant and could even be "reserved" if we so
> >> wanted. Renaming it to whatever proves that all old references are gone.
> > 
> > I disagree with removing static checks whenever possible.
> > 
> >> Background is that qdev and QOM work differently with regards to
> >> inheritance: as mentioned in the preceding patch, for qdev the parent
> >> was (had to be) identified by name and could be anywhere in the struct;
> > 
> > Not entirely true, being at the beginning of the struct is already
> > enforced by using DO_UPCAST (which is admittedly a strange name for a
> > downcast macro) instead of container_of.
> 
> If you look at the patchset you will find that it was not properly enforced!
> 
> Andreas

Yes DO_UPCAST is IMO evil and should go - everyone can eiter use dynamic
QOM types with lookup by name; or stop passing nonsafe void * pointers
around and use container_of consistently.


> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
> > 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 <afaerber@suse.de>
> >>> ---
> >>>  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.
> 
> As per Anthony the parent field in the QOM instance structs is not
> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
> that it doesn't even show up in gtk-doc documentation.

PCIHostState is not private here. And if it were, it won't be
of any use because compiler does not read gtk-doc documentation
and neither do developers.

> If it is unused,
> its name becomes irrelevant and could even be "reserved" if we so
> wanted. Renaming it to whatever proves that all old references are gone.

Adding arbitrary rules like that only seems to make code future proof.
People will not remember and will use the field anyway,
then when you try to change it there will be work to be done.
So let's do the work when we really need it.

> Background is that qdev and QOM work differently with regards to
> inheritance: as mentioned in the preceding patch, for qdev the parent
> was (had to be) identified by name and could be anywhere in the struct;
> for QOM the parent is a subset of the struct from the start and it's
> supposed to be accessed through the struct type that provides the
> fields, the usual way to get such a pointer is through
> OBJECT_CHECK()-derived cast macros.

It makes sense if you go from parent to child. Even in C++
you don't use dynamic_cast to go from child to parent.


> In the snippet above I chose to use the macro directly since there was
> no second use but I can introduce a variable if preferred.
> What I see now though is that this code snippet creating the PCIBus
> should be moved up into the SysBus init function.
> 
> I'll wait with posting a v4 til I hear what exactly to rebase it on.
> As for further renaming/splitting wishes this is what I referenced:
> http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03069.html
> 
> In particular, did you expect me to rename PCIHostState to PCIHostBridge
> / PCIHostBridgeState everywhere? pci_host.c to pci_host_bridge.c? Or did
> you just mean the new PCI_HOST_BRIDGE macros like I did in this v3?
> 
> Regards,
> Andreas

Yes just the new macros.

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Am 05.07.2012 15:34, schrieb Michael S. Tsirkin:
> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
>>> 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 <afaerber@suse.de>
>>>>> ---
>>>>>  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.
>>
>> As per Anthony the parent field in the QOM instance structs is not
>> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
>> that it doesn't even show up in gtk-doc documentation.
> 
> PCIHostState is not private here. And if it were, it won't be
> of any use because compiler does not read gtk-doc documentation
> and neither do developers.
> 
>> If it is unused,
>> its name becomes irrelevant and could even be "reserved" if we so
>> wanted. Renaming it to whatever proves that all old references are gone.
> 
> Adding arbitrary rules like that only seems to make code future proof.
> People will not remember and will use the field anyway,
> then when you try to change it there will be work to be done.
> So let's do the work when we really need it.
> 
>> Background is that qdev and QOM work differently with regards to
>> inheritance: as mentioned in the preceding patch, for qdev the parent
>> was (had to be) identified by name and could be anywhere in the struct;
>> for QOM the parent is a subset of the struct from the start and it's
>> supposed to be accessed through the struct type that provides the
>> fields, the usual way to get such a pointer is through
>> OBJECT_CHECK()-derived cast macros.
> 
> It makes sense if you go from parent to child. Even in C++
> you don't use dynamic_cast to go from child to parent.

Which is the core point: In C++ you don't need to go through some parent
field, you access the fields and methods directly through the child
variable. In C that's not possible. Sticking to the specific type that
has the fields you want to access allows you to code C++-style.

Anyway, I am not doing these changes here because I have great arguments
foo and bar for it but because this is QOM that so far no one opposed
when Anthony introduced it. Please discuss that with Anthony. My
interest is having it done consistently and not having legacy code lying
around that gets copied by new contributors.

Andreas

On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
>>> 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<afaerber@suse.de>
>>>>> ---
>>>>>   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.
>>
>> As per Anthony the parent field in the QOM instance structs is not
>> supposed to be touched (cf. object.h). We mark it /*<  private>*/ so
>> that it doesn't even show up in gtk-doc documentation.
>
> PCIHostState is not private here. And if it were, it won't be
> of any use because compiler does not read gtk-doc documentation
> and neither do developers.
>
>> If it is unused,
>> its name becomes irrelevant and could even be "reserved" if we so
>> wanted. Renaming it to whatever proves that all old references are gone.
>
> Adding arbitrary rules like that only seems to make code future proof.
> People will not remember and will use the field anyway,
> then when you try to change it there will be work to be done.
> So let's do the work when we really need it.
>
>> Background is that qdev and QOM work differently with regards to
>> inheritance: as mentioned in the preceding patch, for qdev the parent
>> was (had to be) identified by name and could be anywhere in the struct;
>> for QOM the parent is a subset of the struct from the start and it's
>> supposed to be accessed through the struct type that provides the
>> fields, the usual way to get such a pointer is through
>> OBJECT_CHECK()-derived cast macros.
>
> It makes sense if you go from parent to child. Even in C++
> you don't use dynamic_cast to go from child to parent.

But you use static cast.  Casting is not the same thing as dereferencing a 
member.  IOW:

Foo *foo = (Foo *)bar;

Is very different than:

Foo *foo = &bar.foo;

Using cast macros makes the code an awful lot more readable because it tells the 
reader more.

Foo *foo = FOO(bar);

Tells the user that we're casting bar to the a type Foo.  OTOH:

Foo *foo = &bar.foo;

Doesn't tell the user anything.  A FOO() macro is consistent regardless of how 
the type is implemented too.  It gets even worse when you are going up multiple 
levels:

DeviceState *dev = &bar.host.qdev;

Is unintelligible whereas:

DeviceState *dev = DEVICE(bar);

Tells you exactly what's happening.

But really, this isn't the place to debate this.  QOM has been around for a 
while.  Consistency is important and this is how things are done in QOM.

If you want to revisit this style, you should start a separate thread about it 
and we can talk about it.  But this patch is consistent with the current 
infrastructure.

Regards,

Anthony Liguori

>
>
>> In the snippet above I chose to use the macro directly since there was
>> no second use but I can introduce a variable if preferred.
>> What I see now though is that this code snippet creating the PCIBus
>> should be moved up into the SysBus init function.
>>
>> I'll wait with posting a v4 til I hear what exactly to rebase it on.
>> As for further renaming/splitting wishes this is what I referenced:
>> http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03069.html
>>
>> In particular, did you expect me to rename PCIHostState to PCIHostBridge
>> / PCIHostBridgeState everywhere? pci_host.c to pci_host_bridge.c? Or did
>> you just mean the new PCI_HOST_BRIDGE macros like I did in this v3?
>>
>> Regards,
>> Andreas
>
> Yes just the new macros.
>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>>

On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote:
> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
> >On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
> >>Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
> >>>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<afaerber@suse.de>
> >>>>>---
> >>>>>  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.
> >>
> >>As per Anthony the parent field in the QOM instance structs is not
> >>supposed to be touched (cf. object.h). We mark it /*<  private>*/ so
> >>that it doesn't even show up in gtk-doc documentation.
> >
> >PCIHostState is not private here. And if it were, it won't be
> >of any use because compiler does not read gtk-doc documentation
> >and neither do developers.
> >
> >>If it is unused,
> >>its name becomes irrelevant and could even be "reserved" if we so
> >>wanted. Renaming it to whatever proves that all old references are gone.
> >
> >Adding arbitrary rules like that only seems to make code future proof.
> >People will not remember and will use the field anyway,
> >then when you try to change it there will be work to be done.
> >So let's do the work when we really need it.
> >
> >>Background is that qdev and QOM work differently with regards to
> >>inheritance: as mentioned in the preceding patch, for qdev the parent
> >>was (had to be) identified by name and could be anywhere in the struct;
> >>for QOM the parent is a subset of the struct from the start and it's
> >>supposed to be accessed through the struct type that provides the
> >>fields, the usual way to get such a pointer is through
> >>OBJECT_CHECK()-derived cast macros.
> >
> >It makes sense if you go from parent to child. Even in C++
> >you don't use dynamic_cast to go from child to parent.
> 
> But you use static cast.  Casting is not the same thing as
> dereferencing a member.  IOW:
> 
> Foo *foo = (Foo *)bar;
> 
> Is very different than:
> 
> Foo *foo = &bar.foo;
> 
> Using cast macros makes the code an awful lot more readable because
> it tells the reader more.
> 
> Foo *foo = FOO(bar);
> 
> Tells the user that we're casting bar to the a type Foo.  OTOH:
> 
> Foo *foo = &bar.foo;
> 
> Doesn't tell the user anything.  A FOO() macro is consistent
> regardless of how the type is implemented too.  It gets even worse
> when you are going up multiple levels:
> 
> DeviceState *dev = &bar.host.qdev;
> 
> Is unintelligible whereas:
> 
> DeviceState *dev = DEVICE(bar);
> 
> Tells you exactly what's happening.
> 
> But really, this isn't the place to debate this.  QOM has been
> around for a while.  Consistency is important and this is how things
> are done in QOM.

It's important but not the most important thing.
It does not make sense to do casts *everywhere*.
Do it where it makes sense.

> If you want to revisit this style, you should start a separate
> thread about it and we can talk about it.  But this patch is
> consistent with the current infrastructure.
> 
> Regards,
> 
> Anthony Liguori

So far QOM was a win.
You examples look better with a macro. Patch 13 looks better:
-    PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+    PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
this is an improvement: devices do not want to deal with sysbus.

The code above that is being changed looks better without.

This is why this thread is about the specific patch and not
general QOM.

> >
> >
> >>In the snippet above I chose to use the macro directly since there was
> >>no second use but I can introduce a variable if preferred.
> >>What I see now though is that this code snippet creating the PCIBus
> >>should be moved up into the SysBus init function.
> >>
> >>I'll wait with posting a v4 til I hear what exactly to rebase it on.
> >>As for further renaming/splitting wishes this is what I referenced:
> >>http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03069.html
> >>
> >>In particular, did you expect me to rename PCIHostState to PCIHostBridge
> >>/ PCIHostBridgeState everywhere? pci_host.c to pci_host_bridge.c? Or did
> >>you just mean the new PCI_HOST_BRIDGE macros like I did in this v3?
> >>
> >>Regards,
> >>Andreas
> >
> >Yes just the new macros.
> >
> >>--
> >>SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >>GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> >>

(Dropping some borked CCs)

Am 05.07.2012 16:15, schrieb Michael S. Tsirkin:
> On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote:
>> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
>>> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
>>>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
>>>>> 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<afaerber@suse.de>
>>>>>>> ---
>>>>>>>  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.
>>>>
>>>> As per Anthony the parent field in the QOM instance structs is not
>>>> supposed to be touched (cf. object.h). We mark it /*<  private>*/ so
>>>> that it doesn't even show up in gtk-doc documentation.
>>>
>>> PCIHostState is not private here. And if it were, it won't be
>>> of any use because compiler does not read gtk-doc documentation
>>> and neither do developers.
>>>
>>>> If it is unused,
>>>> its name becomes irrelevant and could even be "reserved" if we so
>>>> wanted. Renaming it to whatever proves that all old references are gone.
>>>
>>> Adding arbitrary rules like that only seems to make code future proof.
>>> People will not remember and will use the field anyway,
>>> then when you try to change it there will be work to be done.
>>> So let's do the work when we really need it.
>>>
>>>> Background is that qdev and QOM work differently with regards to
>>>> inheritance: as mentioned in the preceding patch, for qdev the parent
>>>> was (had to be) identified by name and could be anywhere in the struct;
>>>> for QOM the parent is a subset of the struct from the start and it's
>>>> supposed to be accessed through the struct type that provides the
>>>> fields, the usual way to get such a pointer is through
>>>> OBJECT_CHECK()-derived cast macros.
>>>
>>> It makes sense if you go from parent to child. Even in C++
>>> you don't use dynamic_cast to go from child to parent.
>>
>> But you use static cast.  Casting is not the same thing as
>> dereferencing a member.  IOW:
>>
>> Foo *foo = (Foo *)bar;
>>
>> Is very different than:
>>
>> Foo *foo = &bar.foo;
>>
>> Using cast macros makes the code an awful lot more readable because
>> it tells the reader more.
>>
>> Foo *foo = FOO(bar);
>>
>> Tells the user that we're casting bar to the a type Foo.  OTOH:
>>
>> Foo *foo = &bar.foo;
>>
>> Doesn't tell the user anything.  A FOO() macro is consistent
>> regardless of how the type is implemented too.  It gets even worse
>> when you are going up multiple levels:
>>
>> DeviceState *dev = &bar.host.qdev;
>>
>> Is unintelligible whereas:
>>
>> DeviceState *dev = DEVICE(bar);
>>
>> Tells you exactly what's happening.
>>
>> But really, this isn't the place to debate this.  QOM has been
>> around for a while.  Consistency is important and this is how things
>> are done in QOM.
> 
> It's important but not the most important thing.
> It does not make sense to do casts *everywhere*.
> Do it where it makes sense.
> 
>> If you want to revisit this style, you should start a separate
>> thread about it and we can talk about it.  But this patch is
>> consistent with the current infrastructure.
>>
>> Regards,
>>
>> Anthony Liguori
> 
> So far QOM was a win.
> You examples look better with a macro. Patch 13 looks better:
> -    PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
> this is an improvement: devices do not want to deal with sysbus.
> 
> The code above that is being changed looks better without.

Do you want me to apologize for my macro misuse now? I apologize. :)
Replace PCI_HOST_BRIDGE(s)->bus = b; with
PCI_HOST_BRIDGE *phb = PCI_HOST_BRIDGE(s);
phb->bus = b;
in your mind and it matches exactly what you liked better above, no?

> This is why this thread is about the specific patch and not
> general QOM.

You're basically saying, QOM was a win but don't use it here. That's a
contradiction and thus a general QOM issue since that paradigm not only
applies here: Either we need to design all structs so that their fields
have nice names to access directly as you suggest, or nobody must access
the parent field as Anthony suggests. Having a wild mix of both at
maintainers' gusto is not a good solution.

Arguably we can just leave out this last patch if it is so
controversial. However, the split is arbitrary since I backwards-coded
this series, moving code snippets to earlier individual patches using
checkout -p, i.e. patches 1-11 have this final code design in mind and
thus went from SYS_BUS_DEVICE() to PCIHostState above rather than
through parent_obj. The reason I couldn't do it there directly is that
we didn't have the PCI_HOST_BRIDGE() macro before patch 13.
I also based patch 14 on the assumption that i440fx will get more state
fields when Anthony goes ahead with his QOM Pin series, otherwise we
could just scratch the I440FXState typedef (same discussion as for the
qtest herbivore test case).

Andreas

On Thu, Jul 05, 2012 at 05:00:00PM +0200, Andreas Färber wrote:
> (Dropping some borked CCs)
> 
> Am 05.07.2012 16:15, schrieb Michael S. Tsirkin:
> > On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote:
> >> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
> >>> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
> >>>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
> >>>>> 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<afaerber@suse.de>
> >>>>>>> ---
> >>>>>>>  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.
> >>>>
> >>>> As per Anthony the parent field in the QOM instance structs is not
> >>>> supposed to be touched (cf. object.h). We mark it /*<  private>*/ so
> >>>> that it doesn't even show up in gtk-doc documentation.
> >>>
> >>> PCIHostState is not private here. And if it were, it won't be
> >>> of any use because compiler does not read gtk-doc documentation
> >>> and neither do developers.
> >>>
> >>>> If it is unused,
> >>>> its name becomes irrelevant and could even be "reserved" if we so
> >>>> wanted. Renaming it to whatever proves that all old references are gone.
> >>>
> >>> Adding arbitrary rules like that only seems to make code future proof.
> >>> People will not remember and will use the field anyway,
> >>> then when you try to change it there will be work to be done.
> >>> So let's do the work when we really need it.
> >>>
> >>>> Background is that qdev and QOM work differently with regards to
> >>>> inheritance: as mentioned in the preceding patch, for qdev the parent
> >>>> was (had to be) identified by name and could be anywhere in the struct;
> >>>> for QOM the parent is a subset of the struct from the start and it's
> >>>> supposed to be accessed through the struct type that provides the
> >>>> fields, the usual way to get such a pointer is through
> >>>> OBJECT_CHECK()-derived cast macros.
> >>>
> >>> It makes sense if you go from parent to child. Even in C++
> >>> you don't use dynamic_cast to go from child to parent.
> >>
> >> But you use static cast.  Casting is not the same thing as
> >> dereferencing a member.  IOW:
> >>
> >> Foo *foo = (Foo *)bar;
> >>
> >> Is very different than:
> >>
> >> Foo *foo = &bar.foo;
> >>
> >> Using cast macros makes the code an awful lot more readable because
> >> it tells the reader more.
> >>
> >> Foo *foo = FOO(bar);
> >>
> >> Tells the user that we're casting bar to the a type Foo.  OTOH:
> >>
> >> Foo *foo = &bar.foo;
> >>
> >> Doesn't tell the user anything.  A FOO() macro is consistent
> >> regardless of how the type is implemented too.  It gets even worse
> >> when you are going up multiple levels:
> >>
> >> DeviceState *dev = &bar.host.qdev;
> >>
> >> Is unintelligible whereas:
> >>
> >> DeviceState *dev = DEVICE(bar);
> >>
> >> Tells you exactly what's happening.
> >>
> >> But really, this isn't the place to debate this.  QOM has been
> >> around for a while.  Consistency is important and this is how things
> >> are done in QOM.
> > 
> > It's important but not the most important thing.
> > It does not make sense to do casts *everywhere*.
> > Do it where it makes sense.
> > 
> >> If you want to revisit this style, you should start a separate
> >> thread about it and we can talk about it.  But this patch is
> >> consistent with the current infrastructure.
> >>
> >> Regards,
> >>
> >> Anthony Liguori
> > 
> > So far QOM was a win.
> > You examples look better with a macro. Patch 13 looks better:
> > -    PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
> > this is an improvement: devices do not want to deal with sysbus.
> > 
> > The code above that is being changed looks better without.
> 
> Do you want me to apologize for my macro misuse now? I apologize. :)
> Replace PCI_HOST_BRIDGE(s)->bus = b; with
> PCI_HOST_BRIDGE *phb = PCI_HOST_BRIDGE(s);
> phb->bus = b;
> in your mind and it matches exactly what you liked better above, no?

No, what I liked above is that we hide sysbus from devices.

> > This is why this thread is about the specific patch and not
> > general QOM.
> 
> You're basically saying, QOM was a win but don't use it here. That's a
> contradiction and thus a general QOM issue since that paradigm not only
> applies here: Either we need to design all structs so that their fields
> have nice names to access directly as you suggest, or nobody must access
> the parent field as Anthony suggests. Having a wild mix of both at
> maintainers' gusto is not a good solution.

With time we'll end up with a mix anyway since compiler does not enforce
no direct access.

> Arguably we can just leave out this last patch if it is so
> controversial. However, the split is arbitrary since I backwards-coded
> this series, moving code snippets to earlier individual patches using
> checkout -p, i.e. patches 1-11 have this final code design in mind and
> thus went from SYS_BUS_DEVICE() to PCIHostState above rather than
> through parent_obj. The reason I couldn't do it there directly is that
> we didn't have the PCI_HOST_BRIDGE() macro before patch 13.
> I also based patch 14 on the assumption that i440fx will get more state
> fields when Anthony goes ahead with his QOM Pin series, otherwise we
> could just scratch the I440FXState typedef (same discussion as for the
> qtest herbivore test case).
> 
> Andreas

The real problem is that devices have to tweak pcihost->bus and that
is because we don't model has-a relationship between pci host bridge and
bus well: pci host bridge has a pci bus.  As long as that remains true
it seems to me the more explicit it is the better, so it's easier to
fix.

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>