qdev: move the code adding the device out of realize

PATCH In review Amos Kong 3 participants 4 messages Mar 3, 2014

Share & tools

Amos Kong

author

2014-03-03 07:59 UTC #0 lore raw

PATCH v3 1/1 qdev: move the code adding the device out of realize

Test steps:
(qemu) device_add e1000,addr=adsf
Property 'e1000.addr' doesn't take value 'adsf'
(qemu) info qtree
Then qemu crashed.

Currently we set a link to the new device for qdev parent bus, but the device hasn't been added to QOM tree. When it fails to set properties, object_unparent() can't cleanup the device.

This patch moves the code adding the device output realize, when it fails to set properties, the device can be cleaned successfully.

Signed-off-by Amos Kong <[email protected]>

---
V2: fix bz by adjust the initialization order (Paolo)
V3: fix bug without making it differs with legacy devices
creation (Andreas)

---
 qdev-monitor.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

qdev-monitor.c +9 -6

index 6673e3c..9268c87 100644
@@ -522,7 +522,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         return NULL;
     }
 
-    /* create device, set properties */
+    /* create device */
     dev = DEVICE(object_new(driver));
 
     if (bus) {

@@ -533,11 +533,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     if (id) {
         dev->id = id;
     }
-    if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
-        object_unparent(OBJECT(dev));
-        object_unref(OBJECT(dev));
-        return NULL;
-    }
+
     if (dev->id) {
         object_property_add_child(qdev_get_peripheral(), dev->id,
                                   OBJECT(dev), NULL);

@@ -549,6 +545,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         g_free(name);
     }
 
+    /* set properties */
+    if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
+        object_unparent(OBJECT(dev));
+        object_unref(OBJECT(dev));
+        return NULL;
+    }
+
     dev->opts = opts;
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {

Signature · 1 line

1.8.5.3

Igor Mammedov

Reviewed-by on v3 1/1

2014-03-03 13:35 UTC #1 lore raw

On Mon, 3 Mar 2014 15:57:55 +0800
Amos Kong <[email protected]> wrote:

s/subj/qdev: set properties after device's parent is assigned/

Quoted 6 lines

Test steps:
(qemu) device_add e1000,addr=adsf
Property 'e1000.addr' doesn't take value 'adsf'
(qemu) info qtree
Then qemu crashed.

Quoted 6 lines

Currently we set a link to the new device for qdev parent bus, but the device hasn't been added to QOM tree. When it fails to set properties, object_unparent() can't cleanup the device.
This patch moves the code adding the device output realize, when it fails to set properties, the device can be cleaned successfully.

Suggest rephrase as:
Delay device property setting until device's parent is assigned. This way when property setting fails, object_unparent() can cleanup failed device properly.

with above correction:

Reviewed-by Igor Mammedov <[email protected]>

Quoted 50 lines

Signed-off-by Amos Kong <[email protected]>

---
V2: fix bz by adjust the initialization order (Paolo)
V3: fix bug without making it differs with legacy devices
creation (Andreas)

---
 qdev-monitor.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

qdev-monitor.c +9 -6

index 6673e3c..9268c87 100644
@@ -522,7 +522,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         return NULL;
     }
 
-    /* create device, set properties */
+    /* create device */
     dev = DEVICE(object_new(driver));
 
     if (bus) {

@@ -533,11 +533,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     if (id) {
         dev->id = id;
     }
-    if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
-        object_unparent(OBJECT(dev));
-        object_unref(OBJECT(dev));
-        return NULL;
-    }
+
     if (dev->id) {
         object_property_add_child(qdev_get_peripheral(), dev->id,
                                   OBJECT(dev), NULL);

@@ -549,6 +545,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         g_free(name);
     }
 
+    /* set properties */
+    if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
+        object_unparent(OBJECT(dev));
+        object_unref(OBJECT(dev));
+        return NULL;
+    }
+
     dev->opts = opts;
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {

Andreas Färber

2014-03-03 15:56 UTC #2 lore raw

Am 03.03.2014 14:34, schrieb Igor Mammedov:

Quoted 17 lines

On Mon, 3 Mar 2014 15:57:55 +0800
Amos Kong <[email protected]> wrote:
s/subj/qdev: set properties after device's parent is assigned/
Quoted 6 lines
Test steps:
(qemu) device_add e1000,addr=adsf
Property 'e1000.addr' doesn't take value 'adsf'
(qemu) info qtree
Then qemu crashed.
Quoted 6 lines
Currently we set a link to the new device for qdev parent bus, but the device hasn't been added to QOM tree. When it fails to set properties, object_unparent() can't cleanup the device.
This patch moves the code adding the device output realize, when it fails to set properties, the device can be cleaned successfully.

Quoted 14 lines

Suggest rephrase as:
Delay device property setting until device's parent is assigned. This way when property setting fails, object_unparent() can cleanup failed device properly.
with above correction:
Reviewed-by Igor Mammedov <[email protected]>
Quoted 6 lines
Signed-off-by Amos Kong <[email protected]>
---
V2: fix bz by adjust the initialization order (Paolo)
V3: fix bug without making it differs with legacy devices
creation (Andreas)

Perfect, I've tweaked the commit message along the lines of Igor's suggestion - please take a look if you want it changed differently:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Thanks,
Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Igor Mammedov

2014-03-03 17:25 UTC #3 lore raw

Quoted 33 lines · On Mon, 03 Mar 2014 16:56:19 +0100 Andreas Färber <[email protected]> wrote:

Am 03.03.2014 14:34, schrieb Igor Mammedov:
Quoted 17 lines
On Mon, 3 Mar 2014 15:57:55 +0800
Amos Kong <[email protected]> wrote:
s/subj/qdev: set properties after device's parent is assigned/
Quoted 6 lines
Test steps:
(qemu) device_add e1000,addr=adsf
Property 'e1000.addr' doesn't take value 'adsf'
(qemu) info qtree
Then qemu crashed.
Quoted 6 lines
Currently we set a link to the new device for qdev parent bus, but the device hasn't been added to QOM tree. When it fails to set properties, object_unparent() can't cleanup the device.
This patch moves the code adding the device output realize, when it fails to set properties, the device can be cleaned successfully.
Quoted 14 lines
Suggest rephrase as:
Delay device property setting until device's parent is assigned. This way when property setting fails, object_unparent() can cleanup failed device properly.
with above correction:
Reviewed-by Igor Mammedov <[email protected]>
Quoted 6 lines
Signed-off-by Amos Kong <[email protected]>
---
V2: fix bz by adjust the initialization order (Paolo)
V3: fix bug without making it differs with legacy devices
creation (Andreas)

Quoted 3 lines

Perfect, I've tweaked the commit message along the lines of Igor's suggestion - please take a look if you want it changed differently:
https://github.com/afaerber/qemu-cpu/commits/qom-next

I'm ok with it.
Thanks!

Quoted 4 lines

Thanks,
Andreas