Re: [Qemu-devel] [PATCH v6 1/3] qdev: DEVICE_DELETED event

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

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

Donning my downstream hat for a minute.

Upstream after this series is fully applied:
* event always has data
* data always has member path
* data may have member device

Downstream after backport of just 1/3:
* event may have data
* data always has member device

Alternative downstream: event always has data, data 
* event always has data
* data may have member device

Smaller difference to upstream.  Would libvirt care?

>> > +
>> > +{ "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?

It doesn't matter upstream, because the anomaly created by 1/3 gets
removed by 3/3.  It matters downstream if 1/3 gets backported without
3/3.

I will not withhold my upstream ACK because of such downstream concerns.

[...]