Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event

[Osier Yang <jyang@redhat.com>]

On 2013年03月08日 03:15, Michael S. Tsirkin wrote:
> 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@redhat.com>  writes:
>>>>
>>>>> On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote:
>>>>>> Andreas Färber<afaerber@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@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@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.

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