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
