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" <address@hidden> 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 <address@hidden>
> > ---
> >  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"?