[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc: Fix some memory leaks
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc: Fix some memory leaks |
Date: |
Tue, 16 Jul 2019 18:28:42 +0200 |
On Tue, 16 Jul 2019 03:24:57 -0500
Shivaprasad G Bhat <address@hidden> wrote:
> valgrind showed some memory leaks while running qemu-system-ppc64.
> Fixing them in this patch.
>
Since this covers many unrelated code paths, I think it would be better to
make this a series of separate patches, rather than one patch.
> Signed-off-by: Shivaprasad G Bhat <address@hidden>
> ---
> hw/ppc/spapr_caps.c | 2 ++
> hw/ppc/spapr_drc.c | 5 ++++-
> hw/ppc/spapr_hcall.c | 2 ++
> target/ppc/kvm.c | 3 ++-
> 4 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index bbb001f84a..8e3350f777 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -786,11 +786,13 @@ void spapr_caps_add_properties(SpaprMachineClass *smc,
> Error **errp)
> NULL, cap, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> + g_free((char *)name);
Like Richard indicated in another mail, this is wrong : 'const char *'
means that the string is constant forever. If you have to free it, then
it shouldn't be const in the first place.
> return;
> }
>
> desc = g_strdup_printf("%s", cap->description);
> object_class_property_set_description(klass, name, desc, &local_err);
> + g_free((char *)name);
> g_free(desc);
> if (local_err) {
> error_propagate(errp, local_err);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index bacadfcac5..37fbfe6900 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -827,6 +827,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner,
> uint32_t drc_type_mask)
> Object *obj;
> SpaprDrc *drc;
> SpaprDrcClass *drck;
> + const char *drc_name = NULL;
Same here.
> uint32_t drc_index, drc_power_domain;
>
> if (!strstart(prop->type, "link<", NULL)) {
> @@ -856,8 +857,10 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner,
> uint32_t drc_type_mask)
> g_array_append_val(drc_power_domains, drc_power_domain);
>
> /* ibm,drc-names */
> - drc_names = g_string_append(drc_names, spapr_drc_name(drc));
> + drc_name = spapr_drc_name(drc);
> + drc_names = g_string_append(drc_names, drc_name);
> drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
> + g_free((char *)drc_name);
>
> /* ibm,drc-types */
> drc_types = g_string_append(drc_types, drck->typename);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 6808d4cda8..0fc58156a0 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1612,6 +1612,7 @@ static target_ulong
> h_client_architecture_support(PowerPCCPU *cpu,
> ov5_updates = spapr_ovec_new();
> spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
> ov5_cas_old, spapr->ov5_cas);
> + spapr_ovec_cleanup(ov5_cas_old);
> /* Now that processing is finished, set the radix/hash bit for the
> * guest if it requested a valid mode; otherwise terminate the boot. */
> if (guest_radix) {
> @@ -1640,6 +1641,7 @@ static target_ulong
> h_client_architecture_support(PowerPCCPU *cpu,
> (spapr_h_cas_compose_response(spapr, args[1], args[2],
> ov5_updates) != 0);
> }
> + spapr_ovec_cleanup(ov1_guest);
>
Maybe a few lines ealier, just after its last user:
spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
OV1_PPC_3_00);
> /*
> * Ensure the guest asks for an interrupt mode we support; otherwise
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8a06d3171e..498ca6d53b 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -491,7 +491,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
> return ret;
> }
>
> - idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
> + if (!idle_timer)
> + idle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, kvm_kick_cpu, cpu);
>
Yikes... idle_timer is a static but it is used by all vcpus... it looks
like it's a bug.
> switch (cenv->mmu_model) {
> case POWERPC_MMU_BOOKE206:
>
>