* [Qemu-devel] [PATCH] spapr_pci: rename some structured types
@ 2018-10-11 7:00 Greg Kurz
2018-10-11 11:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 5+ messages in thread
From: Greg Kurz @ 2018-10-11 7:00 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson
According to CODING_STYLE, structured types names are expected to be
in CamelCase. PCI is dropped from the name for better readability.
While here, this also converts a call to g_malloc(n * sizeofi(foo)) to
g_new(n, foo), which is a recommended good practice.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
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;
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;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr_pci: rename some structured types
2018-10-11 7:00 [Qemu-devel] [PATCH] spapr_pci: rename some structured types Greg Kurz
@ 2018-10-11 11:02 ` Philippe Mathieu-Daudé
2018-10-11 13:04 ` Greg Kurz
0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-11 11:02 UTC (permalink / raw)
To: Greg Kurz, qemu-devel; +Cc: qemu-ppc, David Gibson
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.
>
> While here, this also converts a call to g_malloc(n * sizeofi(foo)) to
sizeof(foo)
> g_new(n, foo), which is a recommended good practice.
Clearly 2 different patches...
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 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.
> 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;
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr_pci: rename some structured types
2018-10-11 11:02 ` Philippe Mathieu-Daudé
@ 2018-10-11 13:04 ` Greg Kurz
2018-10-12 0:17 ` David Gibson
0 siblings, 1 reply; 5+ messages in thread
From: Greg Kurz @ 2018-10-11 13:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-ppc, David Gibson
On Thu, 11 Oct 2018 13:02:50 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> 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.
> >
> > 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 :)
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 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.
>
> > 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;
> >
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr_pci: rename some structured types
2018-10-11 13:04 ` Greg Kurz
@ 2018-10-12 0:17 ` David Gibson
2018-10-12 8:24 ` Greg Kurz
0 siblings, 1 reply; 5+ messages in thread
From: David Gibson @ 2018-10-12 0:17 UTC (permalink / raw)
To: Greg Kurz; +Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 7015 bytes --]
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é <philmd@redhat.com> 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.
> > >
> > > 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.
>
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > 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;
> > >
> > >
> > >
>
--
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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr_pci: rename some structured types
2018-10-12 0:17 ` David Gibson
@ 2018-10-12 8:24 ` Greg Kurz
0 siblings, 0 replies; 5+ messages in thread
From: Greg Kurz @ 2018-10-12 8:24 UTC (permalink / raw)
To: David Gibson; +Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 7945 bytes --]
On Fri, 12 Oct 2018 11:17:32 +1100
David Gibson <david@gibson.dropbear.id.au> 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é <philmd@redhat.com> 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 <groug@kaod.org>
> > > > ---
> > > > 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;
> > > >
> > > >
> > > >
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-12 8:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-11 7:00 [Qemu-devel] [PATCH] spapr_pci: rename some structured types Greg Kurz
2018-10-11 11:02 ` Philippe Mathieu-Daudé
2018-10-11 13:04 ` Greg Kurz
2018-10-12 0:17 ` David Gibson
2018-10-12 8:24 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.