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

[Qemu-devel] [PATCH v6 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>

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 v6 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                 | 12 ++++++++++++
 include/monitor/monitor.h |  1 +
 monitor.c                 |  1 +
 qapi-schema.json          |  4 +++-
 5 files changed, 33 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..bebc44d 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,16 @@ 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 = NULL;
+    }
+    monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
+    if (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 v6 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 bebc44d..50bf2e6 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 v6 3/3] qmp: add path to device_deleted event

[Michael S. Tsirkin]

Add QOM path to device deleted event.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 QMP/qmp-events.txt | 4 +++-
 hw/qdev.c          | 9 ++++-----
 2 files changed, 7 insertions(+), 6 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 50bf2e6..2c861c1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -782,14 +782,13 @@ 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 = NULL;
+        event_data = qobject_from_jsonf("{ 'path': %s }", path);
     }
     monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
-    if (event_data) {
-        qobject_decref(event_data);
-    }
+    qobject_decref(event_data);
 }
 
 static void device_class_init(ObjectClass *class, void *data)
-- 
MST

Re: [Qemu-devel] [PATCH v6 1/3] qdev: 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.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  QMP/qmp-events.txt        | 16 ++++++++++++++++
>  hw/qdev.c                 | 12 ++++++++++++
>  include/monitor/monitor.h |  1 +
>  monitor.c                 |  1 +
>  qapi-schema.json          |  4 +++-
>  5 files changed, 33 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)

If there are no members present, do we get an event without member
"data", or do we get one with an empty member?

    { "event": "DEVICE_DELETED",
      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }


    { "event": "DEVICE_DELETED",
      "data": { },
      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }

There's no precedence, as this is the first event with data where all
data members are 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..bebc44d 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,16 @@ 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 = NULL;
> +    }
> +    monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
> +    if (event_data) {
> +        qobject_decref(event_data);
> +    }

You make this unconditional in 3/3.  Actually, unconditional should work
just fine even here.  No need to respin just for that.

Answering my doc question: we get an event without member "data".

Is that what we want?

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

What do you mean by "Guest reset will automatically complete removal for
all devices"?

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

[Michael S. Tsirkin]

On Thu, Mar 14, 2013 at 09:06:15AM +0100, Markus Armbruster wrote:
> "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.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  QMP/qmp-events.txt        | 16 ++++++++++++++++
> >  hw/qdev.c                 | 12 ++++++++++++
> >  include/monitor/monitor.h |  1 +
> >  monitor.c                 |  1 +
> >  qapi-schema.json          |  4 +++-
> >  5 files changed, 33 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)
> 
> If there are no members present, do we get an event without member
> "data", or do we get one with an empty member?
> 
>     { "event": "DEVICE_DELETED",
>       "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> 
> 
>     { "event": "DEVICE_DELETED",
>       "data": { },
>       "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> 
> There's no precedence, as this is the first event with data where all
> data members are 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..bebc44d 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,16 @@ 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 = NULL;
> > +    }
> > +    monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
> > +    if (event_data) {
> > +        qobject_decref(event_data);
> > +    }
> 
> You make this unconditional in 3/3.  Actually, unconditional should work
> just fine even here.  No need to respin just for that.
> 
> Answering my doc question: we get an event without member "data".
> 
> Is that what we want?
> 
> >  }
> >  
> >  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
> >  ##
> 
> What do you mean by "Guest reset will automatically complete removal for
> all devices"?

Just this. Try this: rmmod acpiphp in guest, then:

device_del
system_reset

and see the device disappear even though it was not acked by guest.

-- 
MST

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

[Michael S. Tsirkin]

On Thu, Mar 14, 2013 at 09:06:15AM +0100, Markus Armbruster wrote:
> "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.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  QMP/qmp-events.txt        | 16 ++++++++++++++++
> >  hw/qdev.c                 | 12 ++++++++++++
> >  include/monitor/monitor.h |  1 +
> >  monitor.c                 |  1 +
> >  qapi-schema.json          |  4 +++-
> >  5 files changed, 33 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)
> 
> If there are no members present, do we get an event without member
> "data", or do we get one with an empty member?
> 
>     { "event": "DEVICE_DELETED",
>       "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> 
> 
>     { "event": "DEVICE_DELETED",
>       "data": { },
>       "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> 
> There's no precedence, as this is the first event with data where all
> data members are optional.

We get one without data. We currently have events with data and some
members, and events without data, but not events with an empty data,
I didn't see a need to invent one with an empty data. You?

> > +
> > +{ "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..bebc44d 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,16 @@ 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 = NULL;
> > +    }
> > +    monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
> > +    if (event_data) {
> > +        qobject_decref(event_data);
> > +    }
> 
> You make this unconditional in 3/3.  Actually, unconditional should work
> just fine even here.  No need to respin just for that.
> 
> Answering my doc question: we get an event without member "data".
> 
> Is that what we want?

Does it matter?

> >  }
> >  
> >  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
> >  ##
> 
> What do you mean by "Guest reset will automatically complete removal for
> all devices"?

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

[Markus Armbruster]

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

> On Thu, Mar 14, 2013 at 09:06:15AM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
[...]
>> > 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
>> >  ##
>> 
>> What do you mean by "Guest reset will automatically complete removal for
>> all devices"?
>
> Just this. Try this: rmmod acpiphp in guest, then:
>
> device_del
> system_reset
>
> and see the device disappear even though it was not acked by guest.

Cool, I didn't know that.

Just to make sure: does this automatic removal completion send
DEVICE_DELETED events?

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

[Michael S. Tsirkin]

On Thu, Mar 14, 2013 at 01:13:54PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Mar 14, 2013 at 09:06:15AM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> [...]
> >> > 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
> >> >  ##
> >> 
> >> What do you mean by "Guest reset will automatically complete removal for
> >> all devices"?
> >
> > Just this. Try this: rmmod acpiphp in guest, then:
> >
> > device_del
> > system_reset
> >
> > and see the device disappear even though it was not acked by guest.
> 
> Cool, I didn't know that.
> 
> Just to make sure: does this automatic removal completion send
> DEVICE_DELETED events?

With my patch, it does.

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

[Markus Armbruster]

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

> On Thu, Mar 14, 2013 at 09:06:15AM +0100, Markus Armbruster wrote:
>> "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.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> >  QMP/qmp-events.txt        | 16 ++++++++++++++++
>> >  hw/qdev.c                 | 12 ++++++++++++
>> >  include/monitor/monitor.h |  1 +
>> >  monitor.c                 |  1 +
>> >  qapi-schema.json          |  4 +++-
>> >  5 files changed, 33 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)
>> 
>> If there are no members present, do we get an event without member
>> "data", or do we get one with an empty member?
>> 
>>     { "event": "DEVICE_DELETED",
>>       "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> 
>> 
>>     { "event": "DEVICE_DELETED",
>>       "data": { },
>>       "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> 
>> There's no precedence, as this is the first event with data where all
>> data members are optional.
>
> We get one without data. We currently have events with data and some
> members, and events without data, but not events with an empty data,
> I didn't see a need to invent one with an empty data. You?

Donning my downstream hat for a minute.

Upstream after this series is fully applied:
* event always has data
* data always has member path
* data may have member device

Downstream after backport of just 1/3:
* event may have data
* data always has member device

Alternative downstream: event always has data, data 
* event always has data
* data may have member device

Smaller difference to upstream.  Would libvirt care?

>> > +
>> > +{ "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..bebc44d 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,16 @@ 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 = NULL;
>> > +    }
>> > +    monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data);
>> > +    if (event_data) {
>> > +        qobject_decref(event_data);
>> > +    }
>> 
>> You make this unconditional in 3/3.  Actually, unconditional should work
>> just fine even here.  No need to respin just for that.
>> 
>> Answering my doc question: we get an event without member "data".
>> 
>> Is that what we want?
>
> Does it matter?

It doesn't matter upstream, because the anomaly created by 1/3 gets
removed by 3/3.  It matters downstream if 1/3 gets backported without
3/3.

I will not withhold my upstream ACK because of such downstream concerns.

[...]

Re: [Qemu-devel] [libvirt] [PATCH v6 1/3] qdev: DEVICE_DELETED event

[Eric Blake]

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

On 03/14/2013 06:22 AM, Michael S. Tsirkin wrote:
>>> Just this. Try this: rmmod acpiphp in guest, then:
>>>
>>> device_del
>>> system_reset
>>>
>>> and see the device disappear even though it was not acked by guest.
>>
>> Cool, I didn't know that.
>>
>> Just to make sure: does this automatic removal completion send
>> DEVICE_DELETED events?
> 
> With my patch, it does.

Good.  That is the best behavior, if libvirt is going to start relying
on the event as a means to avoid polling on all but libvirtd restarts.

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