Patchwork [v4] qdev: DEVICE_DELETED event

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 7, 2013, 6:46 p.m.
Message ID <20130307184647.GA31012@redhat.com>
Download mbox | patch
Permalink /patch/225896/
State New
Headers show

Comments

Michael S. Tsirkin - March 7, 2013, 6:46 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.

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(-)
Markus Armbruster - March 7, 2013, 7:57 p.m.
"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.
Michael S. Tsirkin - March 7, 2013, 8:23 p.m.
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
Markus Armbruster - March 8, 2013, 7:58 a.m.
"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.

[...]
Anthony Liguori - March 8, 2013, 1:36 p.m.
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
Michael S. Tsirkin - March 10, 2013, 9:30 a.m.
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.
Michael S. Tsirkin - March 11, 2013, 5:39 p.m.
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}
Anthony Liguori - March 11, 2013, 7:26 p.m.
"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
Michael S. Tsirkin - March 11, 2013, 7:35 p.m.
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.

Patch

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