On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <m...@redhat.com> writes: > > > On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: > >> Andreas Färber <afaer...@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" <m...@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 <m...@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. -- MST