From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45806) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDXjU-0004Q5-0g for qemu-devel@nongnu.org; Thu, 07 Mar 2013 05:07:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDXjR-0006vk-00 for qemu-devel@nongnu.org; Thu, 07 Mar 2013 05:07:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:4968) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDXjQ-0006vg-OI for qemu-devel@nongnu.org; Thu, 07 Mar 2013 05:07:24 -0500 Date: Thu, 7 Mar 2013 12:07:40 +0200 From: "Michael S. Tsirkin" Message-ID: <20130307100740.GB5302@redhat.com> References: <20130306130055.GA29446@redhat.com> <51374B42.2090405@suse.de> <20130306141324.GA30001@redhat.com> <87y5dzlek4.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <87y5dzlek4.fsf@blackfin.pond.sub.org> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , Anthony Liguori , Eduardo Habkost , Stefan Hajnoczi , qemu-devel@nongnu.org, Luiz Capitulino , Gerd Hoffmann , laine@redhat.com, Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: >=20 > > On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas F=E4rber 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. > >> >=20 > >> > Signed-off-by: Michael S. Tsirkin > >>=20 > >> Sounds like a good idea to me. :) > >>=20 > >> [...] > >> > 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" > >> > =20 > >> > int qdev_hotplug =3D 0; > >> > static bool qdev_hot_added =3D 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 =3D qobject_from_jsonf("{ 'device': %s }", = dev->id); > >> > + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > >> > + qobject_decref(data); > >> > + } > >> > object_unparent(OBJECT(dev)); > >> > } > >> > =20 > >>=20 > >> 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 s390= x > >> and unref'ing contexts. > >> I would suggest to place your code into device_unparent() instead. > >>=20 > >> 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. >=20 > In old qdev, ID was all we had, because paths were busted. Thus, > management had no choice but use IDs. >=20 > 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 canonica= l > 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. > >>=20 > >> 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? > >>=20 > >> Andreas > > > > Backend deletion doesn't normally have guest interaction right? > > So why do we need an event? >=20 > We need an event because device_del may send its reply before it > completes the job. >=20 > device_del does that when it deletion needs to interact with the guest, > which can take unbounded time. >=20 > 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. Of= f > hand, I can't see why backend deletion would do anything else. >=20 > 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.