From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uk5is-0000nR-4m for qemu-devel@nongnu.org; Wed, 05 Jun 2013 00:53:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uk5il-00043X-Vg for qemu-devel@nongnu.org; Wed, 05 Jun 2013 00:53:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30392) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uk5il-00041h-Ne for qemu-devel@nongnu.org; Wed, 05 Jun 2013 00:53:15 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r554rEgl030433 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 5 Jun 2013 00:53:14 -0400 Date: Wed, 5 Jun 2013 07:53:48 +0300 From: "Michael S. Tsirkin" Message-ID: <20130605045348.GA31253@redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51AE6CC0.4050808@redhat.com> 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: Paolo Bonzini Cc: qemu-devel@nongnu.org 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. If there's a leak there was always a leak, 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