diff mbox

[08/13] qdev: device free fixups.

Message ID 1253611767-6483-9-git-send-email-kraxel@redhat.com
State Superseded
Headers show

Commit Message

Gerd Hoffmann Sept. 22, 2009, 9:29 a.m. UTC 
Two bug fixes:
 * When freeing a device we unregister stuff unconditionally,
   even in case we didn't register in the first place because
   the ->init() callback failed.
 * When freeing a device with child busses attached we don't
   zap the child bus (and the devices attached to it).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   35 +++++++++++++++++++++++++++++++----
 hw/qdev.h |    7 +++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

Comments

Markus Armbruster Sept. 24, 2009, 6:55 p.m. UTC | #1 
Gerd Hoffmann <kraxel@redhat.com> writes:

> Two bug fixes:
>  * When freeing a device we unregister stuff unconditionally,
>    even in case we didn't register in the first place because
>    the ->init() callback failed.
>  * When freeing a device with child busses attached we don't
>    zap the child bus (and the devices attached to it).
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Maybe I'm just having a particularly dense day, but I misread this as
"this is what the fix does".  In reality, it's "this is what we did
wrong".  Careless readers like me can be kept on track by emphasizing
the "wrongness" of what's being done, so that it's (more) obvious we're
talking about the bug and not the fix:

  * When freeing a device we unregister even stuff we didn't register in
    the first place because the ->init() callback failed.
  * When freeing a device with child busses attached, we fail to zap the
    child bus (and the devices attached to it).

Another language trick is to use past tense for the bug and present
tense for the fix.
diff mbox

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 43372c1..4931da1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -102,6 +102,7 @@  DeviceState *qdev_create(BusState *bus, const char *name)
     qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
     qdev_prop_set_compat(dev);
     QLIST_INSERT_HEAD(&bus->children, dev, sibling);
+    dev->state = DEV_STATE_CREATED;
     return dev;
 }
 
@@ -216,6 +217,7 @@  int qdev_init(DeviceState *dev)
 {
     int rc;
 
+    assert(dev->state == DEV_STATE_CREATED);
     rc = dev->info->init(dev, dev->info);
     if (rc < 0)
         return rc;
@@ -223,18 +225,27 @@  int qdev_init(DeviceState *dev)
         qemu_register_reset(dev->info->reset, dev);
     if (dev->info->vmsd)
         vmstate_register(-1, dev->info->vmsd, dev);
+    dev->state = DEV_STATE_INITIALIZED;
     return 0;
 }
 
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
+    BusState *bus;
+
+    if (dev->state == DEV_STATE_INITIALIZED) {
+        while (dev->num_child_bus) {
+            bus = QLIST_FIRST(&dev->child_bus);
+            qbus_free(bus);
+        }
 #if 0 /* FIXME: need sane vmstate_unregister function */
-    if (dev->info->vmsd)
-        vmstate_unregister(dev->info->vmsd, dev);
+        if (dev->info->vmsd)
+            vmstate_unregister(dev->info->vmsd, dev);
 #endif
-    if (dev->info->reset)
-        qemu_unregister_reset(dev->info->reset, dev);
+        if (dev->info->reset)
+            qemu_unregister_reset(dev->info->reset, dev);
+    }
     QLIST_REMOVE(dev, sibling);
     qemu_free(dev);
 }
@@ -549,6 +560,22 @@  BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name)
     return bus;
 }
 
+void qbus_free(BusState *bus)
+{
+    DeviceState *dev;
+
+    while ((dev = QLIST_FIRST(&bus->children)) != NULL) {
+        qdev_free(dev);
+    }
+    if (bus->parent) {
+        QLIST_REMOVE(bus, sibling);
+        bus->parent->num_child_bus--;
+    }
+    if (bus->qdev_allocated) {
+        qemu_free(bus);
+    }
+}
+
 #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__)
 static void qbus_print(Monitor *mon, BusState *bus, int indent);
 
diff --git a/hw/qdev.h b/hw/qdev.h
index ccc45b9..c036aff 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -19,10 +19,16 @@  typedef struct BusState BusState;
 
 typedef struct BusInfo BusInfo;
 
+enum DevState {
+    DEV_STATE_CREATED = 1,
+    DEV_STATE_INITIALIZED,
+};
+
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
 struct DeviceState {
     const char *id;
+    enum DevState state;
     DeviceInfo *info;
     BusState *parent_bus;
     int num_gpio_out;
@@ -148,6 +154,7 @@  BusState *qdev_get_parent_bus(DeviceState *dev);
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
+void qbus_free(BusState *bus);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)