On 07/21/2016 06:48 PM, Marc-André Lureau wrote:
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)
I am sure the leak is gone :), but the devices attached to PXB still work?
I'll test it and get back to you.
Thanks,
Marcel
Thanks,
Marcel
}
/*