From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bYDjB-0000Q5-US for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:46:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bYDj6-0003pt-TB for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:46:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57604) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bYDj6-0003pm-KK for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:46:24 -0400 References: <1470941158-7043-1-git-send-email-ashijeetacharya@gmail.com> From: Paolo Bonzini Message-ID: <9dbfdcae-5c86-cbd9-7d54-261ac1a61e76@redhat.com> Date: Fri, 12 Aug 2016 11:58:30 +0200 MIME-Version: 1.0 In-Reply-To: <1470941158-7043-1-git-send-email-ashijeetacharya@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ashijeet Acharya , jsnow@redhat.com Cc: qemu-devel@nongnu.org On 11/08/2016 20:45, Ashijeet Acharya wrote: > Introduce VMChangeStateEntry parameter in ide_register_restart_cb() to = handle possible memory leak from qemu_add_vm_change_state_handler(). >=20 > Signed-off-by: Ashijeet Acharya > --- > hw/ide/ahci.c | 2 +- > hw/ide/cmd646.c | 2 +- > hw/ide/core.c | 4 ++-- > hw/ide/isa.c | 3 ++- > hw/ide/piix.c | 2 +- > hw/ide/via.c | 2 +- > include/hw/ide/ahci.h | 1 + > include/hw/ide/internal.h | 2 +- > include/hw/ide/pci.h | 1 + > 9 files changed, 11 insertions(+), 8 deletions(-) >=20 > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index bcb9ff9..d7c96df 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1476,7 +1476,7 @@ void ahci_realize(AHCIState *s, DeviceState *qdev= , AddressSpace *as, int ports) > ad->port_no =3D i; > ad->port.dma =3D &ad->dma; > ad->port.dma->ops =3D &ahci_dma_ops; > - ide_register_restart_cb(&ad->port); > + ide_register_restart_cb(&ad->port, s->vmstate); You need more than one vmstate handler here... > } > } > =20 > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index 9ebb8d4..b906aa7 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -369,7 +369,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, = Error **errp) > =20 > bmdma_init(&d->bus[i], &d->bmdma[i], d); > d->bmdma[i].bus =3D &d->bus[i]; > - ide_register_restart_cb(&d->bus[i]); > + ide_register_restart_cb(&d->bus[i], d->vmstate); ... and here. It's probably simplest to add the field to IDEBus. You're also missing the calls to qemu_del_vm_change_state_handler. They should also be conditional on bus->dma->ops->restart_dma. I suggest creating an idebus_unrealize function to do so, and registering it in hw/ide/qdev.c with k->unrealize =3D idebus_unrealize; Thanks, Paolo > } > =20 > vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d); > diff --git a/hw/ide/core.c b/hw/ide/core.c > index d117b7c..0a27d4d 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -2578,10 +2578,10 @@ static void ide_restart_cb(void *opaque, int ru= nning, RunState state) > } > } > =20 > -void ide_register_restart_cb(IDEBus *bus) > +void ide_register_restart_cb(IDEBus *bus, VMChangeStateEntry *e) > { > if (bus->dma->ops->restart_dma) { > - qemu_add_vm_change_state_handler(ide_restart_cb, bus); > + e =3D qemu_add_vm_change_state_handler(ide_restart_cb, bus); > } > } > =20 > diff --git a/hw/ide/isa.c b/hw/ide/isa.c > index 40213d6..74a5078 100644 > --- a/hw/ide/isa.c > +++ b/hw/ide/isa.c > @@ -45,6 +45,7 @@ typedef struct ISAIDEState { > uint32_t iobase2; > uint32_t isairq; > qemu_irq irq; > + VMChangeStateEntry *vmstate; > } ISAIDEState; > =20 > static void isa_ide_reset(DeviceState *d) > @@ -75,7 +76,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error= **errp) > isa_init_irq(isadev, &s->irq, s->isairq); > ide_init2(&s->bus, s->irq); > vmstate_register(dev, 0, &vmstate_ide_isa, s); > - ide_register_restart_cb(&s->bus); > + ide_register_restart_cb(&s->bus, s->vmstate); > } > =20 > ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isai= rq, > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index c190fca..2c67508 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -144,7 +144,7 @@ static void pci_piix_init_ports(PCIIDEState *d) { > =20 > bmdma_init(&d->bus[i], &d->bmdma[i], d); > d->bmdma[i].bus =3D &d->bus[i]; > - ide_register_restart_cb(&d->bus[i]); > + ide_register_restart_cb(&d->bus[i], d->vmstate); > } > } > =20 > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 5b32ecb..82fbbf0 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -167,7 +167,7 @@ static void vt82c686b_init_ports(PCIIDEState *d) { > =20 > bmdma_init(&d->bus[i], &d->bmdma[i], d); > d->bmdma[i].bus =3D &d->bus[i]; > - ide_register_restart_cb(&d->bus[i]); > + ide_register_restart_cb(&d->bus[i], d->vmstate); > } > } > =20 > diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h > index 0ca7c65..fa4a680 100644 > --- a/include/hw/ide/ahci.h > +++ b/include/hw/ide/ahci.h > @@ -298,6 +298,7 @@ typedef struct AHCIState { > int32_t ports; > qemu_irq irq; > AddressSpace *as; > + VMChangeStateEntry *vmstate; > } AHCIState; > =20 > typedef struct AHCIPCIState { > diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h > index 7824bc3..a95e6e9 100644 > --- a/include/hw/ide/internal.h > +++ b/include/hw/ide/internal.h > @@ -605,7 +605,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, = IDEDriveKind kind, > int chs_trans); > void ide_init2(IDEBus *bus, qemu_irq irq); > void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int ioba= se2); > -void ide_register_restart_cb(IDEBus *bus); > +void ide_register_restart_cb(IDEBus *bus, VMChangeStateEntry *e); > =20 > void ide_exec_cmd(IDEBus *bus, uint32_t val); > =20 > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index dbc6a03..95df1c0 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -57,6 +57,7 @@ typedef struct PCIIDEState { > uint32_t secondary; /* used only for cmd646 */ > MemoryRegion bmdma_bar; > CMD646BAR cmd646_bar[2]; /* used only for cmd646 */ > + VMChangeStateEntry *vmstate; > } PCIIDEState; > =20 > =20 >=20