From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQFKi-0003Dh-0a for qemu-devel@nongnu.org; Thu, 21 Jul 2016 10:52:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQFKe-0008R3-2f for qemu-devel@nongnu.org; Thu, 21 Jul 2016 10:52:16 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:33999) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQFKd-0008Qu-Qv for qemu-devel@nongnu.org; Thu, 21 Jul 2016 10:52:12 -0400 Received: by mail-lf0-x241.google.com with SMTP id l69so5680081lfg.1 for ; Thu, 21 Jul 2016 07:52:11 -0700 (PDT) Reply-To: marcel@redhat.com References: <20160719085432.4572-1-marcandre.lureau@redhat.com> <20160719085432.4572-19-marcandre.lureau@redhat.com> From: Marcel Apfelbaum Message-ID: <5790E196.1030203@gmail.com> Date: Thu, 21 Jul 2016 17:52:06 +0300 MIME-Version: 1.0 In-Reply-To: <20160719085432.4572-19-marcandre.lureau@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit 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: marcandre.lureau@redhat.com, qemu-devel@nongnu.org On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau > > 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 > --- > 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. 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? Thanks, Marcel > } > > /* >