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().
>
> Signed-off-by: Ashijeet Acharya <ashijeetacha...@gmail.com>
> ---
> 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(-)
>
> 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 = i;
> ad->port.dma = &ad->dma;
> ad->port.dma->ops = &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...
> }
> }
>
> 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)
>
> bmdma_init(&d->bus[i], &d->bmdma[i], d);
> d->bmdma[i].bus = &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 = idebus_unrealize;
Thanks,
Paolo
> }
>
> 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 running,
> RunState state)
> }
> }
>
> -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 = qemu_add_vm_change_state_handler(ide_restart_cb, bus);
> }
> }
>
> 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;
>
> 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);
> }
>
> ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
> 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) {
>
> bmdma_init(&d->bus[i], &d->bmdma[i], d);
> d->bmdma[i].bus = &d->bus[i];
> - ide_register_restart_cb(&d->bus[i]);
> + ide_register_restart_cb(&d->bus[i], d->vmstate);
> }
> }
>
> 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) {
>
> bmdma_init(&d->bus[i], &d->bmdma[i], d);
> d->bmdma[i].bus = &d->bus[i];
> - ide_register_restart_cb(&d->bus[i]);
> + ide_register_restart_cb(&d->bus[i], d->vmstate);
> }
> }
>
> 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;
>
> 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 iobase2);
> -void ide_register_restart_cb(IDEBus *bus);
> +void ide_register_restart_cb(IDEBus *bus, VMChangeStateEntry *e);
>
> void ide_exec_cmd(IDEBus *bus, uint32_t val);
>
> 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;
>
>
>
