[libvirt] [PATCHv5 0/3] DEVICE_DELETED event
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 for devices with ID, second path adds it for all devices and adds a path fields. Split this way for ease of backport (stable downstreams without QOM would want to only take the first patch), and also because the 2nd/3rd patches might turn out to be more controversial - if they really turn out such I'd like just the 1st one to get applied while we discuss 2 and 3. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> 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
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 | 17 +++++++++++++++++ hw/qdev.c | 6 ++++++ include/monitor/monitor.h | 1 + monitor.c | 1 + qapi-schema.json | 4 +++- 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index b2698e4..0ab5017 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -136,6 +136,23 @@ 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. This event is only emitted for devices with +a user-specified ID. +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) + +{ "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..393e83e 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; @@ -778,6 +779,11 @@ static void device_unparent(Object *obj) object_unref(OBJECT(dev->parent_bus)); dev->parent_bus = NULL; } + if (dev->id) { + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); + qobject_decref(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
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 393e83e..8ee8a97 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
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 | 9 +++++---- hw/qdev.c | 11 ++++++++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index 0ab5017..dcc826d 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -140,17 +140,18 @@ DEVICE_DELETED ----------------- Emitted whenever the device removal completion is acknowledged -by the guest. This event is only emitted for devices with -a user-specified ID. +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) +- "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 8ee8a97..2c861c1 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -761,6 +761,7 @@ static void device_unparent(Object *obj, const char *path) 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); @@ -779,11 +780,15 @@ static void device_unparent(Object *obj, const char *path) object_unref(OBJECT(dev->parent_bus)); dev->parent_bus = NULL; } + if (dev->id) { - QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); - monitor_protocol_event(QEVENT_DEVICE_DELETED, data); - qobject_decref(data); + event_data = qobject_from_jsonf("{ 'device': %s, 'path': %s }", + dev->id, path); + } else { + event_data = qobject_from_jsonf("{ 'path': %s }", path); } + monitor_protocol_event(QEVENT_DEVICE_DELETED, event_data); + qobject_decref(event_data); } static void device_class_init(ObjectClass *class, void *data) -- MST
Anthony asked for a space between "PATCH" and "v<N>" in the subject. Please try to remember next time. "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 for devices with ID, second path adds it for all devices and adds a path fields. Split this way for ease of backport (stable downstreams without QOM would want to only take the first patch),
I'd *strongly* advice downstreams to take the first patch plus the part of the third patch that changes the event trigger. Let's compare the difference to upstream for both approaches. Just the first patch: event fires only when device has an ID. Upstream: event fires always. Subtle semantic difference. First patch + changed trigger: event doesn't have member path. Upstream: event has member path. Blatantly obvious syntactic difference. I'd take syntactic over semantic any day. Note that even without member path a QMP client can still find which device is gone, by polling. Any client designed to keep track of state accurately across disconnect/reconnect (such as libvirt) needs such polling code anyway. If you want to make backporters' lives easier, move the event trigger change from patch 3 to patch 1.
and also because the 2nd/3rd patches might turn out to be more controversial - if they really turn out such I'd like just the 1st one to get applied while we discuss 2 and 3.
I don't expect more controversy. If I'm wrong, I don't want just patch 1 applied while we discuss, because that creates an longer stretch in git where the event is there, but doesn't work like we want it to, for no appreciable gain. We're in no particular hurry here, so let's do it right.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Options in decreasing order of preference, pick one that suits you: 1. Go the extra mile for backporters, and move the event trigger change from PATCH 3 into 1. 2. Squash PATCH 1 into 3. Backporting trouble is obvious and easy to resolve: just drop member path. Feel free to point that out in the commit message. 3. Let the series stand as is. Backporting trouble is subtle and easy to miss: backporting just PATCH 1 is tempting, but screws up the event trigger. I wouldn't do it that way myself, but I'm not going to NAK usable upstream work because of such downstream concerns. In case you pick 3: Acked-by: Markus Armbruster <armbru@redhat.com>
On Wed, Mar 13, 2013 at 08:45:51AM +0100, Markus Armbruster wrote:
Anthony asked for a space between "PATCH" and "v<N>" in the subject. Please try to remember next time.
"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 for devices with ID, second path adds it for all devices and adds a path fields. Split this way for ease of backport (stable downstreams without QOM would want to only take the first patch),
I'd *strongly* advice downstreams to take the first patch plus the part of the third patch that changes the event trigger.
Let's compare the difference to upstream for both approaches.
Just the first patch: event fires only when device has an ID. Upstream: event fires always. Subtle semantic difference.
First patch + changed trigger: event doesn't have member path. Upstream: event has member path. Blatantly obvious syntactic difference.
I'd take syntactic over semantic any day.
Note that even without member path a QMP client can still find which device is gone, by polling. Any client designed to keep track of state accurately across disconnect/reconnect (such as libvirt) needs such polling code anyway.
Ah, good point. Empty event seemed useless to me, now I see it isn't.
If you want to make backporters' lives easier, move the event trigger change from patch 3 to patch 1.
and also because the 2nd/3rd patches might turn out to be more controversial - if they really turn out such I'd like just the 1st one to get applied while we discuss 2 and 3.
I don't expect more controversy. If I'm wrong, I don't want just patch 1 applied while we discuss, because that creates an longer stretch in git where the event is there, but doesn't work like we want it to, for no appreciable gain. We're in no particular hurry here, so let's do it right.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Options in decreasing order of preference, pick one that suits you:
1. Go the extra mile for backporters, and move the event trigger change from PATCH 3 into 1.
OK will do.
2. Squash PATCH 1 into 3. Backporting trouble is obvious and easy to resolve: just drop member path. Feel free to point that out in the commit message.
3. Let the series stand as is. Backporting trouble is subtle and easy to miss: backporting just PATCH 1 is tempting, but screws up the event trigger. I wouldn't do it that way myself, but I'm not going to NAK usable upstream work because of such downstream concerns.
In case you pick 3:
Acked-by: Markus Armbruster <armbru@redhat.com>
participants (2)
-
Markus Armbruster -
Michael S. Tsirkin