Message ID | 1534870966-9287-46-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PULL,01/74] tests: virtio: separate ccw tests from libqos | expand |
On 21 August 2018 at 18:02, Paolo Bonzini <pbonzini@redhat.com> wrote: > From: "Emilio G. Cota" <cota@braap.org> > > This paves the way for implementing the CPU list with an RCU list, > which cannot be traversed in reverse order. > > Note that this is the only caller of CPU_FOREACH_REVERSE. > > Acked-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Emilio G. Cota <cota@braap.org> > Message-Id: <20180819091335.22863-11-cota@braap.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/ppc/spapr.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index e5d8253..ab9c04e 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -622,9 +622,12 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset, > > static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr) > { > + CPUState **rev; > CPUState *cs; > + int n_cpus; > int cpus_offset; > char *nodename; > + int i; > > cpus_offset = fdt_add_subnode(fdt, 0, "cpus"); > _FDT(cpus_offset); > @@ -635,8 +638,19 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr) > * We walk the CPUs in reverse order to ensure that CPU DT nodes > * created by fdt_add_subnode() end up in the right order in FDT > * for the guest kernel the enumerate the CPUs correctly. > + * > + * The CPU list cannot be traversed in reverse order, so we need > + * to do extra work. > */ > - CPU_FOREACH_REVERSE(cs) { > + n_cpus = 0; > + rev = NULL; > + CPU_FOREACH(cs) { > + rev = g_renew(CPUState *, rev, n_cpus + 1); > + rev[n_cpus++] = cs; > + } > + > + for (i = n_cpus - 1; i >= 0; i--) { > + CPUState *cs = rev[i]; > PowerPCCPU *cpu = POWERPC_CPU(cs); > int index = spapr_get_vcpu_id(cpu); > DeviceClass *dc = DEVICE_GET_CLASS(cs); > -- Hi -- Coverity points out in CID1395181 that this introduces a memory leak -- we allocate memory into the rev pointer with g_renew(), but we never free it before leaving the function. thanks -- PMM
On Fri, Aug 24, 2018 at 16:20:24 +0100, Peter Maydell wrote: (snip) > Hi -- Coverity points out in CID1395181 that this introduces > a memory leak -- we allocate memory into the rev pointer > with g_renew(), but we never free it before leaving the function. Thanks for the heads up; fix forthcoming. Emilio
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index e5d8253..ab9c04e 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -622,9 +622,12 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset, static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr) { + CPUState **rev; CPUState *cs; + int n_cpus; int cpus_offset; char *nodename; + int i; cpus_offset = fdt_add_subnode(fdt, 0, "cpus"); _FDT(cpus_offset); @@ -635,8 +638,19 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr) * We walk the CPUs in reverse order to ensure that CPU DT nodes * created by fdt_add_subnode() end up in the right order in FDT * for the guest kernel the enumerate the CPUs correctly. + * + * The CPU list cannot be traversed in reverse order, so we need + * to do extra work. */ - CPU_FOREACH_REVERSE(cs) { + n_cpus = 0; + rev = NULL; + CPU_FOREACH(cs) { + rev = g_renew(CPUState *, rev, n_cpus + 1); + rev[n_cpus++] = cs; + } + + for (i = n_cpus - 1; i >= 0; i--) { + CPUState *cs = rev[i]; PowerPCCPU *cpu = POWERPC_CPU(cs); int index = spapr_get_vcpu_id(cpu); DeviceClass *dc = DEVICE_GET_CLASS(cs);