From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50448) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQH8Q-0003Cx-P7 for qemu-devel@nongnu.org; Thu, 21 Jul 2016 12:47:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQH8L-0004HH-Mj for qemu-devel@nongnu.org; Thu, 21 Jul 2016 12:47:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32797) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQH8L-0004HC-Ct for qemu-devel@nongnu.org; Thu, 21 Jul 2016 12:47:37 -0400 References: <20160719085432.4572-1-marcandre.lureau@redhat.com> <20160719085432.4572-19-marcandre.lureau@redhat.com> <5790E196.1030203@gmail.com> <5790EF75.1080507@redhat.com> From: Marcel Apfelbaum Message-ID: <5790FCA2.1000501@redhat.com> Date: Thu, 21 Jul 2016 19:47:30 +0300 MIME-Version: 1.0 In-Reply-To: <5790EF75.1080507@redhat.com> 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:51 PM, Marcel Apfelbaum wrote: > 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 segm= ent >>>> 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(GPtrArr= ay >>>> *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 w= rapper, >>> preserved the pointers (which we need), but also the inner array whic= h 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=3Dp= ci.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 rec= ord >> 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 wo= rk? > I'll test it and get back to you. Everything works, thank you! Tested-by: Marcel Apfelbaum Reviewed-by: Marcel Apfelbaum > > Thanks, > Marcel > > > >>> >>> >>> Thanks, >>> Marcel >>> >>>> } >>>> >>>> /* >>>> >>> >>> >> >> >> > --=20