[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event |
Date: |
Thu, 07 Mar 2013 20:00:29 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130215 Thunderbird/17.0.3 |
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" <address@hidden> writes:
>>
>>> On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote:
>>>> Andreas Färber <address@hidden> 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" <address@hidden> 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 <address@hidden>
>>>>>>>>>
>>>>>>>>> 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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
- [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Michael S. Tsirkin, 2013/03/06
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Andreas Färber, 2013/03/06
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Michael S. Tsirkin, 2013/03/06
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Markus Armbruster, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Michael S. Tsirkin, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Andreas Färber, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Markus Armbruster, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Michael S. Tsirkin, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Markus Armbruster, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Michael S. Tsirkin, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event,
Andreas Färber <=
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Michael S. Tsirkin, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Osier Yang, 2013/03/08
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Markus Armbruster, 2013/03/08
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Jiri Denemark, 2013/03/08
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Osier Yang, 2013/03/08
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Osier Yang, 2013/03/08
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Markus Armbruster, 2013/03/08
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Markus Armbruster, 2013/03/07
- Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Michael S. Tsirkin, 2013/03/07
Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event, Eric Blake, 2013/03/06