Re: [Qemu-ppc] [Qemu-devel] [PATCH] spapr_pci: rename some structured types

[Greg Kurz]

On Fri, 12 Oct 2018 11:17:32 +1100
David Gibson <address@hidden> wrote:

> On Thu, Oct 11, 2018 at 03:04:08PM +0200, Greg Kurz wrote:
> > On Thu, 11 Oct 2018 13:02:50 +0200
> > Philippe Mathieu-Daudé <address@hidden> wrote:
> >   
> > > Hi Greg,
> > > 
> > > On 11/10/2018 09:00, Greg Kurz wrote:  
> > > > According to CODING_STYLE, structured types names are expected to be
> > > > in CamelCase. PCI is dropped from the name for better  
> > > readability.  
> 
> I'd prefer you didn't drop PCI from the name.  These specifically
> *are* PCI MSIs, whereas lots of PAPR documentation (particularly for
> the interrupt controller) uses MSIs to indicate general message
> interrupts, such as those from VIO and firmware event reporting.
> 

So, if we follow the CamelCase style used in the spapr code, this would give
sPAPRPCIMSI and sPAPRPCIMSIMig. I personally find that it's uneasy to read:
so many upper case letters in a row kinda defeat the purpose of CamelCase.

Looking at the rest of the QEMU code base, I see that there is no definitive
rule for acronyms. They're sometimes all upper-case (eg, AIOCBInfo) or converted
to CamelCase (eg, LinuxAioState)... but I couldn't find any other example where
the type name only contains acronyms.

Maybe we need to twist the rule some more to handle this case.

I'll give a try with sPAPRpciMSI and sPAPRpciMSImig.

> > > > 
> > > > While here, this also converts a call to g_malloc(n * sizeofi(foo)) to  
> > > >   
> > >   
> > 
> > Oops finger slipped on its way to the left parenthesis :)
> >   
> > > sizeof(foo)
> > >   
> > > > g_new(n, foo), which is a recommended good practice.    
> > > 
> > > Clearly 2 different patches...
> > >   
> > 
> > You're right in theory. I added the g_new conversion in the same patch
> > for simplicity mostly. I'll let David decide :)  
> 
> I'd prefer them separate.
> 

Ok.

> >   
> > > > 
> > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > ---
> > > >  hw/ppc/spapr_pci.c          |   22 +++++++++++-----------
> > > >  include/hw/pci-host/spapr.h |   12 ++++++------
> > > >  2 files changed, 17 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > > index c2271e6ed462..bfb959f2b065 100644
> > > > --- a/hw/ppc/spapr_pci.c
> > > > +++ b/hw/ppc/spapr_pci.c
> > > > @@ -277,7 +277,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, 
> > > > sPAPRMachineState *spapr,
> > > >      unsigned int irq, max_irqs = 0;
> > > >      sPAPRPHBState *phb = NULL;
> > > >      PCIDevice *pdev = NULL;
> > > > -    spapr_pci_msi *msi;
> > > > +    sPAPRMSI *msi;    
> > > 
> > > In CamelCase this should be "SpaprMsi"...
> > > I however agree sPAPRMSI is nicer, the code is also easier to
> > > review.  
> 
> Yeah.. it's kind of a mangled combination of CamelCase with what's
> been done elsewhere in the spapr code.
> 
> > >   
> > > >      int *config_addr_key;
> > > >      Error *err = NULL;
> > > >      int i;
> > > > @@ -325,7 +325,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, 
> > > > sPAPRMachineState *spapr,
> > > >          return;
> > > >      }
> > > >  
> > > > -    msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, 
> > > > &config_addr);
> > > > +    msi = (sPAPRMSI *) g_hash_table_lookup(phb->msi, &config_addr);
> > > >  
> > > >      /* Releasing MSIs */
> > > >      if (!req_num) {
> > > > @@ -414,7 +414,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, 
> > > > sPAPRMachineState *spapr,
> > > >                       irq, req_num);
> > > >  
> > > >      /* Add MSI device to cache */
> > > > -    msi = g_new(spapr_pci_msi, 1);
> > > > +    msi = g_new(sPAPRMSI, 1);
> > > >      msi->first_irq = irq;
> > > >      msi->num = req_num;
> > > >      config_addr_key = g_new(int, 1);
> > > > @@ -445,7 +445,7 @@ static void 
> > > > rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
> > > >      unsigned int intr_src_num = -1, ioa_intr_num = rtas_ld(args, 3);
> > > >      sPAPRPHBState *phb = NULL;
> > > >      PCIDevice *pdev = NULL;
> > > > -    spapr_pci_msi *msi;
> > > > +    sPAPRMSI *msi;
> > > >  
> > > >      /* Find sPAPRPHBState */
> > > >      phb = spapr_pci_find_phb(spapr, buid);
> > > > @@ -458,7 +458,7 @@ static void 
> > > > rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
> > > >      }
> > > >  
> > > >      /* Find device descriptor and start IRQ */
> > > > -    msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, 
> > > > &config_addr);
> > > > +    msi = (sPAPRMSI *) g_hash_table_lookup(phb->msi, &config_addr);
> > > >      if (!msi || !msi->first_irq || !msi->num || (ioa_intr_num >= 
> > > > msi->num)) {
> > > >          trace_spapr_pci_msi("Failed to return vector", config_addr);
> > > >          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > > @@ -1849,9 +1849,9 @@ static const VMStateDescription 
> > > > vmstate_spapr_pci_msi = {
> > > >      .version_id = 1,
> > > >      .minimum_version_id = 1,
> > > >      .fields = (VMStateField []) {
> > > > -        VMSTATE_UINT32(key, spapr_pci_msi_mig),
> > > > -        VMSTATE_UINT32(value.first_irq, spapr_pci_msi_mig),
> > > > -        VMSTATE_UINT32(value.num, spapr_pci_msi_mig),
> > > > +        VMSTATE_UINT32(key, sPAPRMSIMig),
> > > > +        VMSTATE_UINT32(value.first_irq, sPAPRMSIMig),
> > > > +        VMSTATE_UINT32(value.num, sPAPRMSIMig),
> > > >          VMSTATE_END_OF_LIST()
> > > >      },
> > > >  };
> > > > @@ -1883,12 +1883,12 @@ static int spapr_pci_pre_save(void *opaque)
> > > >      if (!sphb->msi_devs_num) {
> > > >          return 0;
> > > >      }
> > > > -    sphb->msi_devs = g_malloc(sphb->msi_devs_num * 
> > > > sizeof(spapr_pci_msi_mig));
> > > > +    sphb->msi_devs = g_new(sPAPRMSIMig, sphb->msi_devs_num);
> > > >  
> > > >      g_hash_table_iter_init(&iter, sphb->msi);
> > > >      for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
> > > >          sphb->msi_devs[i].key = *(uint32_t *) key;
> > > > -        sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
> > > > +        sphb->msi_devs[i].value = *(sPAPRMSI *) value;
> > > >      }
> > > >  
> > > >      return 0;
> > > > @@ -1938,7 +1938,7 @@ static const VMStateDescription vmstate_spapr_pci 
> > > > = {
> > > >                               vmstate_spapr_pci_lsi, struct 
> > > > spapr_pci_lsi),
> > > >          VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> > > >          VMSTATE_STRUCT_VARRAY_ALLOC(msi_devs, sPAPRPHBState, 
> > > > msi_devs_num, 0,
> > > > -                                    vmstate_spapr_pci_msi, 
> > > > spapr_pci_msi_mig),
> > > > +                                    vmstate_spapr_pci_msi, 
> > > > sPAPRMSIMig),
> > > >          VMSTATE_END_OF_LIST()
> > > >      },
> > > >  };
> > > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > > > index 7c66c3872f96..041126da573d 100644
> > > > --- a/include/hw/pci-host/spapr.h
> > > > +++ b/include/hw/pci-host/spapr.h
> > > > @@ -34,15 +34,15 @@
> > > >  
> > > >  typedef struct sPAPRPHBState sPAPRPHBState;
> > > >  
> > > > -typedef struct spapr_pci_msi {
> > > > +typedef struct sPAPRMSI {
> > > >      uint32_t first_irq;
> > > >      uint32_t num;
> > > > -} spapr_pci_msi;
> > > > +} sPAPRMSI;
> > > >  
> > > > -typedef struct spapr_pci_msi_mig {
> > > > +typedef struct sPAPRMSIMig {
> > > >      uint32_t key;
> > > > -    spapr_pci_msi value;
> > > > -} spapr_pci_msi_mig;
> > > > +    sPAPRMSI value;
> > > > +} sPAPRMSIMig;
> > > >  
> > > >  struct sPAPRPHBState {
> > > >      PCIHostState parent_obj;
> > > > @@ -70,7 +70,7 @@ struct sPAPRPHBState {
> > > >      GHashTable *msi;
> > > >      /* Temporary cache for migration purposes */
> > > >      int32_t msi_devs_num;
> > > > -    spapr_pci_msi_mig *msi_devs;
> > > > +    sPAPRMSIMig *msi_devs;
> > > >  
> > > >      QLIST_ENTRY(sPAPRPHBState) list;
> > > >  
> > > > 
> > > >     
> >   
>

pgp8BFlPyeM_A.pgp
Description: OpenPGP digital signature