[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event |
Date: |
Thu, 7 Mar 2013 12:07:40 +0200 |
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?
Looks like maintainance hassle without real benefits?
> >> 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.
- [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 <=
- 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, 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, 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