From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDhRW-0002W3-SS for qemu-devel@nongnu.org; Thu, 07 Mar 2013 15:29:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDhRT-0002Qj-8b for qemu-devel@nongnu.org; Thu, 07 Mar 2013 15:29:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35040) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDhRT-0002Pk-0N for qemu-devel@nongnu.org; Thu, 07 Mar 2013 15:29:31 -0500 Date: Thu, 7 Mar 2013 22:29:46 +0200 From: "Michael S. Tsirkin" Message-ID: <20130307202946.GA13243@redhat.com> 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> <877gljvu8n.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <877gljvu8n.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 09:18:48PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: >=20 > > On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: > >> "Michael S. Tsirkin" writes: > >>=20 > >> > On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: > >> >> Andreas F=E4rber writes: > >> >>=20 > >> >> > Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: > >> >> >> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wr= ote: > >> >> >>> "Michael S. Tsirkin" writes: > >> >> >>> > >> >> >>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas F=E4rber w= rote: > >> >> >>>>> 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 rob= ust 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 *de= v) > >> >> >>>>>> /* 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, dat= a); > >> >> >>>>>> + qobject_decref(data); > >> >> >>>>>> + } > >> >> >>>>>> object_unparent(OBJECT(dev)); > >> >> >>>>>> } > >> >> >>>>>> =20 > >> >> >>>>> > >> >> >>>>> I'm pretty sure this is the wrong place to fire the notific= ation. 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() i= nstead. > >> >> >>>>> > >> >> >>>>> 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 managem= ent 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. T= hus, > >> >> >>> management had no choice but use IDs. > >> >> >>> > >> >> >>> If I understand modern qdev correctly, we got a canonical pat= h. Old > >> >> >>> APIs like device_del still accept only ID. Should new APIs s= till be > >> >> >>> designed that way? Or should they always accept / provide > >> >> >>> the canonical > >> >> >>> path, plus optional ID for convenience? > >> >> >>=20 > >> >> >> What are advantages of exposing the path to users in this way? > >> >>=20 > >> >> The path is the device's canonical name. Canonical means path:de= vice is > >> >> 1:1. Path always works. Qdev ID only works when the user assign= ed one. > >> >>=20 > >> >> 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 = code > >> >> simply doesn't emit one. > >> >>=20 > >> >> You could blame the user; after all he could've used -nodefaults,= and > >> >> added the device himself, with an ID. > >> >>=20 > >> >> I blame your design instead, which needlessly complicates the eve= nt's > >> >> semantics: it gets emitted only for devices with a qdev ID. Whic= h 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 indicat= ion > >> > that user does not care about the lifetime of this device. > >> > > >> >> >> Looks like maintainance hassle without real benefits? > >> >>=20 > >> >> 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. > >>=20 > >> 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. > >>=20 > >> >> > Anthony had rejected earlier QOM patches by Paolo related to qd= ev id, > >> >> > saying it was deprecated in favor of those QOM paths. > >> >>=20 > >> >> 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. > >>=20 > >> 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. > >>=20 > >> I believe the chance of not needing the QOM path is effectively zero. > >>=20 > >> Moreover, we'd add not just a member in this case, we'd add a *trigg= er*. > >>=20 > >> Before: the event gets emitted only for devices with a qdev ID. > >>=20 > >> After: the event gets emitted for all devices. > >>=20 > >> 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 th= e > > libvirt guys that this patch does exactly what they need so I'll keep= it > > simple. >=20 > You're keeping it simple only in the sense of keeping it as it is. I > think it's not as simple as it easily could be. Specifically, I want > you to simplify the event's trigger, and add a path member. I'm > convinced we'll have to do this anyway, so why not do it right right > away, and simplify the future. >=20 > This is based on my limited understanding of qdev and QOM. If the QOM > folks tell me that we shouldn't use QOM paths to name the device, I'll > gladly admit to be wrong, and retract my request. >=20 > > You are welcome to send a follow-up patch adding a path > > and more triggers, I won't object. >=20 > The usual protocol for patch submission is maintainer (that would be me= , > until Luiz is back) reviews, submitter (that would be you) addresses > review comments, within reason. >=20 > I believe my request is well within reason. As a patch submitter, I've > jumped through hoops I found less reasonable (to put it charitably) man= y > times. >=20 > You, as a maintainer, should know that asking the maintainer to address > his review comments himself doesn't scale. >=20 > If you think I'm an unreasonable maintainer, feel free to ignore me and > try again when Luiz is back. I think it's a reasonable request, just not sure it's the right thing to do and don't see why it should block this patch when we can add functionality in a follow-up patch. --=20 MST