[libvirt] [libvirt-glib] RFC: delay event handle freeing to avoid dead lock

[Daniel P. Berrange]

On Fri, Sep 30, 2011 at 07:41:41PM +0200, Marc-André Lureau wrote:
> Can be reproduced with the updated test.
> ---
>  examples/conn-test.c              |   20 +++++++++++++++-----
>  libvirt-glib/libvirt-glib-event.c |   17 +++++++++++++----
>  2 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/examples/conn-test.c b/examples/conn-test.c
> index 08045a4..8ea5ad7 100644
> --- a/examples/conn-test.c
> +++ b/examples/conn-test.c
> @@ -31,12 +31,19 @@ do_connection_open(GObject *source,
>  {
>      GVirConnection *conn = GVIR_CONNECTION(source);
>      GError *err = NULL;
> -    GMainLoop *loop = opaque;
>  
>      if (!gvir_connection_open_finish(conn, res, &err)) {
>          g_error("%s", err->message);
>      }
>      g_print("Connected to libvirt\n");
> +    g_object_unref(conn);
> +}
> +
> +static void quit(gpointer data,
> +                 GObject *where_the_object_was)
> +{
> +    GMainLoop *loop = data;
> +
>      g_main_loop_quit(loop);
>  }
>  
> @@ -47,19 +54,22 @@ int main(int argc, char **argv)
>  
>      gvir_init_object(&argc, &argv);
>  
> -    if (argc != 2)
> +    if (argc != 2) {
>          g_error("syntax: %s URI", argv[0]);
> -
> -    conn = gvir_connection_new(argv[1]);
> +        return 1;
> +    }
>  
>      loop = g_main_loop_new(g_main_context_default(),
>                             TRUE);
>  
> -    gvir_connection_open_async(conn, NULL, do_connection_open, loop);
> +    conn = gvir_connection_new(argv[1]);
> +    g_object_weak_ref(G_OBJECT(conn), quit, loop);
> +
>      gvir_connection_open_async(conn, NULL, do_connection_open, loop);
>  
>      g_main_loop_run(loop);
>  
> +
>      return 0;
>  }
>  
> diff --git a/libvirt-glib/libvirt-glib-event.c b/libvirt-glib/libvirt-glib-event.c
> index a785c93..8b1bddf 100644
> --- a/libvirt-glib/libvirt-glib-event.c
> +++ b/libvirt-glib/libvirt-glib-event.c
> @@ -195,6 +195,18 @@ cleanup:
>      g_mutex_unlock(eventlock);
>  }
>  
> +static gboolean
> +_handle_remove(gpointer data)
> +{
> +    struct gvir_event_handle *h = data;
> +
> +    if (h->ff)
> +        (h->ff)(h->opaque);
> +    free(h);
> +
> +    return FALSE;
> +}
> +
>  static int
>  gvir_event_handle_remove(int watch)
>  {
> @@ -217,10 +229,7 @@ gvir_event_handle_remove(int watch)
>      g_source_remove(data->source);
>      data->source = 0;
>      data->events = 0;
> -    if (data->ff)
> -        (data->ff)(data->opaque);
> -    free(data);
> -
> +    g_idle_add(_handle_remove, data);
>      ret = 0;
>  
>  cleanup:

ACK, the libvirt default event loop already does a very similar
thing, since we need to be re-entrant safe.

This same change would also apply to the gvir_event_timeout_remove
function I expect.


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