Hi On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum <marcel.apfelb...@gmail.com> wrote: > On 07/19/2016 11:54 AM, marcandre.lur...@redhat.com wrote: >> >> From: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> The free_ranges array is used as a temporary pointer array, the segment >> should still be freed, > > > Right. If I understand, this is the leak. > > however, it shouldn't free the elements themself. > > And it didn't, right? otherwise it would not work since these ranges > are used later. > >> >> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> --- >> hw/i386/acpi-build.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index fbba461..f4ba3a4 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -761,7 +761,7 @@ static gint crs_range_compare(gconstpointer a, >> gconstpointer b) >> static void crs_replace_with_free_ranges(GPtrArray *ranges, >> uint64_t start, uint64_t end) >> { >> - GPtrArray *free_ranges = >> g_ptr_array_new_with_free_func(crs_range_free); >> + GPtrArray *free_ranges = g_ptr_array_new(); > > > Indeed, we are not going to free the ranges in this array, adding the > GDestroyNotify > here is not needed. > >> uint64_t free_base = start; >> int i; >> >> @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArray >> *ranges, >> g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i)); >> } >> >> - g_ptr_array_free(free_ranges, false); >> + g_ptr_array_free(free_ranges, true); > > > This *is* scary since "true" means delete everything, but looking at > documentation: > "If array contents point to dynamically-allocated memory, > they should be freed separately if free_seg is TRUE and > no GDestroyNotify function has been set for array." > So your approach should work. > > I think I understand the leak. Previous approach deleted the GArray wrapper, > preserved the pointers (which we need), but also the inner array which we > don't.
yes, it's only the inner array we need to free. > > One question: how did you test that it still works :) ? > Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device > e1000,bus=pxb and see the device > e100 device gets the required resources? If you run this under it valgrind you get, after the patch the leak is gone: ==20313== 32 bytes in 1 blocks are definitely lost in loss record 5,326 of 10,190 ==20313== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) ==20313== by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x1E181D42: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x1E139880: g_ptr_array_sized_new (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x1E13991D: g_ptr_array_new_with_free_func (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x3E8BE8: crs_range_merge (acpi-build.c:797) ==20313== by 0x3E8FE6: build_crs (acpi-build.c:918) ==20313== by 0x3ED857: build_dsdt (acpi-build.c:2014) ==20313== by 0x3EF659: acpi_build (acpi-build.c:2590) ==20313== by 0x3EFE61: acpi_setup (acpi-build.c:2793) ==20313== by 0x3D810D: pc_machine_done (pc.c:1270) ==20313== by 0x7E6A23: notifier_list_notify (notify.c:40) > > > Thanks, > Marcel > >> } >> >> /* >> > > -- Marc-André Lureau