| Message ID | 1393862124-26806-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, Am 03.03.2014 16:55, schrieb Amos Kong: > 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. > > Delay device property setting until device's parent is assigned. This > way when property setting fails, object_unparent() can cleanup failed > device properly. > > Signed-off-by: Amos Kong <akong@redhat.com> > Reviewed-By: Igor Mammedov <imammedo@redhat.com> > --- > V2: fix bz by adjust the initialization order (Paolo) > V3: fix bug without making it differs with legacy devices > creation (Andreas) > V4: update subject and commitlog I already applied a variation of v3. In particular I used qdev-monitor for consistency and clarified that it is about device_add. https://github.com/afaerber/qemu-cpu/commits/qom-next If you don't like something in there, can you please just suggest an alternative sentence/paragraph for me to update? Thanks, Andreas
On Mon, Mar 03, 2014 at 05:01:34PM +0100, Andreas Färber wrote: > Hi, > > Am 03.03.2014 16:55, schrieb Amos Kong: > > 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. > > > > Delay device property setting until device's parent is assigned. This > > way when property setting fails, object_unparent() can cleanup failed > > device properly. > > > > Signed-off-by: Amos Kong <akong@redhat.com> > > Reviewed-By: Igor Mammedov <imammedo@redhat.com> > > --- > > V2: fix bz by adjust the initialization order (Paolo) > > V3: fix bug without making it differs with legacy devices > > creation (Andreas) > > V4: update subject and commitlog > > I already applied a variation of v3. In particular I used qdev-monitor > for consistency and clarified that it is about device_add. > > https://github.com/afaerber/qemu-cpu/commits/qom-next Your change's fine. I saw your comment on V3 thread after sent out V4 :-) Thanks. > If you don't like something in there, can you please just suggest an > alternative sentence/paragraph for me to update? > > Thanks, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/qdev-monitor.c b/qdev-monitor.c index 6673e3c..9268c87 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -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) {