Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Paolo Bonzini]

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.

>> > @@ -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