[Qemu-devel] [PATCH v2 0/2] spapr_pci: coding style fixes

[Qemu-devel] [PATCH v2 0/2] spapr_pci: coding style fixes

[Greg Kurz]

As requested by David, two distinct patches and PCI still mentionned
in the new type names.

--
Greg

---

Greg Kurz (2):
      spapr_pci: convert g_malloc() to g_new()
      spapr_pci: rename some structured types


 hw/ppc/spapr_pci.c          |   22 +++++++++++-----------
 include/hw/pci-host/spapr.h |   12 ++++++------
 2 files changed, 17 insertions(+), 17 deletions(-)

[Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new()

[Greg Kurz]

When allocating an array, it is a recommended coding practice to call
g_new(FooType, n) instead of g_malloc(n * sizeof(FooType)) because
it takes care to avoid overflow when calculating the size of the
allocated block and it returns FooType *, which allows the compiler
to perform type checking.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index c2271e6ed462..0537ce018f51 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1883,7 +1883,7 @@ 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(spapr_pci_msi_mig, sphb->msi_devs_num);
 
     g_hash_table_iter_init(&iter, sphb->msi);
     for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {

Re: [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new()

[Philippe Mathieu-Daudé]

On 12/10/2018 11:05, Greg Kurz wrote:
> When allocating an array, it is a recommended coding practice to call
> g_new(FooType, n) instead of g_malloc(n * sizeof(FooType)) because
> it takes care to avoid overflow when calculating the size of the
> allocated block and it returns FooType *, which allows the compiler
> to perform type checking.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/ppc/spapr_pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c2271e6ed462..0537ce018f51 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1883,7 +1883,7 @@ 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(spapr_pci_msi_mig, sphb->msi_devs_num);
>  
>      g_hash_table_iter_init(&iter, sphb->msi);
>      for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
> 
> 

Re: [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On Fri, Oct 12, 2018 at 11:05:09AM +0200, Greg Kurz wrote:
> When allocating an array, it is a recommended coding practice to call
> g_new(FooType, n) instead of g_malloc(n * sizeof(FooType)) because
> it takes care to avoid overflow when calculating the size of the
> allocated block and it returns FooType *, which allows the compiler
> to perform type checking.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.1, thanks.

> ---
>  hw/ppc/spapr_pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c2271e6ed462..0537ce018f51 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1883,7 +1883,7 @@ 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(spapr_pci_msi_mig, sphb->msi_devs_num);
>  
>      g_hash_table_iter_init(&iter, sphb->msi);
>      for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types

[Greg Kurz]

According to CODING_STYLE, structured types names are expected to be
in CamelCase but we have:

typedef struct spapr_pci_msi {
    uint32_t first_irq;
    uint32_t num;
} spapr_pci_msi;

typedef struct spapr_pci_msi_mig {
    uint32_t key;
    spapr_pci_msi value;
} spapr_pci_msi_mig;

Acronyms are often written in upper-case, but here we would en up with
a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
purpose of CamelCase. It even displays "RPC" which looks awkward.

This patch twists the rule a bit to keep the type names readable:
sPAPRpciMSI and sPAPRpciMSImig.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - moved g_malloc change to a separate patch
    - new naming proposal that doesn't drop PCI
    - more detailed description in the changelog
---
 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 0537ce018f51..2ee933e2d1ec 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;
+    sPAPRpciMSI *msi;
     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 = (sPAPRpciMSI *) 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(sPAPRpciMSI, 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;
+    sPAPRpciMSI *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 = (sPAPRpciMSI *) 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, sPAPRpciMSImig),
+        VMSTATE_UINT32(value.first_irq, sPAPRpciMSImig),
+        VMSTATE_UINT32(value.num, sPAPRpciMSImig),
         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_new(spapr_pci_msi_mig, sphb->msi_devs_num);
+    sphb->msi_devs = g_new(sPAPRpciMSImig, 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 = *(sPAPRpciMSI *) 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, sPAPRpciMSImig),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 7c66c3872f96..eb8436b4fc32 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 sPAPRpciMSI {
     uint32_t first_irq;
     uint32_t num;
-} spapr_pci_msi;
+} sPAPRpciMSI;
 
-typedef struct spapr_pci_msi_mig {
+typedef struct sPAPRpciMSImig {
     uint32_t key;
-    spapr_pci_msi value;
-} spapr_pci_msi_mig;
+    sPAPRpciMSI value;
+} sPAPRpciMSImig;
 
 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;
+    sPAPRpciMSImig *msi_devs;
 
     QLIST_ENTRY(sPAPRPHBState) list;
 

Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types

[Alexey Kardashevskiy]

On 12/10/2018 20:05, Greg Kurz wrote:
> According to CODING_STYLE, structured types names are expected to be
> in CamelCase but we have:
> 
> typedef struct spapr_pci_msi {
>     uint32_t first_irq;
>     uint32_t num;
> } spapr_pci_msi;
> 
> typedef struct spapr_pci_msi_mig {
>     uint32_t key;
>     spapr_pci_msi value;
> } spapr_pci_msi_mig;
> 
> Acronyms are often written in upper-case, but here we would en up with
> a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
> purpose of CamelCase. It even displays "RPC" which looks awkward.

Yet more common than this. I vote for sPAPRPCIMSI as PCI is an acronym.
"pci" is small letters hurts my eyes :)



> This patch twists the rule a bit to keep the type names readable:
> sPAPRpciMSI and sPAPRpciMSImig.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - moved g_malloc change to a separate patch
>     - new naming proposal that doesn't drop PCI
>     - more detailed description in the changelog
> ---
>  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 0537ce018f51..2ee933e2d1ec 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;
> +    sPAPRpciMSI *msi;
>      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 = (sPAPRpciMSI *) 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(sPAPRpciMSI, 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;
> +    sPAPRpciMSI *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 = (sPAPRpciMSI *) 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, sPAPRpciMSImig),
> +        VMSTATE_UINT32(value.first_irq, sPAPRpciMSImig),
> +        VMSTATE_UINT32(value.num, sPAPRpciMSImig),
>          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_new(spapr_pci_msi_mig, sphb->msi_devs_num);
> +    sphb->msi_devs = g_new(sPAPRpciMSImig, 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 = *(sPAPRpciMSI *) 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, sPAPRpciMSImig),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 7c66c3872f96..eb8436b4fc32 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 sPAPRpciMSI {
>      uint32_t first_irq;
>      uint32_t num;
> -} spapr_pci_msi;
> +} sPAPRpciMSI;
>  
> -typedef struct spapr_pci_msi_mig {
> +typedef struct sPAPRpciMSImig {
>      uint32_t key;
> -    spapr_pci_msi value;
> -} spapr_pci_msi_mig;
> +    sPAPRpciMSI value;
> +} sPAPRpciMSImig;
>  
>  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;
> +    sPAPRpciMSImig *msi_devs;
>  
>      QLIST_ENTRY(sPAPRPHBState) list;
>  
> 
> 

-- 
Alexey

Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

On Mon, Oct 15, 2018 at 12:49:53PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 12/10/2018 20:05, Greg Kurz wrote:
> > According to CODING_STYLE, structured types names are expected to be
> > in CamelCase but we have:
> > 
> > typedef struct spapr_pci_msi {
> >     uint32_t first_irq;
> >     uint32_t num;
> > } spapr_pci_msi;
> > 
> > typedef struct spapr_pci_msi_mig {
> >     uint32_t key;
> >     spapr_pci_msi value;
> > } spapr_pci_msi_mig;
> > 
> > Acronyms are often written in upper-case, but here we would en up with
> > a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
> > purpose of CamelCase. It even displays "RPC" which looks awkward.
> 
> Yet more common than this. I vote for sPAPRPCIMSI as PCI is an acronym.
> "pci" is small letters hurts my eyes :)

Hrm.  So, yes, I know I kind of started it, but these various
compromises about how to captialize things means this patch is now
"change from non-camelcase to.. something that's also not really
camelcase".

At which point I'm not particularly convinced it's worth the bother.

If we really want to go ahead with CamelCasing this, I think we'd need
to start by fixing up the existing sorta-camelcase-but-not-really
stuff to actual camelcase.  Which would mean the slightly odd reading
"Spapr" and "Pci" and "Msi" and so forth, but it might be worth it for
consistency.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

On Wed, 7 Nov 2018 15:46:35 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Oct 15, 2018 at 12:49:53PM +1100, Alexey Kardashevskiy wrote:
> > 
> > 
> > On 12/10/2018 20:05, Greg Kurz wrote:  
> > > According to CODING_STYLE, structured types names are expected to be
> > > in CamelCase but we have:
> > > 
> > > typedef struct spapr_pci_msi {
> > >     uint32_t first_irq;
> > >     uint32_t num;
> > > } spapr_pci_msi;
> > > 
> > > typedef struct spapr_pci_msi_mig {
> > >     uint32_t key;
> > >     spapr_pci_msi value;
> > > } spapr_pci_msi_mig;
> > > 
> > > Acronyms are often written in upper-case, but here we would en up with
> > > a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
> > > purpose of CamelCase. It even displays "RPC" which looks awkward.  
> > 
> > Yet more common than this. I vote for sPAPRPCIMSI as PCI is an acronym.
> > "pci" is small letters hurts my eyes :)  
> 
> Hrm.  So, yes, I know I kind of started it, but these various
> compromises about how to captialize things means this patch is now
> "change from non-camelcase to.. something that's also not really
> camelcase".
> 
> At which point I'm not particularly convinced it's worth the bother.
> 
> If we really want to go ahead with CamelCasing this, I think we'd need
> to start by fixing up the existing sorta-camelcase-but-not-really
> stuff to actual camelcase.  Which would mean the slightly odd reading
> "Spapr" and "Pci" and "Msi" and so forth, but it might be worth it for
> consistency.
> 

Looking at sPAPR only we already get:

$ git grep sPAPR | wc -l
1070

I understand your point but this would cause a lot of changes,
ie, a lot of noise in git blame and probably harder backports
of subsequent commits... I guess it isn't worth the pain.

Maybe we can just forget this patch and live with this minor
coding style violation. :)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]