Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event

[Markus Armbruster]

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

> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote:
>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin:
>> > libvirt has a long-standing bug: when removing the device,
>> > it can request removal but does not know when does the
>> > removal complete. Add an event so we can fix this in a robust way.
>> > 
>> > Signed-off-by: Michael S. Tsirkin <address@hidden>
>> 
>> Sounds like a good idea to me. :)
>> 
>> [...]
>> > diff --git a/hw/qdev.c b/hw/qdev.c
>> > index 689cd54..f30d251 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;
>> > @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev)
>> >  /* Unlink device from bus and free the structure.  */
>> >  void qdev_free(DeviceState *dev)
>> >  {
>> > +    if (dev->id) {
>> > +        QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id);
>> > +        monitor_protocol_event(QEVENT_DEVICE_DELETED, data);
>> > +        qobject_decref(data);
>> > +    }
>> >      object_unparent(OBJECT(dev));
>> >  }
>> >  
>> 
>> I'm pretty sure this is the wrong place to fire the notification. We
>> should rather do this when the device is actually deleted - which
>> qdev_free() does *not* actually guarantee, as criticized in the s390x
>> and unref'ing contexts.
>> I would suggest to place your code into device_unparent() instead.
>> 
>> Another thing to consider is what data to pass to the event: Not all
>> devices have an ID.
>
> If they don't they were not created by management so management is
> probably not interested in them being removed.
>
> We could always add a 'path' key later if this assumption
> proves incorrect.

In old qdev, ID was all we had, because paths were busted.  Thus,
management had no choice but use IDs.

If I understand modern qdev correctly, we got a canonical path.  Old
APIs like device_del still accept only ID.  Should new APIs still be
designed that way?  Or should they always accept / provide the canonical
path, plus optional ID for convenience?

>> We should still have a canonical path when we fire
>> this event in either qdev_free() or in device_unparent() before the if
>> (dev->parent_bus) block though. That would be a question for Anthony,
>> not having a use case for the event I am indifferent there.
>> 
>> Further, thinking of objects such as virtio-rng backends or future
>> blockdev/chardev objects, might it make sense to turn this into a
>> generic object deletion event rather than a device event?
>> 
>> Andreas
>
> Backend deletion doesn't normally have guest interaction right?
> So why do we need an event?

We need an event because device_del may send its reply before it
completes the job.

device_del does that when it deletion needs to interact with the guest,
which can take unbounded time.

Conversely, we don't need an event when a QMP always completes the job
(as far as observable by the QMP client) before it sends its reply.  Off
hand, I can't see why backend deletion would do anything else.

I'm always reluctant to abstract when there are fewer than two
different, concrete things to abstract from.  Right now, we got just
one: device models.