1:05 p.m.
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(a)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
1:05 p.m.
New subject: [libvirt] [PATCHv5 1/3] qdev: 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.
Signed-off-by: Michael S. Tsirkin <mst(a)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
1:06 p.m.
New subject: [libvirt] [PATCHv5 2/3] qom: pass original path to unparent method
We need to know the original path since unparenting loses this state.
Signed-off-by: Michael S. Tsirkin <mst(a)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
1:06 p.m.
New subject: [libvirt] [PATCHv5 3/3] qmp: add path to device_deleted event
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(a)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
2:45 a.m.
New subject: [libvirt] [Qemu-devel] [PATCHv5 0/3] DEVICE_DELETED event
Anthony asked for a space between "PATCH" and "v<N>" in the
subject.
Please try to remember next time.
"Michael S. Tsirkin" <mst(a)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(a)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(a)redhat.com>
11:41 a.m.
New subject: [libvirt] [Qemu-devel] [PATCHv5 0/3] DEVICE_DELETED event
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(a)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(a)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(a)redhat.com>
4302
days inactive
4304
days old
5 comments
2 participants
participants (2)
-
Markus Armbruster -
Michael S. Tsirkin