| Message ID | 1393833475-10179-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 3 Mar 2014 15:57:55 +0800 Amos Kong <akong@redhat.com> wrote: s/subj/qdev: set properties after device's parent is assigned/ > 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. 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 <imammedo@redhat.com> > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > 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(-) > > 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) {
Am 03.03.2014 14:34, schrieb Igor Mammedov: > On Mon, 3 Mar 2014 15:57:55 +0800 > Amos Kong <akong@redhat.com> wrote: > > s/subj/qdev: set properties after device's parent is assigned/ > >> 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. > 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 <imammedo@redhat.com> > >> >> Signed-off-by: Amos Kong <akong@redhat.com> >> --- >> 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
On Mon, 03 Mar 2014 16:56:19 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 03.03.2014 14:34, schrieb Igor Mammedov: > > On Mon, 3 Mar 2014 15:57:55 +0800 > > Amos Kong <akong@redhat.com> wrote: > > > > s/subj/qdev: set properties after device's parent is assigned/ > > > >> 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. > > 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 <imammedo@redhat.com> > > > >> > >> Signed-off-by: Amos Kong <akong@redhat.com> > >> --- > >> 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 I'm ok with it. Thanks! > > Thanks, > Andreas >
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) {
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 <akong@redhat.com> --- 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(-)