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@xxxxxxxxxx> writes:
On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote:
Andreas Färber<afaerber@xxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list