[virt-tools-list] [PATCH] Document that ff callbacks need to be invoked from a clean stack

[Daniel P. Berrange]

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 at 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


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|