[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit |
Date: |
Thu, 06 Jun 2013 21:01:12 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
Il 05/06/2013 06:32, Michael S. Tsirkin ha scritto:
> 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.
When an object is created, it is all-zeroed.
>>> 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 ...
Can't, because table_size/pba_size is not available at init time (e.g.
for VFIO not until the host BARs are processed). What about using
g_realloc + memset?
Paolo
- [Qemu-devel] [PATCH 07/39] serial: split exit and instance_finalize, (continued)
- [Qemu-devel] [PATCH 07/39] serial: split exit and instance_finalize, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH 08/39] tpci200: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH 09/39] pci-assign: split exit and instance_finalize, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH 10/39] ahci: split exit and instance_finalize, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 12/39] cmd646: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 13/39] ide/piix: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 14/39] ide/via: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 15/39] ivshmem: split exit and instance_finalize, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 16/39] pci-testdev: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 17/39] vfio: split exit and instance_finalize, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 18/39] e1000: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 19/39] eepro100: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 20/39] ne2000: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH 21/39] pcnet: use instance_finalize instead of exit, Paolo Bonzini, 2013/06/04