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