qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb()


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb()
Date: Fri, 12 Aug 2016 11:58:30 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0


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 <address@hidden>
> ---
>  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;
>  
>  
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]