| Message ID | 20130307184647.GA31012@redhat.com |
|---|---|
| State | New |
| Headers | show |
"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> Speaking as the acting QMP maintainer, just to avoid misunderstandings: there's disagreement on the event's design, namely when it should fire, and how it should name the device. I don't want the discussion preempted by a commit.
On Thu, Mar 07, 2013 at 08:57:52PM +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> > > Speaking as the acting QMP maintainer, just to avoid misunderstandings: > there's disagreement on the event's design, namely when it should fire, > and how it should name the device. I don't want the discussion > preempted by a commit. Yes, you are asking for more functionality, but can I add this in a follow-up commit please? I prefer this patch as is, as it can be backported to stable branches and downstreams. Upstream a follow up patch can add fields and more triggers which won't apply to any downstreams. This patch will help resolve a set of serious long-standing bugs, see e.g. https://bugzilla.redhat.com/show_bug.cgi?id=813748
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Thu, Mar 07, 2013 at 08:57:52PM +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> >> >> Speaking as the acting QMP maintainer, just to avoid misunderstandings: >> there's disagreement on the event's design, namely when it should fire, >> and how it should name the device. I don't want the discussion >> preempted by a commit. > > Yes, you are asking for more functionality, but can I add this in a > follow-up commit please? I prefer this patch as is, as it can be > backported to stable branches and downstreams. Upstream a follow up > patch can add fields and more triggers which won't apply to any > downstreams. If you want to address my review comments in a separate patch, go right ahead. Please post both together as a series, for coherent review and to simplify patch tracking. I'm asking for two things: 1. Event member path. Fair to call this "more functionality". I agree that backporting it to pre-QOM versions isn't practical. 2. Sane event trigger condition: on any device deletion, not just when the device happens to have a qdev ID. This isn't "more", it's "different". I'd definitely backport this part, because: * I abhor subtle semantic differences to upstream like a different event trigger. * Backporting it reduces the difference to event member path missing. Syntactic and in-your-face. * Without member path, the event triggered by deleting a device without a qdev ID can't tell us which device went away. But you can find out using the polling code you need anyway. Thus, the event trigger is not only simpler and consistent with upstream, it can also be more useful. [...]
Markus Armbruster <armbru@redhat.com> writes: > "Michael S. Tsirkin" <mst@redhat.com> writes: > >> On Thu, Mar 07, 2013 at 08:57:52PM +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> >>> >>> Speaking as the acting QMP maintainer, just to avoid misunderstandings: >>> there's disagreement on the event's design, namely when it should fire, >>> and how it should name the device. I don't want the discussion >>> preempted by a commit. >> >> Yes, you are asking for more functionality, but can I add this in a >> follow-up commit please? I prefer this patch as is, as it can be >> backported to stable branches and downstreams. Upstream a follow up >> patch can add fields and more triggers which won't apply to any >> downstreams. > > If you want to address my review comments in a separate patch, go right > ahead. Please post both together as a series, for coherent review and > to simplify patch tracking. > > I'm asking for two things: > > 1. Event member path. Fair to call this "more functionality". I agree > that backporting it to pre-QOM versions isn't practical. > > 2. Sane event trigger condition: on any device deletion, not just when > the device happens to have a qdev ID. This isn't "more", it's > "different". Ack. Regards, Anthony Liguori
On Fri, Mar 08, 2013 at 08:58:43AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Thu, Mar 07, 2013 at 08:57:52PM +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> > >> > >> Speaking as the acting QMP maintainer, just to avoid misunderstandings: > >> there's disagreement on the event's design, namely when it should fire, > >> and how it should name the device. I don't want the discussion > >> preempted by a commit. > > > > Yes, you are asking for more functionality, but can I add this in a > > follow-up commit please? I prefer this patch as is, as it can be > > backported to stable branches and downstreams. Upstream a follow up > > patch can add fields and more triggers which won't apply to any > > downstreams. > > If you want to address my review comments in a separate patch, go right > ahead. Please post both together as a series, for coherent review and > to simplify patch tracking. Sure. > I'm asking for two things: > > 1. Event member path. Fair to call this "more functionality". I agree > that backporting it to pre-QOM versions isn't practical. > > 2. Sane event trigger condition: on any device deletion, not just when > the device happens to have a qdev ID. This isn't "more", it's > "different". > > I'd definitely backport this part, because: > > * I abhor subtle semantic differences to upstream like a different > event trigger. > > * Backporting it reduces the difference to event member path missing. > Syntactic and in-your-face. > > * Without member path, the event triggered by deleting a device > without a qdev ID can't tell us which device went away. But you > can find out using the polling code you need anyway. Thus, the > event trigger is not only simpler and consistent with upstream, it > can also be more useful. > > [...] Will do, thanks for the comments.
On Fri, Mar 08, 2013 at 07:36:28AM -0600, Anthony Liguori wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > >> On Thu, Mar 07, 2013 at 08:57:52PM +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> > >>> > >>> Speaking as the acting QMP maintainer, just to avoid misunderstandings: > >>> there's disagreement on the event's design, namely when it should fire, > >>> and how it should name the device. I don't want the discussion > >>> preempted by a commit. > >> > >> Yes, you are asking for more functionality, but can I add this in a > >> follow-up commit please? I prefer this patch as is, as it can be > >> backported to stable branches and downstreams. Upstream a follow up > >> patch can add fields and more triggers which won't apply to any > >> downstreams. > > > > If you want to address my review comments in a separate patch, go right > > ahead. Please post both together as a series, for coherent review and > > to simplify patch tracking. > > > > I'm asking for two things: > > > > 1. Event member path. Fair to call this "more functionality". I agree > > that backporting it to pre-QOM versions isn't practical. > > > > 2. Sane event trigger condition: on any device deletion, not just when > > the device happens to have a qdev ID. This isn't "more", it's > > "different". > > Ack. > > Regards, > > Anthony Liguori So how does one get the path that you require? ERROR:qom/object.c:1011:object_get_canonical_path: assertion failed: (prop != NULL) Program received signal SIGABRT, Aborted. [Switching to Thread 0x7fffeffff700 (LWP 29475)] 0x00007ffff587d8a5 in raise () from /lib64/libc.so.6 Missing separate debuginfos, use: debuginfo-install SDL-1.2.14-2.el6.x86_64 glib2-2.22.5-6.el6.x86_64 glibc-2.12-1.80.el6_3.3.x86_64 libX11-1.3-2.el6.x86_64 libXau-1.0.5-1.el6.x86_64 libpng-1.2.48-1.el6_2.x86_64 libxcb-1.5-1.el6.x86_64 ncurses-libs-5.7-3.20090208.el6.x86_64 pixman-0.18.4-1.el6_0.1.x86_64 zlib-1.2.3-27.el6.x86_64 (gdb) where #0 0x00007ffff587d8a5 in raise () from /lib64/libc.so.6 #1 0x00007ffff587f085 in abort () from /lib64/libc.so.6 #2 0x00007ffff737ca7f in g_assertion_message () from /lib64/libglib-2.0.so.0 #3 0x00007ffff737d020 in g_assertion_message_expr () from /lib64/libglib-2.0.so.0 #4 0x00007ffff7deef76 in object_get_canonical_path (obj=0x7ffff8f1a010) at qom/object.c:1011 #5 0x00007ffff7d7abcd in device_unparent (obj=0x7ffff8f1a010) at hw/qdev.c:785 #6 0x00007ffff7dee30f in object_unparent (obj=0x7ffff8f1a010) at qom/object.c:370 #7 0x00007ffff7d093b0 in acpi_piix_eject_slot (s=0x7ffff8ed6bd0, slots=<value optimized out>) at hw/acpi_piix4.c:306 #8 0x00007ffff7e7ae20 in access_with_adjusted_size (addr=8, value=0x7fffefffec88, size=4, access_size_min=<value optimized out>, access_size_max=<value optimized out>, access= 0x7ffff7e7c590 <memory_region_write_accessor>, opaque=0x7ffff8ed73d8) at /home/mst/scm/qemu/memory.c:364 #9 0x00007ffff7e7b44b in memory_region_iorange_write (iorange=<value optimized out>, offset=<value optimized out>, width=<value optimized out>, data=128) at /home/mst/scm/qemu/memory.c:439 #10 0x00007ffff7e7a39c in kvm_handle_io (env=0x7ffff8d27a80) at /home/mst/scm/qemu/kvm-all.c:1432 #11 kvm_cpu_exec (env=0x7ffff8d27a80) at /home/mst/scm/qemu/kvm-all.c:1581 #12 0x00007ffff7e28971 in qemu_kvm_cpu_thread_fn (arg=0x7ffff8d27a80) at /home/mst/scm/qemu/cpus.c:759 #13 0x00007ffff681f851 in start_thread () from /lib64/libpthread.so.0 #14 0x00007ffff59326dd in clone () from /lib64/libc.so.6 (gdb) frame 4 #4 0x00007ffff7deef76 in object_get_canonical_path (obj=0x7ffff8f1a010) at qom/object.c:1011 1011 g_assert(prop != NULL); (gdb) p obj $1 = (Object *) 0x7ffff8f1a010 (gdb) p (DeviceState*)obj $2 = (DeviceState *) 0x7ffff8f1a010 (gdb) p $2 $3 = (DeviceState *) 0x7ffff8f1a010 (gdb) p *$2 $4 = {parent_obj = {class = 0x7ffff8ede8f0, free = 0x7ffff735f5b0 <g_free>, properties = { tqh_first = 0x7ffff8eec8a0, tqh_last = 0x7ffff8f1dc00}, ref = 1, parent = 0x7ffff8c81d00}, id = 0x7ffff8ba6710 "bah", realized = true, opts = 0x7ffff8ba66c0, hotplugged = 0, parent_bus = 0x0, num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0, child_bus = {lh_first = 0x0}, num_child_bus = 0, instance_id_alias = -1, alias_required_for_version = 0} (gdb) $5 = {parent_obj = {class = 0x7ffff8ede8f0, free = 0x7ffff735f5b0 <g_free>, properties = { tqh_first = 0x7ffff8eec8a0, tqh_last = 0x7ffff8f1dc00}, ref = 1, parent = 0x7ffff8c81d00}, id = 0x7ffff8ba6710 "bah", realized = true, opts = 0x7ffff8ba66c0, hotplugged = 0, parent_bus = 0x0, num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0, child_bus = {lh_first = 0x0}, num_child_bus = 0, instance_id_alias = -1, alias_required_for_version = 0} (gdb) $6 = {parent_obj = {class = 0x7ffff8ede8f0, free = 0x7ffff735f5b0 <g_free>, properties = { tqh_first = 0x7ffff8eec8a0, tqh_last = 0x7ffff8f1dc00}, ref = 1, parent = 0x7ffff8c81d00}, id = 0x7ffff8ba6710 "bah", realized = true, opts = 0x7ffff8ba66c0, hotplugged = 0, parent_bus = 0x0, num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0, child_bus = {lh_first = 0x0}, num_child_bus = 0, instance_id_alias = -1, alias_required_for_version = 0}
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Fri, Mar 08, 2013 at 07:36:28AM -0600, Anthony Liguori wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >> > "Michael S. Tsirkin" <mst@redhat.com> writes: >> > >> >> On Thu, Mar 07, 2013 at 08:57:52PM +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> >> >>> >> >>> Speaking as the acting QMP maintainer, just to avoid misunderstandings: >> >>> there's disagreement on the event's design, namely when it should fire, >> >>> and how it should name the device. I don't want the discussion >> >>> preempted by a commit. >> >> >> >> Yes, you are asking for more functionality, but can I add this in a >> >> follow-up commit please? I prefer this patch as is, as it can be >> >> backported to stable branches and downstreams. Upstream a follow up >> >> patch can add fields and more triggers which won't apply to any >> >> downstreams. >> > >> > If you want to address my review comments in a separate patch, go right >> > ahead. Please post both together as a series, for coherent review and >> > to simplify patch tracking. >> > >> > I'm asking for two things: >> > >> > 1. Event member path. Fair to call this "more functionality". I agree >> > that backporting it to pre-QOM versions isn't practical. >> > >> > 2. Sane event trigger condition: on any device deletion, not just when >> > the device happens to have a qdev ID. This isn't "more", it's >> > "different". >> >> Ack. >> >> Regards, >> >> Anthony Liguori > > > So how does one get the path that you require? > > ERROR:qom/object.c:1011:object_get_canonical_path: assertion failed: > (prop != NULL) Can you share your patch? This means something is wrong. All devices have a canonical path. Regards, Anthony Liguori
On Mon, Mar 11, 2013 at 02:26:58PM -0500, Anthony Liguori wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Fri, Mar 08, 2013 at 07:36:28AM -0600, Anthony Liguori wrote: > >> Markus Armbruster <armbru@redhat.com> writes: > >> > >> > "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > > >> >> On Thu, Mar 07, 2013 at 08:57:52PM +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> > >> >>> > >> >>> Speaking as the acting QMP maintainer, just to avoid misunderstandings: > >> >>> there's disagreement on the event's design, namely when it should fire, > >> >>> and how it should name the device. I don't want the discussion > >> >>> preempted by a commit. > >> >> > >> >> Yes, you are asking for more functionality, but can I add this in a > >> >> follow-up commit please? I prefer this patch as is, as it can be > >> >> backported to stable branches and downstreams. Upstream a follow up > >> >> patch can add fields and more triggers which won't apply to any > >> >> downstreams. > >> > > >> > If you want to address my review comments in a separate patch, go right > >> > ahead. Please post both together as a series, for coherent review and > >> > to simplify patch tracking. > >> > > >> > I'm asking for two things: > >> > > >> > 1. Event member path. Fair to call this "more functionality". I agree > >> > that backporting it to pre-QOM versions isn't practical. > >> > > >> > 2. Sane event trigger condition: on any device deletion, not just when > >> > the device happens to have a qdev ID. This isn't "more", it's > >> > "different". > >> > >> Ack. > >> > >> Regards, > >> > >> Anthony Liguori > > > > > > So how does one get the path that you require? > > > > ERROR:qom/object.c:1011:object_get_canonical_path: assertion failed: > > (prop != NULL) > > Can you share your patch? This means something is wrong. All devices > have a canonical path. > > Regards, > > Anthony Liguori I figured it out - we were trying to get the path after the device was detached from the parent. We'll just have to calculate the path before unparenting and pass it in.
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 ##
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> --- At Anthony's request, reposting with a corrected subject. Sorry about the noise. 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 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(-)