On 09/22/2016 01:47 AM, Ashijeet Acharya wrote:
On Thu, Sep 22, 2016 at 1:08 AM, John Snow <js...@redhat.com> 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 <ashijeetacha...@gmail.com>
---
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



Reply via email to