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

2016-09-27 Thread Ashijeet Acharya 
On Tue, Sep 27, 2016 at 10:08 PM, John Snow  wrote:
>
>
> On 09/22/2016 01:47 AM, Ashijeet Acharya wrote:
>>
>> On Thu, Sep 22, 2016 at 1:08 AM, John Snow  wrote:
>>>
>>>
>>>
>>> On 09/21/2016 01:53 PM, 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.

>>>
>>> Minor rebase issue, but it's trivially resolved.
>>>
>>>
 Signed-off-by: Ashijeet Acharya 
 ---
 Changes in v2:
 -v1 was corrupted at line 64
 -Move idebus_unrealize() below ide_bus_class_init()
 ---
  hw/ide/core.c |  2 +-
  hw/ide/qdev.c | 13 +
  include/hw/ide/internal.h |  1 +
  3 files changed, 15 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 2eb055a..c94f9f8 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),
 @@ -44,6 +45,17 @@ static void ide_bus_class_init(ObjectClass *klass,
 void
 *data)
  k->get_fw_dev_path = idebus_get_fw_dev_path;
  }

 +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);
 +}
 +}
 +}
 +
>>>
>>>
>>>
>>> Naive question: I saw Paolo say that this should be conditional on
>>> bus->dma->ops->restart_dma -- Why can't we just say if (bus->vmstate) ?
>>>
>>> I see that we only allocate the change state handler when restart_dma is
>>> present, but this makes this portion of the code look funny when if
>>> (bus->vmstate) would be just as simple, no?
>>>
>>
>> I had a similar thought, because other pieces of code also do it in
>> "if (bus->vmstate)" manner but maybe I was missing on something
>> important and ended up coding how Paolo instructed me.
>>
>
> I think we can use the smaller conditional -- less prone to error that way
> in case we change the conditional on how it was constructed. Let's do that
> and I'll merge this for you finally.

Sure, I will send v3 ASAP.

Ashijeet
>
>
>>> (Unless we can't rely on its NULL initialization or some such.)
>>
>>
>> Maybe, but shouldn't the same logic apply elsewhere then?
>>
>>>
  static const TypeInfo ide_bus_info = {
  .name = TYPE_IDE_BUS,
  .parent = TYPE_BUS,
 @@ -355,6 +367,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;
  }

 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;
>>>
>>>
>>>
>>> Hmm, I was going to complain and say that 'vmstate' is a generic name for
>>> this field, but it's by far the most common name for this task. I cede!
>>>
  };

  #define TYPE_IDE_DEVICE "ide-device"

>>>
>>> Seems good otherwise, thank you!
>>
>> Thanks!
>>
>> Ashijeet
>>
>

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

2016-09-27 Thread John Snow 



On 09/22/2016 01:47 AM, Ashijeet Acharya wrote:

On Thu, Sep 22, 2016 at 1:08 AM, John Snow  wrote:



On 09/21/2016 01:53 PM, 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.



Minor rebase issue, but it's trivially resolved.



Signed-off-by: Ashijeet Acharya 
---
Changes in v2:
-v1 was corrupted at line 64
-Move idebus_unrealize() below ide_bus_class_init()
---
 hw/ide/core.c |  2 +-
 hw/ide/qdev.c | 13 +
 include/hw/ide/internal.h |  1 +
 3 files changed, 15 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 2eb055a..c94f9f8 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),
@@ -44,6 +45,17 @@ static void ide_bus_class_init(ObjectClass *klass, void
*data)
 k->get_fw_dev_path = idebus_get_fw_dev_path;
 }

+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);
+}
+}
+}
+



Naive question: I saw Paolo say that this should be conditional on
bus->dma->ops->restart_dma -- Why can't we just say if (bus->vmstate) ?

I see that we only allocate the change state handler when restart_dma is
present, but this makes this portion of the code look funny when if
(bus->vmstate) would be just as simple, no?



I had a similar thought, because other pieces of code also do it in
"if (bus->vmstate)" manner but maybe I was missing on something
important and ended up coding how Paolo instructed me.



I think we can use the smaller conditional -- less prone to error that 
way in case we change the conditional on how it was constructed. Let's 
do that and I'll merge this for you finally.



(Unless we can't rely on its NULL initialization or some such.)


Maybe, but shouldn't the same logic apply elsewhere then?




 static const TypeInfo ide_bus_info = {
 .name = TYPE_IDE_BUS,
 .parent = TYPE_BUS,
@@ -355,6 +367,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;
 }

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;



Hmm, I was going to complain and say that 'vmstate' is a generic name for
this field, but it's by far the most common name for this task. I cede!


 };

 #define TYPE_IDE_DEVICE "ide-device"



Seems good otherwise, thank you!

Thanks!

Ashijeet

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

2016-09-21 Thread Ashijeet Acharya 
On Thu, Sep 22, 2016 at 1:08 AM, John Snow  wrote:
>
>
> On 09/21/2016 01:53 PM, 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.
>>
>
> Minor rebase issue, but it's trivially resolved.
>
>
>> Signed-off-by: Ashijeet Acharya 
>> ---
>> Changes in v2:
>> -v1 was corrupted at line 64
>> -Move idebus_unrealize() below ide_bus_class_init()
>> ---
>>  hw/ide/core.c |  2 +-
>>  hw/ide/qdev.c | 13 +
>>  include/hw/ide/internal.h |  1 +
>>  3 files changed, 15 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 2eb055a..c94f9f8 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),
>> @@ -44,6 +45,17 @@ static void ide_bus_class_init(ObjectClass *klass, void
>> *data)
>>  k->get_fw_dev_path = idebus_get_fw_dev_path;
>>  }
>>
>> +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);
>> +}
>> +}
>> +}
>> +
>
>
> Naive question: I saw Paolo say that this should be conditional on
> bus->dma->ops->restart_dma -- Why can't we just say if (bus->vmstate) ?
>
> I see that we only allocate the change state handler when restart_dma is
> present, but this makes this portion of the code look funny when if
> (bus->vmstate) would be just as simple, no?
>

I had a similar thought, because other pieces of code also do it in
"if (bus->vmstate)" manner but maybe I was missing on something
important and ended up coding how Paolo instructed me.

> (Unless we can't rely on its NULL initialization or some such.)

Maybe, but shouldn't the same logic apply elsewhere then?

>
>>  static const TypeInfo ide_bus_info = {
>>  .name = TYPE_IDE_BUS,
>>  .parent = TYPE_BUS,
>> @@ -355,6 +367,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;
>>  }
>>
>> 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;
>
>
> Hmm, I was going to complain and say that 'vmstate' is a generic name for
> this field, but it's by far the most common name for this task. I cede!
>
>>  };
>>
>>  #define TYPE_IDE_DEVICE "ide-device"
>>
>
> Seems good otherwise, thank you!
Thanks!

Ashijeet

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

2016-09-21 Thread John Snow 



On 09/21/2016 01:53 PM, 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.



Minor rebase issue, but it's trivially resolved.


Signed-off-by: Ashijeet Acharya 
---
Changes in v2:
-v1 was corrupted at line 64
-Move idebus_unrealize() below ide_bus_class_init()
---
 hw/ide/core.c |  2 +-
 hw/ide/qdev.c | 13 +
 include/hw/ide/internal.h |  1 +
 3 files changed, 15 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 2eb055a..c94f9f8 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),
@@ -44,6 +45,17 @@ static void ide_bus_class_init(ObjectClass *klass, void 
*data)
 k->get_fw_dev_path = idebus_get_fw_dev_path;
 }

+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);
+}
+}
+}
+


Naive question: I saw Paolo say that this should be conditional on 
bus->dma->ops->restart_dma -- Why can't we just say if (bus->vmstate) ?


I see that we only allocate the change state handler when restart_dma is 
present, but this makes this portion of the code look funny when if 
(bus->vmstate) would be just as simple, no?


(Unless we can't rely on its NULL initialization or some such.)


 static const TypeInfo ide_bus_info = {
 .name = TYPE_IDE_BUS,
 .parent = TYPE_BUS,
@@ -355,6 +367,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;
 }

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;


Hmm, I was going to complain and say that 'vmstate' is a generic name 
for this field, but it's by far the most common name for this task. I cede!



 };

 #define TYPE_IDE_DEVICE "ide-device"



Seems good otherwise, thank you!