From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44400) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDF5p-0004Go-F3 for qemu-devel@nongnu.org; Wed, 06 Mar 2013 09:13:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDF5i-0007gf-0x for qemu-devel@nongnu.org; Wed, 06 Mar 2013 09:13:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34199) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDF5h-0007gW-PN for qemu-devel@nongnu.org; Wed, 06 Mar 2013 09:13:09 -0500 Date: Wed, 6 Mar 2013 16:13:24 +0200 From: "Michael S. Tsirkin" Message-ID: <20130306141324.GA30001@redhat.com> References: <20130306130055.GA29446@redhat.com> <51374B42.2090405@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <51374B42.2090405@suse.de> 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: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Kevin Wolf , Anthony Liguori , Eduardo Habkost , Stefan Hajnoczi , qemu-devel@nongnu.org, Markus Armbruster , Gerd Hoffmann , laine@redhat.com, Paolo Bonzini , Luiz Capitulino 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 s390x > 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. > 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 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg