Patchwork [v8,2/3] qom: pass original path to unparent method

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 14, 2013, 12:40 p.m.
Message ID <50e744fbae4b08dc4ec33d5d44acc83da7170391.1363264726.git.mst@redhat.com>
Download mbox | patch
Permalink /patch/227647/
State New
Headers show

Comments

Michael S. Tsirkin - March 14, 2013, 12:40 p.m.
We need to know the original path since unparenting loses this state.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/qdev.c            | 4 ++--
 include/qom/object.h | 3 ++-
 qom/object.c         | 4 +++-
 3 files changed, 7 insertions(+), 4 deletions(-)
Anthony Liguori - March 18, 2013, 2:24 p.m.
"Michael S. Tsirkin" <mst@redhat.com> writes:

> We need to know the original path since unparenting loses this state.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/qdev.c            | 4 ++--
>  include/qom/object.h | 3 ++-
>  qom/object.c         | 4 +++-
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 741af96..64546cf 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -436,7 +436,7 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
>      }
>  }
>  
> -static void bus_unparent(Object *obj)
> +static void bus_unparent(Object *obj, const char *path)
>  {
>      BusState *bus = BUS(obj);
>      BusChild *kid;
> @@ -756,7 +756,7 @@ static void device_class_base_init(ObjectClass *class, void *data)
>      klass->props = NULL;
>  }
>  
> -static void device_unparent(Object *obj)
> +static void device_unparent(Object *obj, const char *path)
>  {
>      DeviceState *dev = DEVICE(obj);
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> diff --git a/include/qom/object.h b/include/qom/object.h
> index cf094e7..f0790d4 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -330,11 +330,12 @@ typedef struct ObjectProperty
>  /**
>   * ObjectUnparent:
>   * @obj: the object that is being removed from the composition tree
> + * @path: canonical path that object had if any
>   *
>   * Called when an object is being removed from the QOM composition tree.
>   * The function should remove any backlinks from children objects to @obj.
>   */
> -typedef void (ObjectUnparent)(Object *obj);
> +typedef void (ObjectUnparent)(Object *obj, const char *path);
>  
>  /**
>   * ObjectFree:
> diff --git a/qom/object.c b/qom/object.c
> index 3d638ff..21c9da4 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -362,14 +362,16 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>  
>  void object_unparent(Object *obj)
>  {
> +    gchar *path = object_get_canonical_path(obj);
>      object_ref(obj);
>      if (obj->parent) {
>          object_property_del_child(obj->parent, obj, NULL);
>      }
>      if (obj->class->unparent) {
> -        (obj->class->unparent)(obj);
> +        (obj->class->unparent)(obj, path);
>      }

I think you should actually just move this call above
if (obj->parent) { object_parent_del_child(...); }.

There's no harm AFAICT in doing this and it seems more logical to me to
have destruction flow start with the subclass and move up to the base
class.

This avoids needing a hack like this because the object is still in a
reasonable state when unparent is called.

Paolo, do you see anything wrong with this?  I looked at the commit you
added this in and it doesn't look like it would be a problem.

Regards,

Anthony Liguori

>      object_unref(obj);
> +    g_free(path);
>  }
>  
>  static void object_deinit(Object *obj, TypeImpl *type)
> -- 
> MST
Michael S. Tsirkin - March 18, 2013, 2:35 p.m.
On Mon, Mar 18, 2013 at 09:24:16AM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > We need to know the original path since unparenting loses this state.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/qdev.c            | 4 ++--
> >  include/qom/object.h | 3 ++-
> >  qom/object.c         | 4 +++-
> >  3 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 741af96..64546cf 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -436,7 +436,7 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
> >      }
> >  }
> >  
> > -static void bus_unparent(Object *obj)
> > +static void bus_unparent(Object *obj, const char *path)
> >  {
> >      BusState *bus = BUS(obj);
> >      BusChild *kid;
> > @@ -756,7 +756,7 @@ static void device_class_base_init(ObjectClass *class, void *data)
> >      klass->props = NULL;
> >  }
> >  
> > -static void device_unparent(Object *obj)
> > +static void device_unparent(Object *obj, const char *path)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index cf094e7..f0790d4 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -330,11 +330,12 @@ typedef struct ObjectProperty
> >  /**
> >   * ObjectUnparent:
> >   * @obj: the object that is being removed from the composition tree
> > + * @path: canonical path that object had if any
> >   *
> >   * Called when an object is being removed from the QOM composition tree.
> >   * The function should remove any backlinks from children objects to @obj.
> >   */
> > -typedef void (ObjectUnparent)(Object *obj);
> > +typedef void (ObjectUnparent)(Object *obj, const char *path);
> >  
> >  /**
> >   * ObjectFree:
> > diff --git a/qom/object.c b/qom/object.c
> > index 3d638ff..21c9da4 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -362,14 +362,16 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
> >  
> >  void object_unparent(Object *obj)
> >  {
> > +    gchar *path = object_get_canonical_path(obj);
> >      object_ref(obj);
> >      if (obj->parent) {
> >          object_property_del_child(obj->parent, obj, NULL);
> >      }
> >      if (obj->class->unparent) {
> > -        (obj->class->unparent)(obj);
> > +        (obj->class->unparent)(obj, path);
> >      }
> 
> I think you should actually just move this call above
> if (obj->parent) { object_parent_del_child(...); }.
> 
> There's no harm AFAICT in doing this and it seems more logical to me to
> have destruction flow start with the subclass and move up to the base
> class.

At Paolo's request children are intentionally reported before parents,
shouldn't this apply?

> 
> This avoids needing a hack like this because the object is still in a
> reasonable state when unparent is called.
> 
> Paolo, do you see anything wrong with this?  I looked at the commit you
> added this in and it doesn't look like it would be a problem.
> 
> Regards,
> 
> Anthony Liguori

Hmm I already put this on my branch (and sent a pull request).
I guess I could back it out, though it will create minor problems if
someone is basing on my tree.

Cleanup in a separate patch?

> >      object_unref(obj);
> > +    g_free(path);
> >  }
> >  
> >  static void object_deinit(Object *obj, TypeImpl *type)
> > -- 
> > MST
Paolo Bonzini - March 18, 2013, 3:03 p.m.
Il 18/03/2013 15:24, Anthony Liguori ha scritto:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
>> We need to know the original path since unparenting loses this state.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  hw/qdev.c            | 4 ++--
>>  include/qom/object.h | 3 ++-
>>  qom/object.c         | 4 +++-
>>  3 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 741af96..64546cf 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -436,7 +436,7 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
>>      }
>>  }
>>  
>> -static void bus_unparent(Object *obj)
>> +static void bus_unparent(Object *obj, const char *path)
>>  {
>>      BusState *bus = BUS(obj);
>>      BusChild *kid;
>> @@ -756,7 +756,7 @@ static void device_class_base_init(ObjectClass *class, void *data)
>>      klass->props = NULL;
>>  }
>>  
>> -static void device_unparent(Object *obj)
>> +static void device_unparent(Object *obj, const char *path)
>>  {
>>      DeviceState *dev = DEVICE(obj);
>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index cf094e7..f0790d4 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -330,11 +330,12 @@ typedef struct ObjectProperty
>>  /**
>>   * ObjectUnparent:
>>   * @obj: the object that is being removed from the composition tree
>> + * @path: canonical path that object had if any
>>   *
>>   * Called when an object is being removed from the QOM composition tree.
>>   * The function should remove any backlinks from children objects to @obj.
>>   */
>> -typedef void (ObjectUnparent)(Object *obj);
>> +typedef void (ObjectUnparent)(Object *obj, const char *path);
>>  
>>  /**
>>   * ObjectFree:
>> diff --git a/qom/object.c b/qom/object.c
>> index 3d638ff..21c9da4 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -362,14 +362,16 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>>  
>>  void object_unparent(Object *obj)
>>  {
>> +    gchar *path = object_get_canonical_path(obj);
>>      object_ref(obj);
>>      if (obj->parent) {
>>          object_property_del_child(obj->parent, obj, NULL);
>>      }
>>      if (obj->class->unparent) {
>> -        (obj->class->unparent)(obj);
>> +        (obj->class->unparent)(obj, path);
>>      }
> 
> I think you should actually just move this call above
> if (obj->parent) { object_parent_del_child(...); }.
> 
> There's no harm AFAICT in doing this and it seems more logical to me to
> have destruction flow start with the subclass and move up to the base
> class.
> 
> This avoids needing a hack like this because the object is still in a
> reasonable state when unparent is called.
> 
> Paolo, do you see anything wrong with this?  I looked at the commit you
> added this in and it doesn't look like it would be a problem.

Yes, seems okay.  Especially if you think of object_property_del_child
as the base class's implementation of unparent.

Paolo
Paolo Bonzini - March 18, 2013, 3:08 p.m.
Il 18/03/2013 15:35, Michael S. Tsirkin ha scritto:
> > There's no harm AFAICT in doing this and it seems more logical to me to
> > have destruction flow start with the subclass and move up to the base
> > class.
> 
> At Paolo's request children are intentionally reported before parents,
> shouldn't this apply?

That's ok.  Because children are reported first, the parent's path will
still be in place while the children are being unparented.

Subclasses and the composition tree form two different dimensions, but
in both cases the idea is to unparent bottom-up:

1) subclasses dictate the order in which to unparent a single object.
The subclasses should be "disconnected" first, before moving up towards
Object whose unparenting is done by object_parent_del_child.  This way,
when a subclass's unparent runs the superclass's bits are still in a
good state.

2) the composition tree dictates the order in which to unparent multiple
objects.  Here children should be unparented first.  This way, when a
child's unparent runs the parent is still in a good state.

Paolo
Anthony Liguori - March 18, 2013, 4:04 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 18/03/2013 15:24, Anthony Liguori ha scritto:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>>> We need to know the original path since unparenting loses this state.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  hw/qdev.c            | 4 ++--
>>>  include/qom/object.h | 3 ++-
>>>  qom/object.c         | 4 +++-
>>>  3 files changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index 741af96..64546cf 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -436,7 +436,7 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
>>>      }
>>>  }
>>>  
>>> -static void bus_unparent(Object *obj)
>>> +static void bus_unparent(Object *obj, const char *path)
>>>  {
>>>      BusState *bus = BUS(obj);
>>>      BusChild *kid;
>>> @@ -756,7 +756,7 @@ static void device_class_base_init(ObjectClass *class, void *data)
>>>      klass->props = NULL;
>>>  }
>>>  
>>> -static void device_unparent(Object *obj)
>>> +static void device_unparent(Object *obj, const char *path)
>>>  {
>>>      DeviceState *dev = DEVICE(obj);
>>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>> index cf094e7..f0790d4 100644
>>> --- a/include/qom/object.h
>>> +++ b/include/qom/object.h
>>> @@ -330,11 +330,12 @@ typedef struct ObjectProperty
>>>  /**
>>>   * ObjectUnparent:
>>>   * @obj: the object that is being removed from the composition tree
>>> + * @path: canonical path that object had if any
>>>   *
>>>   * Called when an object is being removed from the QOM composition tree.
>>>   * The function should remove any backlinks from children objects to @obj.
>>>   */
>>> -typedef void (ObjectUnparent)(Object *obj);
>>> +typedef void (ObjectUnparent)(Object *obj, const char *path);
>>>  
>>>  /**
>>>   * ObjectFree:
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 3d638ff..21c9da4 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -362,14 +362,16 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>>>  
>>>  void object_unparent(Object *obj)
>>>  {
>>> +    gchar *path = object_get_canonical_path(obj);
>>>      object_ref(obj);
>>>      if (obj->parent) {
>>>          object_property_del_child(obj->parent, obj, NULL);
>>>      }
>>>      if (obj->class->unparent) {
>>> -        (obj->class->unparent)(obj);
>>> +        (obj->class->unparent)(obj, path);
>>>      }
>> 
>> I think you should actually just move this call above
>> if (obj->parent) { object_parent_del_child(...); }.
>> 
>> There's no harm AFAICT in doing this and it seems more logical to me to
>> have destruction flow start with the subclass and move up to the base
>> class.
>> 
>> This avoids needing a hack like this because the object is still in a
>> reasonable state when unparent is called.
>> 
>> Paolo, do you see anything wrong with this?  I looked at the commit you
>> added this in and it doesn't look like it would be a problem.
>
> Yes, seems okay.  Especially if you think of object_property_del_child
> as the base class's implementation of unparent.

Cool, Michael can you update your patch?  Should simplify it quite a
bit.

Regards,

Anthony Liguori

>
> Paolo

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 741af96..64546cf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -436,7 +436,7 @@  static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
     }
 }
 
-static void bus_unparent(Object *obj)
+static void bus_unparent(Object *obj, const char *path)
 {
     BusState *bus = BUS(obj);
     BusChild *kid;
@@ -756,7 +756,7 @@  static void device_class_base_init(ObjectClass *class, void *data)
     klass->props = NULL;
 }
 
-static void device_unparent(Object *obj)
+static void device_unparent(Object *obj, const char *path)
 {
     DeviceState *dev = DEVICE(obj);
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
diff --git a/include/qom/object.h b/include/qom/object.h
index cf094e7..f0790d4 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -330,11 +330,12 @@  typedef struct ObjectProperty
 /**
  * ObjectUnparent:
  * @obj: the object that is being removed from the composition tree
+ * @path: canonical path that object had if any
  *
  * Called when an object is being removed from the QOM composition tree.
  * The function should remove any backlinks from children objects to @obj.
  */
-typedef void (ObjectUnparent)(Object *obj);
+typedef void (ObjectUnparent)(Object *obj, const char *path);
 
 /**
  * ObjectFree:
diff --git a/qom/object.c b/qom/object.c
index 3d638ff..21c9da4 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -362,14 +362,16 @@  static void object_property_del_child(Object *obj, Object *child, Error **errp)
 
 void object_unparent(Object *obj)
 {
+    gchar *path = object_get_canonical_path(obj);
     object_ref(obj);
     if (obj->parent) {
         object_property_del_child(obj->parent, obj, NULL);
     }
     if (obj->class->unparent) {
-        (obj->class->unparent)(obj);
+        (obj->class->unparent)(obj, path);
     }
     object_unref(obj);
+    g_free(path);
 }
 
 static void object_deinit(Object *obj, TypeImpl *type)