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

["Andreas Färber" <afaerber@suse.de>]

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?
> Looks like maintainance hassle without real benefits?

Anthony had rejected earlier QOM patches by Paolo related to qdev id,
saying it was deprecated in favor of those QOM paths.

>>>> We should still have a canonical path when we fire
>>>> this event in either qdev_free() or in device_unparent() before the if
>>>> (dev->parent_bus) block though. That would be a question for Anthony,
>>>> not having a use case for the event I am indifferent there.
>>>>
>>>> Further, thinking of objects such as virtio-rng backends or future
>>>> blockdev/chardev objects, might it make sense to turn this into a
>>>> generic object deletion event rather than a device event?
>>>>
>>>> Andreas
>>>
>>> Backend deletion doesn't normally have guest interaction right?
>>> So why do we need an event?
>>
>> We need an event because device_del may send its reply before it
>> completes the job.
>>
>> device_del does that when it deletion needs to interact with the guest,
>> which can take unbounded time.
>>
>> Conversely, we don't need an event when a QMP always completes the job
>> (as far as observable by the QMP client) before it sends its reply.  Off
>> hand, I can't see why backend deletion would do anything else.
>>
>> I'm always reluctant to abstract when there are fewer than two
>> different, concrete things to abstract from.  Right now, we got just
>> one: device models.

Not quite: It's about unparenting hook and object deletion, which are
both not limited to devices.

But if the ID based approach gets accepted by Anthony then we can still
introduce an OBJECT_DELETED event once someone needs it.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg