| Message ID | 1388624531-13439-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 02/01/2014 02:02, Amos Kong ha scritto: > 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 qdev_set_parent_bus() back to object_property_add_child(), > we only needs to unref the object if setting properties fails. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > V2: fix bz by adjust the initialization order (Paolo) > --- > qdev-monitor.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/qdev-monitor.c b/qdev-monitor.c > index dc37a43..4070b0a 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -518,16 +518,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > /* create device, set properties */ > dev = DEVICE(object_new(driver)); > > - if (bus) { > - qdev_set_parent_bus(dev, bus); > - } > - > id = qemu_opts_id(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; > } > @@ -541,6 +536,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > OBJECT(dev), NULL); > g_free(name); > } > + > + if (bus) { > + qdev_set_parent_bus(dev, bus); > + } > + > object_property_set_bool(OBJECT(dev), true, "realized", &err); > if (err != NULL) { > qerror_report_err(err); > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Amos Kong <akong@redhat.com> writes: > 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 qdev_set_parent_bus() back to object_property_add_child(), > we only needs to unref the object if setting properties fails. > > Signed-off-by: Amos Kong <akong@redhat.com> Broken in commit e0a83fc. Could be mentioned in commit message. Tested-by: Markus Armbruster <armbru@redhat.com>
On Thu, Jan 02, 2014 at 09:02:11AM +0800, Amos Kong wrote: > 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 qdev_set_parent_bus() back to object_property_add_child(), > we only needs to unref the object if setting properties fails. > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > V2: fix bz by adjust the initialization order (Paolo) Hi Anthony, other maintainer The V2 already reviewed and tested by Markus. Can you help to review & apply it? Thanks, Amos > --- > qdev-monitor.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/qdev-monitor.c b/qdev-monitor.c > index dc37a43..4070b0a 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -518,16 +518,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > /* create device, set properties */ > dev = DEVICE(object_new(driver)); > > - if (bus) { > - qdev_set_parent_bus(dev, bus); > - } > - > id = qemu_opts_id(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; > } > @@ -541,6 +536,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) > OBJECT(dev), NULL); > g_free(name); > } > + > + if (bus) { > + qdev_set_parent_bus(dev, bus); > + } > + > object_property_set_bool(OBJECT(dev), true, "realized", &err); > if (err != NULL) { > qerror_report_err(err); > -- > 1.8.4.2 >
Am 17.02.2014 11:23, schrieb Amos Kong: > On Thu, Jan 02, 2014 at 09:02:11AM +0800, Amos Kong wrote: >> 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 qdev_set_parent_bus() back to object_property_add_child(), >> we only needs to unref the object if setting properties fails. >> >> Signed-off-by: Amos Kong <akong@redhat.com> >> --- >> V2: fix bz by adjust the initialization order (Paolo) > > Hi Anthony, other maintainer > > The V2 already reviewed and tested by Markus. Can you help to review & apply it? Amos, I had pointed out to Paolo (IRC?) that this differs from how all legacy devices are being created, so I consider it a bad idea. qdev_set_parent_bus() is called from qdev_try_create(), which is called by qdev_create(). Devices may thus assume that the bus is set early, e.g. in their property setters invoked by qemu_opt_foreach(), and some functions have special behavior for a NULL bus (thinking of ISA here), so the change may lead to silent functional changes. Long-term we will have to move the code adding the device out of realize because we want to make realize work recursively on the composition tree. So what about rather moving the code adding the device to periph-anon / periph between dev->id and qemu_opt_foreach() so that the original unparenting works as expected? Regards, Andreas > > Thanks, Amos > >> --- >> qdev-monitor.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index dc37a43..4070b0a 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -518,16 +518,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> /* create device, set properties */ >> dev = DEVICE(object_new(driver)); >> >> - if (bus) { >> - qdev_set_parent_bus(dev, bus); >> - } >> - >> id = qemu_opts_id(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; >> } >> @@ -541,6 +536,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> OBJECT(dev), NULL); >> g_free(name); >> } >> + >> + if (bus) { >> + qdev_set_parent_bus(dev, bus); >> + } >> + >> object_property_set_bool(OBJECT(dev), true, "realized", &err); >> if (err != NULL) { >> qerror_report_err(err); >> -- >> 1.8.4.2 >>
diff --git a/qdev-monitor.c b/qdev-monitor.c index dc37a43..4070b0a 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -518,16 +518,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) /* create device, set properties */ dev = DEVICE(object_new(driver)); - if (bus) { - qdev_set_parent_bus(dev, bus); - } - id = qemu_opts_id(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; } @@ -541,6 +536,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) OBJECT(dev), NULL); g_free(name); } + + if (bus) { + qdev_set_parent_bus(dev, bus); + } + object_property_set_bool(OBJECT(dev), true, "realized", &err); if (err != NULL) { qerror_report_err(err);
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 qdev_set_parent_bus() back to object_property_add_child(), we only needs to unref the object if setting properties fails. Signed-off-by: Amos Kong <akong@redhat.com> --- V2: fix bz by adjust the initialization order (Paolo) --- qdev-monitor.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)