| Message ID | 1569936988-635-2-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix various memory leaks (but not all) | expand |
On 01/10/2019 15.36, Paolo Bonzini 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); > Maybe you could also update the description of qemu_allocate_irqs() to state that the returned pointer should be g_free'd later? Reviewed-by: Thomas Huth <thuth@redhat.com>
On 10/1/19 8:36 AM, Paolo Bonzini 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); How many of these patches could be fixed by using g_autofree or similar instead?
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.) thanks -- PMM
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
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);
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(+)