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

[Markus Armbruster]

Stefano Dong (董兴水) <address@hidden> 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 (董兴水) <address@hidden>
> ---
>  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" <address@hidden> (supporter:ACPI/SMBIOS)
    Igor Mammedov <address@hidden> (supporter:ACPI/SMBIOS)
    $ scripts/get_maintainer.pl -f hw/ppc/spapr.c 
    David Gibson <address@hidden> (supporter:sPAPR (pseries))
    Alexander Graf <address@hidden> (supporter:sPAPR (pseries))
    Eduardo Habkost <address@hidden> (maintainer:NUMA)
    address@hidden (open list:sPAPR (pseries))

If the combined patch is trivial, cc: address@hidden  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.