From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46895) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uk8pA-0001Lz-Oj for qemu-devel@nongnu.org; Wed, 05 Jun 2013 04:12:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uk8p9-0000u3-0q for qemu-devel@nongnu.org; Wed, 05 Jun 2013 04:12:04 -0400 Received: from mail-wg0-x233.google.com ([2a00:1450:400c:c00::233]:43150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uk8SQ-0001qu-RR for qemu-devel@nongnu.org; Wed, 05 Jun 2013 03:48:34 -0400 Received: by mail-wg0-f51.google.com with SMTP id b13so1020984wgh.6 for ; Wed, 05 Jun 2013 00:48:34 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <51AEED43.2080302@redhat.com> Date: Wed, 05 Jun 2013 09:48:19 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1370371954-8479-1-git-send-email-pbonzini@redhat.com> <1370371954-8479-12-git-send-email-pbonzini@redhat.com> <20130604220328.GB30400@redhat.com> <51AE6CC0.4050808@redhat.com> <20130605045348.GA31253@redhat.com> In-Reply-To: <20130605045348.GA31253@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto: > On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote: >> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: >>>>> + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { >>>>> + msix_free(dev); >>>>> + } >>>>> + >>>>> dev->msix_table = g_malloc0(table_size); >>>>> dev->msix_pba = g_malloc0(pba_size); >>>>> dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); >>> Wow msix_init calls msix_free, and not on error path? >>> What's going on here? >> >> I wasn't too sure that you could get here only with NULL >> msix_table/pba/entry_used and wanted to protect against leaks. I'll >> change it to an assertion. > > I don't think we should require users allocate all memory with g_malloc0. > So no assertion either. Assertion that is is NULL, followed by g_malloc0? > If there's a leak there was always a leak No, there wasn't because msix_uninit would have freed the memory. That is, msix_init msix_uninit msix_init msix_uninit had no leak. Instead, now msix_free is going to be called just once, right before freeing the object itself: msix_init msix_uninit msix_init *** msix_uninit msix_free and will have a leak at ***. I don't think this can happen, unrealize should never be followed by another realize right now, but perhaps in the future it will be if we implement something like "device_poweroff" and "device_poweron". Paolo , let's focus on the > API change in this series, OK? > >>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) >>>>> msix_free_irq_entries(dev); >>>>> dev->msix_entries_nr = 0; >>>>> memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); >>>>> - memory_region_destroy(&dev->msix_pba_mmio); >>>>> - g_free(dev->msix_pba); >>>>> - dev->msix_pba = NULL; >>>>> memory_region_del_subregion(table_bar, &dev->msix_table_mmio); >>>>> - memory_region_destroy(&dev->msix_table_mmio); >>>>> - g_free(dev->msix_table); >>>>> + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; >>>>> +} >>>>> + >>>>> +void msix_free(PCIDevice *dev) >>>>> +{ >>>>> + if (dev->msix_pba) { >>>>> + memory_region_destroy(&dev->msix_pba_mmio); >>>>> + g_free(dev->msix_pba); >>>>> + } >>>>> + dev->msix_pba = NULL; >>>>> + >>>>> + if (dev->msix_table) { >>>>> + memory_region_destroy(&dev->msix_table_mmio); >>>>> + g_free(dev->msix_table); >>>>> + } >>>>> dev->msix_table = NULL; >>>>> + >>>>> g_free(dev->msix_entry_used); >>>>> dev->msix_entry_used = NULL; >>>>> - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; >>>>> } >>>>> >>>>> void msix_uninit_exclusive_bar(PCIDevice *dev) >>> As long as we had init and uninit, it was mostly >>> self-documenting. >>> Now, there are two cleanup functions, so please add documentation. >> >> Yes, will do. >> >> Paolo > >