On Tue, Oct 04, 2011 at 09:00:17AM +0100, Daniel P. Berrange wrote:
On Mon, Oct 03, 2011 at 10:41:17PM +0200, Guido Günther wrote:
On Tue, Aug 16, 2011 at 02:24:53PM +0200, Guido Günther wrote:
Hi Daniel, On Sat, Aug 13, 2011 at 08:57:45PM -0700, Daniel P. Berrange wrote:
On Fri, Aug 12, 2011 at 11:54:28PM +0200, Guido Günther wrote: [..snip..] In the default libvirt event loop, the 'ff' callback is always invoked from a "clean" stack in the event loop, so you never have this problem with re-entrancy.
Working around this by removing the locks from virNetSocketRemoveIOCallback leads to another deadlock:
Yeah this is not a viable approach. Sure. This was only to see what else fails.
I didn't see a simple way to fix this but would welcome any suggestions.
IMHO we just have to document that event loop implementations should make sure that the 'ff' callbacks are always invoked from a clean stack. In the case of virt-viewer, this means changing it to register a g_idle callback function to invoke the 'ff' callback.
Patch for virt-viewer attached. I'll come up with a doc patch for libvirt once I have a bit more time.
As promised here's the patch for libvirt. O.k. to apply? Cheers, -- Guido
From c2e2be4e377871ace12bb7adfde130e9b8779ab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx@sigxcpu.org> Date: Mon, 3 Oct 2011 22:24:13 +0200 Subject: [PATCH] Document that ff callbacks need to be invoked from a clean stack.
--- include/libvirt/libvirt.h.in | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a3c581d..bd7a0f7 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2243,13 +2243,15 @@ typedef void (*virEventHandleCallback)(int watch, int fd, int events, void *opaq * @opaque: user data to pass to the callback * @ff: the callback invoked to free opaque data blob * - * Part of the EventImpl, this callback Adds a file handle callback to + * Part of the EventImpl, this callback adds a file handle callback to * listen for specific events. The same file handle can be registered * multiple times provided the requested event sets are non-overlapping * * If the opaque user data requires free'ing when the handle * is unregistered, then a 2nd callback can be supplied for - * this purpose. + * this purpose. This callback needs to be invoked from a clean stack. + * If 'ff' callbacks are invoked directly from the virEventRemoveHandleFunc + * they will likely deadlock in libvirt. * * Returns a handle watch number to be used for updating * and unregistering for events
ACK Pushed. Thanks. -- Guido