[Qemu-devel] [PATCH v8 0/3] DEVICE_DELETED event

[Qemu-devel] [PATCH v8 0/3] DEVICE_DELETED event

[Michael S. Tsirkin]

libvirt has a long-standing bug: when removing the device,
it can request removal but does not know when the
removal completes. Add an event so we can fix this in a robust way.

First patch only adds the event with ID, second patch adds a path field.
Split this way for ease of backport (stable downstreams without QOM
would want to only take the first patch).
Event without fields is still useful as management can use it to
poll device list to figure out which device was removed.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

If there are no more comments I'll stick this on my
pci branch.

Changes from v7:
    - none, v7 was malformed series sent by mistake

Changes from v6:
    - make empty event use data: {}, Markus prefers this

Changes from v5:
    - Emit an empty event on unnamed devices in patch 1/3, as suggested by Markus

Changes from v4:
    - Add extra triggers and extra fields as requested by Markus

Changes from v3:
    - Document that we only emit events for devices with
      and ID, as suggested by Markus
Changes from v2:
    - move event toward the end of device_unparent,
      so that parents are reported after their children,
      as suggested by Paolo
Changes from v1:
    - move to device_unparent
    - address comments by Andreas and Eric


-- 
Anthony Liguori


Michael S. Tsirkin (3):
  qdev: DEVICE_DELETED event
  qom: pass original path to unparent method
  qmp: add path to device_deleted event

 QMP/qmp-events.txt        | 18 ++++++++++++++++++
 hw/qdev.c                 | 15 +++++++++++++--
 include/monitor/monitor.h |  1 +
 include/qom/object.h      |  3 ++-
 monitor.c                 |  1 +
 qapi-schema.json          |  4 +++-
 qom/object.c              |  4 +++-
 7 files changed, 41 insertions(+), 5 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v8 1/3] qdev: DEVICE_DELETED event

[Michael S. Tsirkin]

libvirt has a long-standing bug: when removing the device,
it can request removal but does not know when the
removal completes. Add an event so we can fix this in a robust way.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 QMP/qmp-events.txt        | 16 ++++++++++++++++
 hw/qdev.c                 | 10 ++++++++++
 include/monitor/monitor.h |  1 +
 monitor.c                 |  1 +
 qapi-schema.json          |  4 +++-
 5 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index b2698e4..24cf3e8 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -136,6 +136,22 @@ Example:
 Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR
 event.
 
+DEVICE_DELETED
+-----------------
+
+Emitted whenever the device removal completion is acknowledged
+by the guest.
+At this point, it's safe to reuse the specified device ID.
+Device removal can be initiated by the guest or by HMP/QMP commands.
+
+Data:
+
+- "device": device name (json-string, optional)
+
+{ "event": "DEVICE_DELETED",
+  "data": { "device": "virtio-net-pci-0" },
+  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+
 DEVICE_TRAY_MOVED
 -----------------
 
diff --git a/hw/qdev.c b/hw/qdev.c
index 689cd54..741af96 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -29,6 +29,7 @@
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "qapi/qmp/qjson.h"
 
 int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
@@ -760,6 +761,7 @@ static void device_unparent(Object *obj)
     DeviceState *dev = DEVICE(obj);
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     BusState *bus;
+    QObject *event_data;
 
     while (dev->num_child_bus) {
         bus = QLIST_FIRST(&dev->child_bus);
@@ -778,6 +780,14 @@ static void device_unparent(Object *obj)
         object_unref(OBJECT(dev->parent_bus));
         dev->parent_bus = NULL;
     }
+
+    if (dev->id) {
+        event_data = qobject_from_jsonf("{ 'device': %s }", dev->id);
+    } else {
+        event_data = qobject_from_jsonf("{ }");
+    }
+    monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
+    qobject_decref(event_data);
 }
 
 static void device_class_init(ObjectClass *class, void *data)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 87fb49c..b868760 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -39,6 +39,7 @@ typedef enum MonitorEvent {
     QEVENT_BLOCK_JOB_CANCELLED,
     QEVENT_BLOCK_JOB_ERROR,
     QEVENT_BLOCK_JOB_READY,
+    QEVENT_DEVICE_DELETED,
     QEVENT_DEVICE_TRAY_MOVED,
     QEVENT_SUSPEND,
     QEVENT_SUSPEND_DISK,
diff --git a/monitor.c b/monitor.c
index 32a6e74..2a5e7b6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -457,6 +457,7 @@ static const char *monitor_event_names[] = {
     [QEVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED",
     [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR",
     [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY",
+    [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED",
     [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
     [QEVENT_SUSPEND] = "SUSPEND",
     [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK",
diff --git a/qapi-schema.json b/qapi-schema.json
index 28b070f..bb361e1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2354,7 +2354,9 @@
 # Notes: When this command completes, the device may not be removed from the
 #        guest.  Hot removal is an operation that requires guest cooperation.
 #        This command merely requests that the guest begin the hot removal
-#        process.
+#        process.  Completion of the device removal process is signaled with a
+#        DEVICE_DELETED event. Guest reset will automatically complete removal
+#        for all devices.
 #
 # Since: 0.14.0
 ##
-- 
MST

[Qemu-devel] [PATCH v8 2/3] qom: pass original path to unparent method

[Michael S. Tsirkin]

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);
     }
     object_unref(obj);
+    g_free(path);
 }
 
 static void object_deinit(Object *obj, TypeImpl *type)
-- 
MST

[Qemu-devel] [PATCH v8 3/3] qmp: add path to device_deleted event

[Michael S. Tsirkin]

Add QOM path to device deleted event.  It now becomes useful to report
it for devices which don't have an ID assigned.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 QMP/qmp-events.txt | 4 +++-
 hw/qdev.c          | 5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 24cf3e8..dcc826d 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -147,9 +147,11 @@ Device removal can be initiated by the guest or by HMP/QMP commands.
 Data:
 
 - "device": device name (json-string, optional)
+- "path": device path (json-string)
 
 { "event": "DEVICE_DELETED",
-  "data": { "device": "virtio-net-pci-0" },
+  "data": { "device": "virtio-net-pci-0",
+            "path": "/machine/peripheral/virtio-net-pci-0" },
   "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 
 DEVICE_TRAY_MOVED
diff --git a/hw/qdev.c b/hw/qdev.c
index 64546cf..2c861c1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -782,9 +782,10 @@ static void device_unparent(Object *obj, const char *path)
     }
 
     if (dev->id) {
-        event_data = qobject_from_jsonf("{ 'device': %s }", dev->id);
+        event_data = qobject_from_jsonf("{ 'device': %s, 'path': %s }",
+                                        dev->id, path);
     } else {
-        event_data = qobject_from_jsonf("{ }");
+        event_data = qobject_from_jsonf("{ 'path': %s }", path);
     }
     monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
     qobject_decref(event_data);
-- 
MST

Re: [Qemu-devel] [PATCH v8 0/3] DEVICE_DELETED event

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> libvirt has a long-standing bug: when removing the device,
> it can request removal but does not know when the
> removal completes. Add an event so we can fix this in a robust way.
>
> First patch only adds the event with ID, second patch adds a path field.
> Split this way for ease of backport (stable downstreams without QOM
> would want to only take the first patch).
> Event without fields is still useful as management can use it to
> poll device list to figure out which device was removed.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Thanks for your patience.

Acked-by: Markus Armbruster <armbru@redhat.com>

> If there are no more comments I'll stick this on my
> pci branch.

I don't mind.

Re: [Qemu-devel] [libvirt] [PATCH v8 0/3] DEVICE_DELETED event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

On 03/14/2013 06:40 AM, Michael S. Tsirkin wrote:
> libvirt has a long-standing bug: when removing the device,
> it can request removal but does not know when the
> removal completes. Add an event so we can fix this in a robust way.
> 
> First patch only adds the event with ID, second patch adds a path field.
> Split this way for ease of backport (stable downstreams without QOM
> would want to only take the first patch).
> Event without fields is still useful as management can use it to
> poll device list to figure out which device was removed.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Series: Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> If there are no more comments I'll stick this on my
> pci branch.

Of course, libvirt still has to still add a patch to start using this
event, but I think we have reached a good design, including
consideration for distros doing partial backports, which will minimize
the surprises for libvirt.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v8 2/3] qom: pass original path to unparent method

[Anthony Liguori]

"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

Re: [Qemu-devel] [PATCH v8 2/3] qom: pass original path to unparent method

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH v8 2/3] qom: pass original path to unparent method

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v8 2/3] qom: pass original path to unparent method

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v8 2/3] qom: pass original path to unparent method

[Anthony Liguori]

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