On Thu, Mar 07, 2013 at 08:00:29PM +0100, Andreas Färber 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"<mst(a)redhat.com> writes: >>> >>>> On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: >>>>> Andreas Färber<afaerber(a)suse.de> 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"<mst(a)redhat.com> writes: >>>>>>>> >>>>>>>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber 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<mst(a)redhat.com> >>>>>>>>>> >>>>>>>>>> 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 = 0; >>>>>>>>>>> static bool qdev_hot_added = 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 = 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 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. >>>>>>>>>> >>>>>>>>>> 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 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:device 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 code >>>>> 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 event'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 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 the >>> DEVICE_DELETED event to carry the device's canonical name: its QOM path. >>> >>>>>> 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 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. >>> >>> [...] >> >> 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 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 in > hw/qdev.c doesn't change. > > Andreas I suggested retrying device_del, this has an advantage of working on more qemu version.