From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48260) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDg3U-00064i-80 for qemu-devel@nongnu.org; Thu, 07 Mar 2013 14:00:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDg3O-0004E0-4U for qemu-devel@nongnu.org; Thu, 07 Mar 2013 14:00:40 -0500 Received: from cantor2.suse.de ([195.135.220.15]:49229 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDg3N-0004DW-O6 for qemu-devel@nongnu.org; Thu, 07 Mar 2013 14:00:34 -0500 Message-ID: <5138E3CD.8090105@suse.de> Date: Thu, 07 Mar 2013 20:00:29 +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> <5138921D.5050604@suse.de> <87ehfrcn60.fsf@blackfin.pond.sub.org> <20130307163540.GB29071@redhat.com> <87k3pj3yzh.fsf@blackfin.pond.sub.org> <20130307181229.GB30633@redhat.com> In-Reply-To: <20130307181229.GB30633@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" Cc: Kevin Wolf , Anthony Liguori , Eduardo Habkost , Stefan Hajnoczi , qemu-devel@nongnu.org, Markus Armbruster , Gerd Hoffmann , laine@redhat.com, Paolo Bonzini , Luiz Capitulino 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=E4rber writes: >>>> >>>>> 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 = }", dev->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 notificatio= n. We >>>>>>>>> should rather do this when the device is actually deleted - whi= ch >>>>>>>>> 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() inste= ad. >>>>>>>>> >>>>>>>>> Another thing to consider is what data to pass to the event: No= t 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 ca= nonical >>>>>>> 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:devic= e is >>>> 1:1. Path always works. Qdev ID only works when the user assigned = 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 cod= e >>>> simply doesn't emit one. >>>> >>>> You could blame the user; after all he could've used -nodefaults, an= d >>>> added the device himself, with an ID. >>>> >>>> I blame your design instead, which needlessly complicates the event'= s >>>> semantics: it gets emitted only for devices with a qdev ID. Which y= ou >>>> 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 indication >>> 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 t= he >> DEVICE_DELETED event to carry the device's canonical name: its QOM pat= h. >> >>>>> Anthony had rejected earlier QOM patches by Paolo related to qdev i= d, >>>>> 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 think >> there's a chance we won't need in the future, then we probably shouldn= '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 *trigger= *. >> >> 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. >> >> [...] >=20 > I still don't see why it's useful for anyone. For now I hear from the > libvirt guys that this patch does exactly what they need so I'll keep i= t > 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 in hw/qdev.c doesn't change. 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