[Qemu-devel] [PATCH v2] qdev: add the device to the QOM tree before using it to set a link

[Qemu-devel] [PATCH v2] qdev: add the device to the QOM tree before using it to set a link

[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.

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);
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v2] qdev: add the device to the QOM tree before using it to set a link

[Paolo Bonzini]

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>

Re: [Qemu-devel] [PATCH v2] qdev: add the device to the QOM tree before using it to set a link

[Markus Armbruster]

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>

Re: [Qemu-devel] [PATCH v2] qdev: add the device to the QOM tree before using it to set a link

[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?

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
> 

Re: [Qemu-devel] [PATCH v2] qdev: add the device to the QOM tree before using it to set a link

[Andreas Färber]

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
>>


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