On 01/10/19 17:58, Peter Maydell wrote: > On Tue, 1 Oct 2019 at 14:38, Paolo Bonzini <pbonz...@redhat.com> wrote: >> >> The array returned by qemu_allocate_irqs is malloced, free it. >> >> Signed-off-by: Paolo Bonzini <pbonz...@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
