From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDac6-000661-KB for qemu-devel@nongnu.org; Thu, 07 Mar 2013 08:12:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDac3-0000WT-2J for qemu-devel@nongnu.org; Thu, 07 Mar 2013 08:12:02 -0500 Received: from cantor2.suse.de ([195.135.220.15]:34877 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDac2-0000WH-Ow for qemu-devel@nongnu.org; Thu, 07 Mar 2013 08:11:58 -0500 Message-ID: <5138921D.5050604@suse.de> Date: Thu, 07 Mar 2013 14:11:57 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <20130306130055.GA29446@redhat.com> <51374B42.2090405@suse.de> <20130306141324.GA30001@redhat.com> <87y5dzlek4.fsf@blackfin.pond.sub.org> <20130307100740.GB5302@redhat.com> In-Reply-To: <20130307100740.GB5302@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 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: "Michael S. Tsirkin" , Markus Armbruster Cc: Kevin Wolf , Anthony Liguori , Eduardo Habkost , Stefan Hajnoczi , qemu-devel@nongnu.org, Luiz Capitulino , Gerd Hoffmann , laine@redhat.com, Paolo Bonzini Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: > On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" writes: >> >>> 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. >>>>> >>>>> Signed-off-by: Michael S. Tsirkin >>>> >>>> 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" >>>>> =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 }", d= ev->id); >>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >>>>> + qobject_decref(data); >>>>> + } >>>>> object_unparent(OBJECT(dev)); >>>>> } >>>>> =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. >>>> >>>> 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 canonic= al >> path, plus optional ID for convenience? >=20 > What are advantages of exposing the path to users in this way? > Looks like maintainance hassle without real benefits? Anthony had rejected earlier QOM patches by Paolo related to qdev id, saying it was deprecated in favor of those QOM paths. >>>> 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. O= ff >> 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. Not quite: It's about unparenting hook and object deletion, which are both not limited to devices. But if the ID based approach gets accepted by Anthony then we can still introduce an OBJECT_DELETED event once someone needs it. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg