| Message ID | 1448539212-7080-1-git-send-email-opensource.dxs@aliyun.com |
|---|---|
| State | New |
| Headers | show |
Stefano Dong (董兴水) <opensource.dxs@aliyun.com> writes: > hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611 > hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 34f2af3d > > Signed-off-by: Stefano Dong (董兴水) <opensource.dxs@aliyun.com> > --- > hw/acpi/memory_hotplug.c | 1 + > hw/ppc/spapr.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index ce428df..e4b9a01 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, > qapi_event_send_mem_unplug_error(dev->id, > error_get_pretty(local_err), > &error_abort); > + error_free(local_err); > break; > } > trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 030ee35..3bb8bcd 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine, > error_report("kernel_irqchip requested but unavailable: %s", > error_get_pretty(err)); > } > + error_free(err); > } > > if (!icp) { Two independent instances of the same kind of bug (failure to free an Error after handling it). Collecting multiple fixes of the same kind in one patch can be fine. Note, however, that the combined patch spans separately maintained areas of the code: $ scripts/get_maintainer.pl -f hw/acpi/memory_hotplug.c "Michael S. Tsirkin" <mst@redhat.com> (supporter:ACPI/SMBIOS) Igor Mammedov <imammedo@redhat.com> (supporter:ACPI/SMBIOS) $ scripts/get_maintainer.pl -f hw/ppc/spapr.c David Gibson <david@gibson.dropbear.id.au> (supporter:sPAPR (pseries)) Alexander Graf <agraf@suse.de> (supporter:sPAPR (pseries)) Eduardo Habkost <ehabkost@redhat.com> (maintainer:NUMA) qemu-ppc@nongnu.org (open list:sPAPR (pseries)) If the combined patch is trivial, cc: qemu-trivial@nongnu.org. I figure this one could qualify. If it isn't, split it up along mainenance boundaries, and cc: the maintainer(s) for each part. When you fix something with serious impact, the commit message should give a clue when it was broken. For minor bugs like these, it's still nice to do. The acpi_memory_hotplug_write() leak was introduced in bc09e06 (v2.4.0), and the xics_system_init() leak in commit 34f2af3 (v2.3.0). Thanks for fixing the mess I made there, by the way. Since this is a straightforward bug fix, proposing it for inclusion into 2.5 makes sense. You can do that by putting [PATCH for-2.5] into the subject.
On Thu, 26 Nov 2015 12:00:12 +0000 Stefano Dong (董兴水) <opensource.dxs@aliyun.com> wrote: > hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611 > hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 34f2af3d > > Signed-off-by: Stefano Dong (董兴水) <opensource.dxs@aliyun.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/acpi/memory_hotplug.c | 1 + > hw/ppc/spapr.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index ce428df..e4b9a01 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, > qapi_event_send_mem_unplug_error(dev->id, > error_get_pretty(local_err), > &error_abort); > + error_free(local_err); > break; > } > trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 030ee35..3bb8bcd 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine, > error_report("kernel_irqchip requested but unavailable: %s", > error_get_pretty(err)); > } > + error_free(err); > } > > if (!icp) {
On 2015/11/26 20:33, Markus Armbruster wrote: > Stefano Dong (董兴水) <opensource.dxs@aliyun.com> writes: > >> hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611 >> hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 34f2af3d >> >> Signed-off-by: Stefano Dong (董兴水) <opensource.dxs@aliyun.com> >> --- >> hw/acpi/memory_hotplug.c | 1 + >> hw/ppc/spapr.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c >> index ce428df..e4b9a01 100644 >> --- a/hw/acpi/memory_hotplug.c >> +++ b/hw/acpi/memory_hotplug.c >> @@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, >> qapi_event_send_mem_unplug_error(dev->id, >> error_get_pretty(local_err), >> &error_abort); >> + error_free(local_err); >> break; >> } >> trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 030ee35..3bb8bcd 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine, >> error_report("kernel_irqchip requested but unavailable: %s", >> error_get_pretty(err)); >> } >> + error_free(err); >> } >> >> if (!icp) { > > Two independent instances of the same kind of bug (failure to free an > Error after handling it). Collecting multiple fixes of the same kind in > one patch can be fine. Note, however, that the combined patch spans > separately maintained areas of the code: > > $ scripts/get_maintainer.pl -f hw/acpi/memory_hotplug.c > "Michael S. Tsirkin" <mst@redhat.com> (supporter:ACPI/SMBIOS) > Igor Mammedov <imammedo@redhat.com> (supporter:ACPI/SMBIOS) > $ scripts/get_maintainer.pl -f hw/ppc/spapr.c > David Gibson <david@gibson.dropbear.id.au> (supporter:sPAPR (pseries)) > Alexander Graf <agraf@suse.de> (supporter:sPAPR (pseries)) > Eduardo Habkost <ehabkost@redhat.com> (maintainer:NUMA) > qemu-ppc@nongnu.org (open list:sPAPR (pseries)) > > If the combined patch is trivial, cc: qemu-trivial@nongnu.org. I figure > this one could qualify. > > If it isn't, split it up along mainenance boundaries, and cc: the > maintainer(s) for each part. > > When you fix something with serious impact, the commit message should > give a clue when it was broken. For minor bugs like these, it's still > nice to do. The acpi_memory_hotplug_write() leak was introduced in > bc09e06 (v2.4.0), and the xics_system_init() leak in commit 34f2af3 > (v2.3.0). Thanks for fixing the mess I made there, by the way. > > Since this is a straightforward bug fix, proposing it for inclusion into > 2.5 makes sense. You can do that by putting [PATCH for-2.5] into the > subject. Thank you Markus! Thanks for teaching me a lot! You are so kind and friendly! >
Stefano Dong <opensource.dxs@aliyun.com> writes:
> Thank you Markus! Thanks for teaching me a lot! You are so kind and friendly!
If I was able to make your initial contribution a pleasant experience,
then I'm glad. Thanks for the fixes!
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index ce428df..e4b9a01 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -155,6 +155,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, qapi_event_send_mem_unplug_error(dev->id, error_get_pretty(local_err), &error_abort); + error_free(local_err); break; } trace_mhp_acpi_pc_dimm_deleted(mem_st->selector); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 030ee35..3bb8bcd 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -125,6 +125,7 @@ static XICSState *xics_system_init(MachineState *machine, error_report("kernel_irqchip requested but unavailable: %s", error_get_pretty(err)); } + error_free(err); } if (!icp) {
hw/ppc/spapr.c: Fix memory leak on error, it was introduced in bc09e0611 hw/acpi/memory_hotplug.c: Fix memory leak on error, it was introduced in 34f2af3d Signed-off-by: Stefano Dong (董兴水) <opensource.dxs@aliyun.com> --- hw/acpi/memory_hotplug.c | 1 + hw/ppc/spapr.c | 1 + 2 files changed, 2 insertions(+)