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 <m...@redhat.com>
>
> 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.
> 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?
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
