On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote: > 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?
No because who sets it to NULL the first time? msix_init just started. > > 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 ***. Yes. And this looks completely sane from outside, so this is a bad API. The way to fix it is not with asserts in code, we need a good API: alloc/free init/uninit ... The problem apparently starts in generic code, let's fix it there? > I don't think this can happen, unrealize > should never be followed by another realize right now, This is not an msix specific problem, I don't think msix should debug generic core - this will just lead to proliferation of asserts. This really should be documented prominently in generic code. Also how about some asserts in generic code making sure ordering is sane? > 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 > > > >