qdev: unparent device when fails to set properties

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

When it fails to set properties, qdev's parent is already set, but the
object hasn't been added to parent object, object_unparent() won't
unparent the device. This patch unparents device in the mediacy.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qdev-monitor.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Tue, Dec 31, 2013 at 04:06:57PM +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.
> 
> When it fails to set properties, qdev's parent is already set, but the
> object hasn't been added to parent object, object_unparent() won't
> unparent the device. This patch unparents device in the mediacy.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qdev-monitor.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index dc37a43..3d8b4f4 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -527,7 +527,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          dev->id = id;
>      }
>      if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
> -        object_unparent(OBJECT(dev));
> +        if (OBJECT(dev)->class->unparent) {
> +            (OBJECT(dev)->class->unparent)(OBJECT(dev));
> +        }

This means object_unparent()(or device_unparent()) doesn't handle
incompletely initialized object correctly. How about fix it in
object_unparent()/device_unparent()?

BTW, it must be commit e0a83fc2c1582dc8 introdues the problem.

On Tue, Dec 31, 2013 at 05:09:36PM +0800, Hu Tao wrote:
> On Tue, Dec 31, 2013 at 04:06:57PM +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.
> > 
> > When it fails to set properties, qdev's parent is already set, but the
> > object hasn't been added to parent object, object_unparent() won't
> > unparent the device. This patch unparents device in the mediacy.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  qdev-monitor.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index dc37a43..3d8b4f4 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -527,7 +527,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >          dev->id = id;
> >      }
> >      if (qemu_opt_foreach(opts, set_property, dev, 1) != 0) {
> > -        object_unparent(OBJECT(dev));
> > +        if (OBJECT(dev)->class->unparent) {
> > +            (OBJECT(dev)->class->unparent)(OBJECT(dev));
> > +        }
> 

Hi Tao,

> This means object_unparent()(or device_unparent()) doesn't handle
> incompletely initialized object correctly. How about fix it in
> object_unparent()/device_unparent()?

We can't fix object_unparent() to cleanup this immature object.
device_unparent() is used to clean device, but it's not called.

OBJECT(dev)->class->unparent is initialized to device_unparent().
So my patch just called OBJECT(dev)->class->unparent().
 
> BTW, it must be commit e0a83fc2c1582dc8 introdues the problem.

Yes.

Il 31/12/2013 09:06, Amos Kong ha scritto:
> When it fails to set properties, qdev's parent is already set

Do not confuse the QOM parent (which is /machine/peripheral, of which
the new device is a child) with the qdev parent bus (which has a link to
the new device)!

In general, you should add the device to the QOM tree before using it to
set a link.  So I believe that object_property_add_child should be
called before qdev_set_parent_bus.  This is the root cause of the bug;
the fix then could be one of the following:

1) move qdev_set_parent_bus later;

2) move object_property_add_child before the setting of properties.


I slightly prefer the first, so that initialization happens in this order:

1) create object

2) set properties -- if it fails, you can just unref the object

3) add child -- if it fails due to duplicate ID, you can again just
unref the object

4) set parent bus (cannot fail)

5) realize -- if it fails, you need to unparent the object and unref the
object

6) drop the reference now that the object is kept solidly alive by
QOM/qdev, and return.

This matches a bit more closely what happens with object-add, too.

Paolo

On Tue, Dec 31, 2013 at 05:06:44PM +0100, Paolo Bonzini wrote:
> Il 31/12/2013 09:06, Amos Kong ha scritto:
> > When it fails to set properties, qdev's parent is already set
> 
> Do not confuse the QOM parent (which is /machine/peripheral, of which
> the new device is a child) with the qdev parent bus (which has a link to
> the new device)!
> 
> In general, you should add the device to the QOM tree before using it to
> set a link.  So I believe that object_property_add_child should be
> called before qdev_set_parent_bus.  This is the root cause of the bug;
> the fix then could be one of the following:
> 
> 1) move qdev_set_parent_bus later;
> 
> 2) move object_property_add_child before the setting of properties.
 
I agree to fix this problem by adjust the initialization order.
Thanks for the detail explain.
 
> I slightly prefer the first, so that initialization happens in this order:
> 
> 1) create object
> 
> 2) set properties -- if it fails, you can just unref the object
> 
> 3) add child -- if it fails due to duplicate ID, you can again just
> unref the object
> 
> 4) set parent bus (cannot fail)
> 
> 5) realize -- if it fails, you need to unparent the object and unref the
> object
> 
> 6) drop the reference now that the object is kept solidly alive by
> QOM/qdev, and return.
> 
> This matches a bit more closely what happens with object-add, too.
> 
> Paolo