Re: [Qemu-devel] [PATCH 01/29] hw/ide/ahci.c: Fix memory leak spotted by valgrind

[Peter Maydell <peter.maydell@linaro.org>]

On 28 May 2015 at 13:24, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>
>
> On 2015/5/28 20:16, Peter Maydell wrote:
>> On 28 May 2015 at 13:08, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>>> > --- a/hw/ide/ahci.c
>>> > +++ b/hw/ide/ahci.c
>>> > @@ -1367,6 +1367,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
>>> >          ad->port.dma->ops = &ahci_dma_ops;
>>> >          ide_register_restart_cb(&ad->port);
>>> >      }
>>> > +    g_free(irqs);
>>> >  }
>> Freeing the return value of qemu_allocate_irqs()
>> directly rather than using qemu_free_irqs() looks
>> dubious to me.
>
> qemu_free_irqs() will free the return value self of qemu_allocate_irqs()
> and also will free all the contents of this array while some of them are
> still in use.

Yes, but you're still looking into the internal implementation
details of this API, which is what I'm suggesting is dubious.

For instance, in this case, why are we calling
qemu_allocate_irqs() in the first place, rather than
just calling qemu_allocate_irq() inside the for() loop in
ahci_init()?

If code wants to deal with the array of irqs as an array,
it should probably be saving the pointer somewhere in its
state struct and freeing the whole thing on deinit. If it
doesn't actually want an array of irqs (as here) it should
probably not be calling qemu_allocate_irqs() in the first
place.

thanks
-- PMM