Re: [PATCH 1/8] ide: fix leak from qemu_allocate_irqs

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/10/19 17:58, Peter Maydell wrote:
> On Tue, 1 Oct 2019 at 14:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> The array returned by qemu_allocate_irqs is malloced, free it.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/ide/cmd646.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index f3ccd11..19984d2 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -300,6 +300,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>>          d->bmdma[i].bus = &d->bus[i];
>>          ide_register_restart_cb(&d->bus[i]);
>>      }
>> +    g_free(irq);
>>
>>      vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
>>      qemu_register_reset(cmd646_reset, d);
>> --
>> 1.8.3.1
> 
> It's a bit weird to be calling qemu_allocate_irqs() here in the
> first place -- usually you'd just have a qemu_irq irqs[2] array
> in the device state struct, qemu_allocate_irq(irq[i], ...) and
> pass irq[i] to the ide_init2() function to tell it what
> qemu_irq to use. Or else you'd keep the pointer to the
> allocated irqs array in the device state struct, so as
> to be able to free it on any theoretical hot-unplug your
> device might support. Calling qemu_allocate_irqs() and then
> immediately freeing the array means that there are now
> two actual qemu_irqs floating around which are supposed
> to be owned by this device but which it has no way to
> get hold of any more. This is only not a leak because
> the cmd646 doesn't support hot-unplug.
> 
> (Hot take version : we should be getting rid of qemu_allocate_irqs()
> entirely: it is essentially a "pre-QOM" API and there are better
> ways to phrase code that's currently calling it.)

Yes, I agree, and it's why I didn't mind doing the quick fix that gets
rid of the boot-serial-test leak the easy way.

Paolo