[Qemu-devel] [PATCH] Fix memory leak on error

[Qemu-devel] [PATCH] Fix memory leak on error

[Stefano Dong (董兴水)]

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) {
-- 
2.5.1.windows.1

Re: [Qemu-devel] [PATCH] Fix memory leak on error

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] Fix memory leak on error

[Igor Mammedov]

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) {

Re: [Qemu-devel] [PATCH] Fix memory leak on error

[Stefano Dong]

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!

> 

Re: [Qemu-devel] [PATCH] Fix memory leak on error

[Markus Armbruster]

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!