From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDrR4-0004qs-Gb for qemu-devel@nongnu.org; Fri, 08 Mar 2013 02:09:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDrR2-0006Dw-NO for qemu-devel@nongnu.org; Fri, 08 Mar 2013 02:09:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:6970) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDrR2-0006Do-Fx for qemu-devel@nongnu.org; Fri, 08 Mar 2013 02:09:44 -0500 Message-ID: <51398EA3.5030004@redhat.com> Date: Fri, 08 Mar 2013 15:09:23 +0800 From: Osier Yang MIME-Version: 1.0 References: <51374B42.2090405@suse.de> <20130306141324.GA30001@redhat.com> <87y5dzlek4.fsf@blackfin.pond.sub.org> <20130307100740.GB5302@redhat.com> <5138921D.5050604@suse.de> <87ehfrcn60.fsf@blackfin.pond.sub.org> <20130307163540.GB29071@redhat.com> <87k3pj3yzh.fsf@blackfin.pond.sub.org> <20130307181229.GB30633@redhat.com> <5138E3CD.8090105@suse.de> <20130307191549.GA12543@redhat.com> In-Reply-To: <20130307191549.GA12543@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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" Cc: Kevin Wolf , Anthony Liguori , Eduardo Habkost , "libvir-list@redhat.com" , Stefan Hajnoczi , qemu-devel@nongnu.org, Markus Armbruster , Gerd Hoffmann , laine@redhat.com, Paolo Bonzini , Luiz Capitulino , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= On 2013=E5=B9=B403=E6=9C=8808=E6=97=A5 03:15, Michael S. Tsirkin wrote: > On Thu, Mar 07, 2013 at 08:00:29PM +0100, Andreas F=C3=A4rber wrote: >> Am 07.03.2013 19:12, schrieb Michael S. Tsirkin: >>> On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: >>>> "Michael S. Tsirkin" writes: >>>> >>>>> On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: >>>>>> Andreas F=C3=A4rber writes: >>>>>> >>>>>>> Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: >>>>>>>> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrot= e: >>>>>>>>> "Michael S. Tsirkin" writes: >>>>>>>>> >>>>>>>>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas F=C3=A4rber = 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 robus= t 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" >>>>>>>>>>>> >>>>>>>>>>>> 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)); >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I'm pretty sure this is the wrong place to fire the notificat= ion. We >>>>>>>>>>> should rather do this when the device is actually deleted - w= hich >>>>>>>>>>> qdev_free() does *not* actually guarantee, as criticized in t= he s390x >>>>>>>>>>> and unref'ing contexts. >>>>>>>>>>> I would suggest to place your code into device_unparent() ins= tead. >>>>>>>>>>> >>>>>>>>>>> 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 managemen= t 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. Thu= s, >>>>>>>>> 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 sti= ll 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? >>>>>> >>>>>> The path is the device's canonical name. Canonical means path:dev= ice is >>>>>> 1:1. Path always works. Qdev ID only works when the user assigne= d one. >>>>>> >>>>>> Funny case: board creates a hot-pluggable device by default (thus = no >>>>>> qdev ID), guest ejects it, what do you put into the event? Your c= ode >>>>>> simply doesn't emit one. >>>>>> >>>>>> You could blame the user; after all he could've used -nodefaults, = and >>>>>> added the device himself, with an ID. >>>>>> >>>>>> I blame your design instead, which needlessly complicates the even= t's >>>>>> semantics: it gets emitted only for devices with a qdev ID. Which= you >>>>>> neglected to document clearly, by the way. >>>>> >>>>> Good point, I'll document this. >>>>> >>>>>> If you put the path into the event, you can emit it always, which = is >>>>>> simpler. Feel free to throw in the qdev ID. >>>>> >>>>> I don't blame anyone. User not assigning an id is a clear indicati= on >>>>> that user does not care about the lifetime of this device. >>>>> >>>>>>>> Looks like maintainance hassle without real benefits? >>>>>> >>>>>> I can't see path being a greater maintenance hassle than ID. >>>>> >>>>> Sure, the less events we emit the less we need to support. >>>>> You want to expose all kind of internal events, >>>>> then management will come to depend on it and >>>>> we'll have to maintain them forever. >>>> >>>> Misunderstanding. I'm *not* asking for more events. I'm asking for= the >>>> DEVICE_DELETED event to carry the device's canonical name: its QOM p= ath. >>>> >>>>>>> Anthony had rejected earlier QOM patches by Paolo related to qdev= id, >>>>>>> saying it was deprecated in favor of those QOM paths. >>>>>> >>>>>> More reason to put the path into the event, not just the qdev ID. >>>>> >>>>> libvirt does not seems to want it there. We'll always be able to >>>>> add info but will never be able to remove info, keep it minimal. >>>> >>>> Yes, adding members to an event is easy. Doesn't mean we should do = it >>>> just for the heck of it. If we don't need a member now, and we thin= k >>>> there's a chance we won't need in the future, then we probably shoul= dn't >>>> add it now. >>>> >>>> I believe the chance of not needing the QOM path is effectively zero. >>>> >>>> Moreover, we'd add not just a member in this case, we'd add a *trigg= er*. >>>> >>>> Before: the event gets emitted only for devices with a qdev ID. >>>> >>>> After: the event gets emitted for all devices. >>>> >>>> I very much prefer the latter, because it's simpler. >>>> >>>> [...] >>> >>> I still don't see why it's useful for anyone. For now I hear from th= e >>> libvirt guys that this patch does exactly what they need so I'll keep= it >>> simple. You are welcome to send a follow-up patch adding a path >>> and more triggers, I won't object. >> >> Well, the libvirt guys have been told to poll using qom-list, which >> needs the path, not an ID. Using it in both places would make it >> symmetrical - that may qualify as useful. >> (I'm not aware of any id -> path lookup QMP command.) >> >> Nontheless, you can retain my Reviewed-by on v4+ as long as the code i= n >> hw/qdev.c doesn't change. >> >> Andreas > > I suggested retrying device_del, this has an advantage of working > on more qemu version. I'm wondering if it could be long time to wait for the device_del completes (AFAIK from previous bugs, it can be, though it should be fine for most of the cases). If it's too long, it will be a problem for management, because it looks like hanging. We can have a timeout for the device_del in libvirt, but the problem is the device_del can be still in progress by qemu, which could cause the inconsistency. Unless qemu has some command to cancel the device_del. Osier