On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <m...@redhat.com> 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 <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. > > 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?
What are advantages of exposing the path to users in this way? Looks like maintainance hassle without real benefits? > >> 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.