[Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Ashijeet Acharya]

Hi,

I was working on a patch regarding a device lifecycle bitesize task
and I wanted to clear my queries about what the task is exactly.

Do I need to create a new function ahci_unrealize() in the
hw/ide/ahci.c file which calls for qemu_del_vm_change_state_handler()
to free the handler at the time of ahci hot unplug.

Please tell me if I am on the right path and correct me if I am wrong.

Thanks
Ashijeet

Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Ashijeet Acharya]

Hi again,
I am still waiting for some guidance...Can I please get some help with this?

Also.. I tried hotplugging an AHCI adapter but got the following error:
Bus 'ahci.0' does not support hotplugging

Steps I followed:

1. launch vm with ahci enabled
2. (qemu) drive_add 0 file=test.qcow2,cache=none,if=none,id=disk2,format=qcow2
OK
3. (qemu) device_add ide-hd,bus=ahci.0,id=ahci-disk,drive=disk2
Bus 'ahci.0' does not support hotplugging

What am I doing wrong?

Thanks
Ashijeet

On Sat, Aug 6, 2016 at 11:21 PM, Ashijeet Acharya
<ashijeetacharya@gmail.com> wrote:
> Hi,
>
> I was working on a patch regarding a device lifecycle bitesize task
> and I wanted to clear my queries about what the task is exactly.
>
> Do I need to create a new function ahci_unrealize() in the
> hw/ide/ahci.c file which calls for qemu_del_vm_change_state_handler()
> to free the handler at the time of ahci hot unplug.
>
> Please tell me if I am on the right path and correct me if I am wrong.
>
> Thanks
> Ashijeet

Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[John Snow]

On 08/09/2016 01:16 PM, Ashijeet Acharya wrote:
> Hi again,
> I am still waiting for some guidance...Can I please get some help with this?
>
> Also.. I tried hotplugging an AHCI adapter but got the following error:
> Bus 'ahci.0' does not support hotplugging
>
> Steps I followed:
>
> 1. launch vm with ahci enabled
> 2. (qemu) drive_add 0 file=test.qcow2,cache=none,if=none,id=disk2,format=qcow2
> OK
> 3. (qemu) device_add ide-hd,bus=ahci.0,id=ahci-disk,drive=disk2
> Bus 'ahci.0' does not support hotplugging
>
> What am I doing wrong?
>
> Thanks
> Ashijeet
>

I think you are confusing hotplugging the AHCI adapter with hotplugging 
an IDE drive into an AHCI adapter.

IDE/ATA/PATA (A rose by any other name...) drives do not support 
hotplugging (hence "Bus 'ahci.0' does not support hotplugging").

SATA drives do in theory, but it hasn't been implemented yet.

That's probably a little more involved than a Bite Sized Task, but it's 
something that I'd be willing to review if you embarked upon such a task.

> On Sat, Aug 6, 2016 at 11:21 PM, Ashijeet Acharya
> <ashijeetacharya@gmail.com> wrote:
>> Hi,
>>
>> I was working on a patch regarding a device lifecycle bitesize task
>> and I wanted to clear my queries about what the task is exactly.
>>
>> Do I need to create a new function ahci_unrealize() in the
>> hw/ide/ahci.c file which calls for qemu_del_vm_change_state_handler()
>> to free the handler at the time of ahci hot unplug.
>>

Sounds about right. grep around for other instances of 
qemu_del_vm_change_state_handler and try to code by example.

>> Please tell me if I am on the right path and correct me if I am wrong.
>>
>> Thanks
>> Ashijeet

-- 
—js

Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Ashijeet Acharya]

On Tue, Aug 9, 2016 at 11:48 PM, John Snow <jsnow@redhat.com> wrote:
>
>
> On 08/09/2016 01:16 PM, Ashijeet Acharya wrote:
>>
>> Hi again,
>> I am still waiting for some guidance...Can I please get some help with
>> this?
>>
>> Also.. I tried hotplugging an AHCI adapter but got the following error:
>> Bus 'ahci.0' does not support hotplugging
>>
>> Steps I followed:
>>
>> 1. launch vm with ahci enabled
>> 2. (qemu) drive_add 0
>> file=test.qcow2,cache=none,if=none,id=disk2,format=qcow2
>> OK
>> 3. (qemu) device_add ide-hd,bus=ahci.0,id=ahci-disk,drive=disk2
>> Bus 'ahci.0' does not support hotplugging
>>
>> What am I doing wrong?
>>
>> Thanks
>> Ashijeet
>>
>
> I think you are confusing hotplugging the AHCI adapter with hotplugging an
> IDE drive into an AHCI adapter.
>
> IDE/ATA/PATA (A rose by any other name...) drives do not support hotplugging
> (hence "Bus 'ahci.0' does not support hotplugging").
>
> SATA drives do in theory, but it hasn't been implemented yet.
>
> That's probably a little more involved than a Bite Sized Task, but it's
> something that I'd be willing to review if you embarked upon such a task.
>
>> On Sat, Aug 6, 2016 at 11:21 PM, Ashijeet Acharya
>> <ashijeetacharya@gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> I was working on a patch regarding a device lifecycle bitesize task
>>> and I wanted to clear my queries about what the task is exactly.
>>>
>>> Do I need to create a new function ahci_unrealize() in the
>>> hw/ide/ahci.c file which calls for qemu_del_vm_change_state_handler()
>>> to free the handler at the time of ahci hot unplug.
>>>
>
> Sounds about right. grep around for other instances of
> qemu_del_vm_change_state_handler and try to code by example.

Alright..thanks!

I found out that ahci_realize() calls for ide_register_restart_cb()
which has qemu_add_vm_change_state_handler() but the returned value
from qemu_add_vm_change_state_handler() does not get assigned to any
VMChangeStateEntry pointer. This should lead (I guess..) to a memory
leak! Maybe introducing a VMChangeStateEntry field in struct AHCIState
help and can be used here. This later can also be passed to
qemu_del_vm_change_state_handler() to free things up.
>
>
>>> Please tell me if I am on the right path and correct me if I am wrong.
>>>
>>> Thanks
>>> Ashijeet
>
>
> --
> —js

Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Ashijeet Acharya]

On Wed, Aug 10, 2016 at 11:12 AM, Ashijeet Acharya
<ashijeetacharya@gmail.com> wrote:
> On Tue, Aug 9, 2016 at 11:48 PM, John Snow <jsnow@redhat.com> wrote:
>>
>>
>> On 08/09/2016 01:16 PM, Ashijeet Acharya wrote:
>>>
>>> Hi again,
>>> I am still waiting for some guidance...Can I please get some help with
>>> this?
>>>
>>> Also.. I tried hotplugging an AHCI adapter but got the following error:
>>> Bus 'ahci.0' does not support hotplugging
>>>
>>> Steps I followed:
>>>
>>> 1. launch vm with ahci enabled
>>> 2. (qemu) drive_add 0
>>> file=test.qcow2,cache=none,if=none,id=disk2,format=qcow2
>>> OK
>>> 3. (qemu) device_add ide-hd,bus=ahci.0,id=ahci-disk,drive=disk2
>>> Bus 'ahci.0' does not support hotplugging
>>>
>>> What am I doing wrong?
>>>
>>> Thanks
>>> Ashijeet
>>>
>>
>> I think you are confusing hotplugging the AHCI adapter with hotplugging an
>> IDE drive into an AHCI adapter.
>>
>> IDE/ATA/PATA (A rose by any other name...) drives do not support hotplugging
>> (hence "Bus 'ahci.0' does not support hotplugging").
>>
>> SATA drives do in theory, but it hasn't been implemented yet.
>>
>> That's probably a little more involved than a Bite Sized Task, but it's
>> something that I'd be willing to review if you embarked upon such a task.

Sure... I can work upon that after I complete this bitesized task and
get more acquainted with this part of the code.

Ashijeet Acharya
>>
>>> On Sat, Aug 6, 2016 at 11:21 PM, Ashijeet Acharya
>>> <ashijeetacharya@gmail.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I was working on a patch regarding a device lifecycle bitesize task
>>>> and I wanted to clear my queries about what the task is exactly.
>>>>
>>>> Do I need to create a new function ahci_unrealize() in the
>>>> hw/ide/ahci.c file which calls for qemu_del_vm_change_state_handler()
>>>> to free the handler at the time of ahci hot unplug.
>>>>
>>
>> Sounds about right. grep around for other instances of
>> qemu_del_vm_change_state_handler and try to code by example.
>
> Alright..thanks!
>
> I found out that ahci_realize() calls for ide_register_restart_cb()
> which has qemu_add_vm_change_state_handler() but the returned value
> from qemu_add_vm_change_state_handler() does not get assigned to any
> VMChangeStateEntry pointer. This should lead (I guess..) to a memory
> leak! Maybe introducing a VMChangeStateEntry field in struct AHCIState
> help and can be used here. This later can also be passed to
> qemu_del_vm_change_state_handler() to free things up.
>>
>>
>>>> Please tell me if I am on the right path and correct me if I am wrong.
>>>>
>>>> Thanks
>>>> Ashijeet
>>
>>
>> --
>> —js

Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Ashijeet Acharya]

On Thu, Aug 11, 2016 at 1:50 PM, Ashijeet Acharya
<ashijeetacharya@gmail.com> wrote:
> On Wed, Aug 10, 2016 at 11:12 AM, Ashijeet Acharya
> <ashijeetacharya@gmail.com> wrote:
>> On Tue, Aug 9, 2016 at 11:48 PM, John Snow <jsnow@redhat.com> wrote:
>>>
>>>
>>> On 08/09/2016 01:16 PM, Ashijeet Acharya wrote:
>>>>
>>>> Hi again,
>>>> I am still waiting for some guidance...Can I please get some help with
>>>> this?
>>>>
>>>> Also.. I tried hotplugging an AHCI adapter but got the following error:
>>>> Bus 'ahci.0' does not support hotplugging
>>>>
>>>> Steps I followed:
>>>>
>>>> 1. launch vm with ahci enabled
>>>> 2. (qemu) drive_add 0
>>>> file=test.qcow2,cache=none,if=none,id=disk2,format=qcow2
>>>> OK
>>>> 3. (qemu) device_add ide-hd,bus=ahci.0,id=ahci-disk,drive=disk2
>>>> Bus 'ahci.0' does not support hotplugging
>>>>
>>>> What am I doing wrong?
>>>>
>>>> Thanks
>>>> Ashijeet
>>>>
>>>
>>> I think you are confusing hotplugging the AHCI adapter with hotplugging an
>>> IDE drive into an AHCI adapter.
>>>
>>> IDE/ATA/PATA (A rose by any other name...) drives do not support hotplugging
>>> (hence "Bus 'ahci.0' does not support hotplugging").
>>>
>>> SATA drives do in theory, but it hasn't been implemented yet.
>>>
>>> That's probably a little more involved than a Bite Sized Task, but it's
>>> something that I'd be willing to review if you embarked upon such a task.
>
> Sure... I can work upon that after I complete this bitesized task and
> get more acquainted with this part of the code.
>
> Ashijeet Acharya
>>>
>>>> On Sat, Aug 6, 2016 at 11:21 PM, Ashijeet Acharya
>>>> <ashijeetacharya@gmail.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I was working on a patch regarding a device lifecycle bitesize task
>>>>> and I wanted to clear my queries about what the task is exactly.
>>>>>
>>>>> Do I need to create a new function ahci_unrealize() in the
>>>>> hw/ide/ahci.c file which calls for qemu_del_vm_change_state_handler()
>>>>> to free the handler at the time of ahci hot unplug.
>>>>>
>>>
>>> Sounds about right. grep around for other instances of
>>> qemu_del_vm_change_state_handler and try to code by example.
>>
>> Alright..thanks!
>>
>> I found out that ahci_realize() calls for ide_register_restart_cb()
>> which has qemu_add_vm_change_state_handler() but the returned value
>> from qemu_add_vm_change_state_handler() does not get assigned to any
>> VMChangeStateEntry pointer. This should lead (I guess..) to a memory
>> leak! Maybe introducing a VMChangeStateEntry field in struct AHCIState
>> help and can be used here. This later can also be passed to
>> qemu_del_vm_change_state_handler() to free things up.
>>>

I think we should do

s->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus);
instead of
qemu_add_vm_change_state_handler(ide_restart_cb, bus);

in ide_register_restart_cb() in hw/ide/core.c to store the returned
pointer to memory to avoid a possible memory leak I guess and
introduce a VMChangeStateEntry field in struct AHCIState to handle
this. Same can then further be used with
qemu_del_vm_change_state_handler() in ahci_unrealize() to free things
up.

Ashijeet

>>>
>>>>> Please tell me if I am on the right path and correct me if I am wrong.
>>>>>
>>>>> Thanks
>>>>> Ashijeet
>>>
>>>
>>> --
>>> —js

Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Paolo Bonzini]

> I think we should do
> 
> s->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus);
> instead of
> qemu_add_vm_change_state_handler(ide_restart_cb, bus);
> 
> in ide_register_restart_cb() in hw/ide/core.c to store the returned
> pointer to memory to avoid a possible memory leak I guess and
> introduce a VMChangeStateEntry field in struct AHCIState to handle
> this. Same can then further be used with
> qemu_del_vm_change_state_handler() in ahci_unrealize() to free things
> up.

Yes, this is correct. Thanks!

Paolo

Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Ashijeet Acharya]

On Thu, Aug 11, 2016 at 10:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> I think we should do
>>
>> s->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus);
>> instead of
>> qemu_add_vm_change_state_handler(ide_restart_cb, bus);
>>
>> in ide_register_restart_cb() in hw/ide/core.c to store the returned
>> pointer to memory to avoid a possible memory leak I guess and
>> introduce a VMChangeStateEntry field in struct AHCIState to handle
>> this. Same can then further be used with
>> qemu_del_vm_change_state_handler() in ahci_unrealize() to free things
>> up.
>
> Yes, this is correct. Thanks!
>
Alright I will make these changes and include them in my patch.

Ashijeet
> Paolo

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

[Ashijeet Acharya]

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 <ashijeetacharya@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);
     }
 }
 
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);
     }
 
     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;
 
 
-- 
2.6.2

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

[Paolo Bonzini]

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 <ashijeetacharya@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;
>  
>  
> 

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

[Ashijeet Acharya]

Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 hw/ide/core.c             |  2 +-
 hw/ide/qdev.c             | 14 ++++++++++++++
 include/hw/ide/internal.h |  1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 45b6df1..eecbb47 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state)
 void ide_register_restart_cb(IDEBus *bus)
 {
     if (bus->dma->ops->restart_dma) {
-        qemu_add_vm_change_state_handler(ide_restart_cb, bus);
+        bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus);
     }
 }
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 67c76bf..6f75f77 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -31,6 +31,7 @@
 /* --------------------------------- */
 
 static char *idebus_get_fw_dev_path(DeviceState *dev);
+static void idebus_unrealize(DeviceState *qdev, Error **errp);
 
 static Property ide_props[] = {
     DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
@@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data)
     k->init = ide_qdev_init;
     set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
     k->bus_type = TYPE_IDE_BUS;
+    k->unrealize = idebus_unrealize;
     k->props = ide_props;
 }
 
@@ -368,3 +370,15 @@ static void ide_register_types(void)
 }
 
 type_init(ide_register_types)
+
+static void idebus_unrealize(DeviceState *qdev, Error **errp)
+{
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
+
+    if (bus->dma->ops->restart_dma) {
+        if (bus->vmstate) {
+            qemu_del_vm_change_state_handler(bus->vmstate);
+        }
+    }
+}

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 7824bc3..2103261 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -480,6 +480,7 @@ struct IDEBus {
     uint8_t retry_unit;
     int64_t retry_sector_num;
     uint32_t retry_nsector;
+    VMChangeStateEntry *vmstate;
 };
 
 #define TYPE_IDE_DEVICE "ide-device"
-- 
2.6.2

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

[John Snow]

Can you please resend this not as a reply to this discussion thread?

http://wiki.qemu.org/Contribute/SubmitAPatch

Thank you,
--js

On 08/16/2016 09:55 AM, Ashijeet Acharya wrote:
> Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  hw/ide/core.c             |  2 +-
>  hw/ide/qdev.c             | 14 ++++++++++++++
>  include/hw/ide/internal.h |  1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 45b6df1..eecbb47 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state)
>  void ide_register_restart_cb(IDEBus *bus)
>  {
>      if (bus->dma->ops->restart_dma) {
> -        qemu_add_vm_change_state_handler(ide_restart_cb, bus);
> +        bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus);
>      }
>  }
>
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 67c76bf..6f75f77 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -31,6 +31,7 @@
>  /* --------------------------------- */
>
>  static char *idebus_get_fw_dev_path(DeviceState *dev);
> +static void idebus_unrealize(DeviceState *qdev, Error **errp);
>
>  static Property ide_props[] = {
>      DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
> @@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data)
>      k->init = ide_qdev_init;
>      set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
>      k->bus_type = TYPE_IDE_BUS;
> +    k->unrealize = idebus_unrealize;
>      k->props = ide_props;
>  }
>
> @@ -368,3 +370,15 @@ static void ide_register_types(void)
>  }
>
>  type_init(ide_register_types)
> +
> +static void idebus_unrealize(DeviceState *qdev, Error **errp)
> +{
> +    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
> +
> +    if (bus->dma->ops->restart_dma) {
> +        if (bus->vmstate) {
> +            qemu_del_vm_change_state_handler(bus->vmstate);
> +        }
> +    }
> +}
>
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 7824bc3..2103261 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -480,6 +480,7 @@ struct IDEBus {
>      uint8_t retry_unit;
>      int64_t retry_sector_num;
>      uint32_t retry_nsector;
> +    VMChangeStateEntry *vmstate;
>  };
>
>  #define TYPE_IDE_DEVICE "ide-device"
>