From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35049) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQGFz-0000HX-9B for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:51:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQGFu-0006uy-7y for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:51:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52655) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQGFu-0006uZ-09 for qemu-devel@nongnu.org; Thu, 21 Jul 2016 11:51:22 -0400 References: <20160719085432.4572-1-marcandre.lureau@redhat.com> <20160719085432.4572-19-marcandre.lureau@redhat.com> <5790E196.1030203@gmail.com> From: Marcel Apfelbaum Message-ID: <5790EF75.1080507@redhat.com> Date: Thu, 21 Jul 2016 18:51:17 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 18/37] acpi-build: fix array leak List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: QEMU On 07/21/2016 06:48 PM, Marc-Andr=E9 Lureau wrote: > Hi > > On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum > wrote: >> On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote: >>> >>> From: Marc-Andr=E9 Lureau >>> >>> The free_ranges array is used as a temporary pointer array, the segme= nt >>> 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=E9 Lureau >>> --- >>> 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 =3D >>> g_ptr_array_new_with_free_func(crs_range_free); >>> + GPtrArray *free_ranges =3D 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 =3D start; >>> int i; >>> >>> @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArra= y >>> *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 wr= apper, >> 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=3Dpxb,bus_nr=3D0x80,bus=3Dpc= i.0 -device >> e1000,bus=3Dpxb 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: > > =3D=3D20313=3D=3D 32 bytes in 1 blocks are definitely lost in loss reco= rd > 5,326 of 10,190 > =3D=3D20313=3D=3D at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) > =3D=3D20313=3D=3D by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0= .so.0.4800.1) > =3D=3D20313=3D=3D by 0x1E181D42: g_slice_alloc (in > /usr/lib64/libglib-2.0.so.0.4800.1) > =3D=3D20313=3D=3D by 0x1E139880: g_ptr_array_sized_new (in > /usr/lib64/libglib-2.0.so.0.4800.1) > =3D=3D20313=3D=3D by 0x1E13991D: g_ptr_array_new_with_free_func (in > /usr/lib64/libglib-2.0.so.0.4800.1) > =3D=3D20313=3D=3D by 0x3E8BE8: crs_range_merge (acpi-build.c:797) > =3D=3D20313=3D=3D by 0x3E8FE6: build_crs (acpi-build.c:918) > =3D=3D20313=3D=3D by 0x3ED857: build_dsdt (acpi-build.c:2014) > =3D=3D20313=3D=3D by 0x3EF659: acpi_build (acpi-build.c:2590) > =3D=3D20313=3D=3D by 0x3EFE61: acpi_setup (acpi-build.c:2793) > =3D=3D20313=3D=3D by 0x3D810D: pc_machine_done (pc.c:1270) > =3D=3D20313=3D=3D 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 >> >>> } >>> >>> /* >>> >> >> > > >