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

[Paolo Bonzini]

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