[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH][libvirt-glib] Don't misuse GMainLoop for libvirt event loop



On Thu, May 24, 2012 at 11:01:19AM +0200, Michal Privoznik wrote:
> Since we are calling APIs within timers, those might block as
> any libvirt API may block. This, however, might get so critical,
> that server will shut us down due to timeout on keepalive.
> Simply, if an API called within a timer block, entire event loop
> is blocked and no keepalive response can be sent.

I don't think glib timeouts are anything special here. gtk+ applications
are callback driven, and if anything block in any callback, this will block
the mainloop. In particular, some libvirt calls will block. If in the mean
time we get a keepalive request, libvirt cannot answer it while the
mainloop is blocked, and the application gets disconnected. Am I getting
things right? Do you have any idea about the time libvirtd will wait for a
keepalive answer before kicking us out? While it's good to improve the
current situation, it's bad from applications to call blocking libvirt
calls from the mainloop.

I haven't looked at this carefully yet, but...

> +static gpointer
> +event_loop(gpointer dummy G_GNUC_UNUSED)
> +{
> +    while (1) {
> +        gboolean do_quit;
> +    
> +        g_mutex_lock(event_lock);
> +        do_quit = quit;
> +        g_mutex_unlock(event_lock);

... glib has atomic operations (
http://developer.gnome.org/glib/stable/glib-Atomic-Operations.html ), I
think this would be better than this mutex. Or maybe deinit_timer could
just call g_thread_exit(NULL); ? (I guess this last solution is too brutal
;)

Christophe

> +
> +        if (do_quit)
> +            break;
> +
> +        if (virEventRunDefaultImpl() < 0)
> +            g_warn_if_reached();
> +    }
> +    return NULL;
> +}
> +
> +static void
> +deinit_timer(int timer G_GNUC_UNUSED, void *dummy G_GNUC_UNUSED)
>  {
> -    int watch;
> -    int fd;
> -    int events;
> -    int enabled;
> -    GIOChannel *channel;
> -    guint source;
> -    virEventHandleCallback cb;
> -    void *opaque;
> -    virFreeCallback ff;
> -};
> -
> -struct gvir_event_timeout
> +    /* nothing to be done here */
> +}
> +
> +static gpointer
> +event_deregister_once(gpointer data G_GNUC_UNUSED)
>  {
>      int timer;
> -    int interval;
> -    guint source;
> -    virEventTimeoutCallback cb;
> -    void *opaque;
> -    virFreeCallback ff;
> -};
>  
> -GMutex *eventlock = NULL;
> -
> -static int nextwatch = 1;
> -static GPtrArray *handles;
> -
> -static int nexttimer = 1;
> -static GPtrArray *timeouts;
> -
> -static gboolean
> -gvir_event_handle_dispatch(GIOChannel *source G_GNUC_UNUSED,
> -                           GIOCondition condition,
> -                           gpointer opaque)
> -{
> -    struct gvir_event_handle *data = opaque;
> -    int events = 0;
> -
> -    if (condition & G_IO_IN)
> -        events |= VIR_EVENT_HANDLE_READABLE;
> -    if (condition & G_IO_OUT)
> -        events |= VIR_EVENT_HANDLE_WRITABLE;
> -    if (condition & G_IO_HUP)
> -        events |= VIR_EVENT_HANDLE_HANGUP;
> -    if (condition & G_IO_ERR)
> -        events |= VIR_EVENT_HANDLE_ERROR;
> -
> -    g_debug("Dispatch handler %d %d %p\n", data->fd, events, data->opaque);
> -
> -    (data->cb)(data->watch, data->fd, events, data->opaque);
> -
> -    return TRUE;
> -}
> -
> -
> -static int
> -gvir_event_handle_add(int fd,
> -                      int events,
> -                      virEventHandleCallback cb,
> -                      void *opaque,
> -                      virFreeCallback ff)
> -{
> -    struct gvir_event_handle *data;
> -    GIOCondition cond = 0;
> -    int ret;
> -
> -    g_mutex_lock(eventlock);
> -
> -    data = g_new0(struct gvir_event_handle, 1);
> -
> -    if (events & VIR_EVENT_HANDLE_READABLE)
> -        cond |= G_IO_IN;
> -    if (events & VIR_EVENT_HANDLE_WRITABLE)
> -        cond |= G_IO_OUT;
> -
> -    data->watch = nextwatch++;
> -    data->fd = fd;
> -    data->events = events;
> -    data->cb = cb;
> -    data->opaque = opaque;
> -    data->channel = g_io_channel_unix_new(fd);
> -    data->ff = ff;
> -
> -    g_debug("Add handle %d %d %p\n", data->fd, events, data->opaque);
> -
> -    data->source = g_io_add_watch(data->channel,
> -                                  cond,
> -                                  gvir_event_handle_dispatch,
> -                                  data);
> -
> -    g_ptr_array_add(handles, data);
> -
> -    ret = data->watch;
> -
> -    g_mutex_unlock(eventlock);
> -
> -    return ret;
> -}
> -
> -static struct gvir_event_handle *
> -gvir_event_handle_find(int watch, guint *idx)
> -{
> -    guint i;
> -
> -    for (i = 0 ; i < handles->len ; i++) {
> -        struct gvir_event_handle *h = g_ptr_array_index(handles, i);
> -
> -        if (h == NULL) {
> -            g_warn_if_reached ();
> -            continue;
> -        }
> -
> -        if (h->watch == watch) {
> -            if (idx != NULL)
> -                *idx = i;
> -            return h;
> -        }
> -    }
> -
> -    return NULL;
> -}
> -
> -static void
> -gvir_event_handle_update(int watch,
> -                         int events)
> -{
> -    struct gvir_event_handle *data;
> -
> -    g_mutex_lock(eventlock);
> -
> -    data = gvir_event_handle_find(watch, NULL);
> -    if (!data) {
> -        g_debug("Update for missing handle watch %d", watch);
> -        goto cleanup;
> +    if (!event_thread) {
> +        g_warn_if_reached();
> +        return NULL;
>      }
>  
> -    if (events) {
> -        GIOCondition cond = 0;
> -        if (events == data->events)
> -            goto cleanup;
> -
> -        if (data->source)
> -            g_source_remove(data->source);
> -
> -        cond |= G_IO_HUP;
> -        if (events & VIR_EVENT_HANDLE_READABLE)
> -            cond |= G_IO_IN;
> -        if (events & VIR_EVENT_HANDLE_WRITABLE)
> -            cond |= G_IO_OUT;
> -        data->source = g_io_add_watch(data->channel,
> -                                      cond,
> -                                      gvir_event_handle_dispatch,
> -                                      data);
> -        data->events = events;
> -    } else {
> -        if (!data->source)
> -            goto cleanup;
> -
> -        g_source_remove(data->source);
> -        data->source = 0;
> -        data->events = 0;
> -    }
> -
> -cleanup:
> -    g_mutex_unlock(eventlock);
> -}
> +    g_mutex_lock(event_lock);
> +    quit = TRUE;
> +    /* Add dummy timeout to break event loop */
> +    timer = virEventAddTimeout(0, deinit_timer, NULL,  NULL);
> +    g_mutex_unlock(event_lock);
>  
> -static gboolean
> -_event_handle_remove(gpointer data)
> -{
> -    struct gvir_event_handle *h = data;
> -
> -    if (h->ff)
> -        (h->ff)(h->opaque);
> -
> -    g_ptr_array_remove_fast(handles, h);
> -
> -    return FALSE;
> -}
> -
> -static int
> -gvir_event_handle_remove(int watch)
> -{
> -    struct gvir_event_handle *data;
> -    int ret = -1;
> -    guint idx;
> -
> -    g_mutex_lock(eventlock);
> -
> -    data = gvir_event_handle_find(watch, &idx);
> -    if (!data) {
> -        g_debug("Remove of missing watch %d", watch);
> -        goto cleanup;
> -    }
> -
> -    g_debug("Remove handle %d %d\n", watch, data->fd);
> -
> -    if (!data->source)
> -        goto cleanup;
> +    g_thread_join(event_thread);
> +    g_mutex_free(event_lock);
> +    event_lock = NULL;
> +    event_thread = NULL;
> +    quit = FALSE;
>  
> -    g_source_remove(data->source);
> -    data->source = 0;
> -    data->events = 0;
> -    g_idle_add(_event_handle_remove, data);
> -
> -    ret = 0;
> -
> -cleanup:
> -    g_mutex_unlock(eventlock);
> -    return ret;
> -}
> -
> -
> -static gboolean
> -gvir_event_timeout_dispatch(void *opaque)
> -{
> -    struct gvir_event_timeout *data = opaque;
> -    g_debug("Dispatch timeout %p %p %d %p\n", data, data->cb, data->timer, data->opaque);
> -    (data->cb)(data->timer, data->opaque);
> -
> -    return TRUE;
> -}
> -
> -static int
> -gvir_event_timeout_add(int interval,
> -                       virEventTimeoutCallback cb,
> -                       void *opaque,
> -                       virFreeCallback ff)
> -{
> -    struct gvir_event_timeout *data;
> -    int ret;
> -
> -    g_mutex_lock(eventlock);
> -
> -    data = g_new0(struct gvir_event_timeout, 1);
> -    data->timer = nexttimer++;
> -    data->interval = interval;
> -    data->cb = cb;
> -    data->opaque = opaque;
> -    data->ff = ff;
> -    if (interval >= 0)
> -        data->source = g_timeout_add(interval,
> -                                     gvir_event_timeout_dispatch,
> -                                     data);
> -
> -    g_ptr_array_add(timeouts, data);
> -
> -    g_debug("Add timeout %p %d %p %p %d\n", data, interval, cb, opaque, data->timer);
> -
> -    ret = data->timer;
> -
> -    g_mutex_unlock(eventlock);
> -
> -    return ret;
> -}
> -
> -
> -static struct gvir_event_timeout *
> -gvir_event_timeout_find(int timer, guint *idx)
> -{
> -    guint i;
> -
> -    g_return_val_if_fail(timeouts != NULL, NULL);
> -
> -    for (i = 0 ; i < timeouts->len ; i++) {
> -        struct gvir_event_timeout *t = g_ptr_array_index(timeouts, i);
> -
> -        if (t == NULL) {
> -            g_warn_if_reached ();
> -            continue;
> -        }
> -
> -        if (t->timer == timer) {
> -            if (idx != NULL)
> -                *idx = i;
> -            return t;
> -        }
> -    }
> +    if (timer != -1)
> +        virEventRemoveTimeout(timer);
>  
>      return NULL;
>  }
>  
> -
> -static void
> -gvir_event_timeout_update(int timer,
> -                          int interval)
> +/*
> + * gvir_event_deregister:
> + *
> + * De-register libvirt event loop and free allocated memory.
> + * If invoked more than once, this method will be no-op.
> + *
> + */
> +void
> +gvir_event_deregister(void)
>  {
> -    struct gvir_event_timeout *data;
> -
> -    g_mutex_lock(eventlock);
> +    static GOnce once = G_ONCE_INIT;
>  
> -    data = gvir_event_timeout_find(timer, NULL);
> -    if (!data) {
> -        g_debug("Update of missing timer %d", timer);
> -        goto cleanup;
> -    }
> -
> -    g_debug("Update timeout %p %d %d\n", data, timer, interval);
> -
> -    if (interval >= 0) {
> -        if (data->source)
> -            goto cleanup;
> -
> -        data->interval = interval;
> -        data->source = g_timeout_add(data->interval,
> -                                     gvir_event_timeout_dispatch,
> -                                     data);
> -    } else {
> -        if (!data->source)
> -            goto cleanup;
> -
> -        g_source_remove(data->source);
> -        data->source = 0;
> -    }
> -
> -cleanup:
> -    g_mutex_unlock(eventlock);
> -}
> -
> -static gboolean
> -_event_timeout_remove(gpointer data)
> -{
> -    struct gvir_event_timeout *t = data;
> -
> -    if (t->ff)
> -        (t->ff)(t->opaque);
> -
> -    g_ptr_array_remove_fast(timeouts, t);
> -
> -    return FALSE;
> +    g_once(&once, event_deregister_once, NULL);
>  }
>  
> -static int
> -gvir_event_timeout_remove(int timer)
> +static gpointer
> +event_register_once(gpointer data G_GNUC_UNUSED)
>  {
> -    struct gvir_event_timeout *data;
> -    guint idx;
> -    int ret = -1;
> -
> -    g_mutex_lock(eventlock);
> +    GError *err;
>  
> -    data = gvir_event_timeout_find(timer, &idx);
> -    if (!data) {
> -        g_debug("Remove of missing timer %d", timer);
> +    if (virEventRegisterDefaultImpl() < 0)
>          goto cleanup;
> -    }
> -
> -    g_debug("Remove timeout %p %d\n", data, timer);
> -
> -    if (!data->source)
> -        goto cleanup;
> -
> -    g_source_remove(data->source);
> -    data->source = 0;
> -    g_idle_add(_event_timeout_remove, data);
>  
> -    ret = 0;
> +    event_lock = g_mutex_new();
> +    
> +    event_thread = g_thread_create(event_loop, NULL, TRUE, &err);
> +    if (!event_thread) {
> +        g_warning("Libvirt event loop thread could not be created: %s",
> +                  err->message);
> +        g_error_free(err);
> +        g_mutex_free(event_lock);
> +    }
>  
>  cleanup:
> -    g_mutex_unlock(eventlock);
> -    return ret;
> +    return event_thread;
>  }
>  
> -
> -static gpointer event_register_once(gpointer data G_GNUC_UNUSED)
> -{
> -    eventlock = g_mutex_new();
> -    timeouts = g_ptr_array_new_with_free_func(g_free);
> -    handles = g_ptr_array_new_with_free_func(g_free);
> -    virEventRegisterImpl(gvir_event_handle_add,
> -                         gvir_event_handle_update,
> -                         gvir_event_handle_remove,
> -                         gvir_event_timeout_add,
> -                         gvir_event_timeout_update,
> -                         gvir_event_timeout_remove);
> -    return NULL;
> -}
> -
> -
>  /**
>   * gvir_event_register:
>   *
> - * Registers a libvirt event loop implementation that is backed
> - * by the default <code>GMain</code> context. If invoked more
> - * than once this method will be a no-op. Applications should,
> - * however, take care not to register any another non-GLib
> - * event loop with libvirt.
> + * Registers a libvirt event loop implementation. If invoked
> + * more than once this method will be a no-op. If event loop
> + * is no longer needed, it should be free'd by invoking
> + * <code>gvir_event_deregister()</code>.
> + * Failure to run the event loop will mean no libvirt events
> + * get dispatched, and the libvirt keepalive timer will kill
> + * off libvirt connections frequently.
>   *
> - * After invoking this method, it is mandatory to run the
> - * default GMain event loop. Typically this can be satisfied
> - * by invoking <code>gtk_main</code> or <code>g_application_run</code>
> - * in the application's main thread. Failure to run the event
> - * loop will mean no libvirt events get dispatched, and the
> - * libvirt keepalive timer will kill off libvirt connections
> - * frequently.
> + * Returns: (transfer full): #TRUE on successful init,
> + * #FALSE otherwise.
>   */
> -void gvir_event_register(void)
> +gboolean
> +gvir_event_register(void)
>  {
>      static GOnce once = G_ONCE_INIT;
>  
>      g_once(&once, event_register_once, NULL);
> +
> +    return once.retval ? TRUE : FALSE;
>  }
> diff --git a/libvirt-glib/libvirt-glib-event.h b/libvirt-glib/libvirt-glib-event.h
> index 57cadab..e163674 100644
> --- a/libvirt-glib/libvirt-glib-event.h
> +++ b/libvirt-glib/libvirt-glib-event.h
> @@ -28,7 +28,8 @@
>  
>  G_BEGIN_DECLS
>  
> -void gvir_event_register(void);
> +gboolean gvir_event_register(void);
> +void gvir_event_deregister(void);
>  
>  G_END_DECLS
>  
> diff --git a/libvirt-glib/libvirt-glib.sym b/libvirt-glib/libvirt-glib.sym
> index 53b8907..8f6f15f 100644
> --- a/libvirt-glib/libvirt-glib.sym
> +++ b/libvirt-glib/libvirt-glib.sym
> @@ -15,4 +15,9 @@ LIBVIRT_GLIB_0.0.7 {
>          *;
>  };
>  
> +LIBVIRT_GLIB_0.0.9 {
> +    global:
> +        gvir_event_deregister;
> +} LIBVIRT_GLIB_0.0.7;
> +
>  # .... define new API here using predicted next version number ....
> diff --git a/libvirt-gobject/libvirt-gobject-main.c b/libvirt-gobject/libvirt-gobject-main.c
> index 9dd6e3c..c7980ff 100644
> --- a/libvirt-gobject/libvirt-gobject-main.c
> +++ b/libvirt-gobject/libvirt-gobject-main.c
> @@ -66,7 +66,8 @@ gboolean gvir_init_object_check(int *argc,
>  {
>      g_type_init();
>  
> -    gvir_event_register();
> +    if (gvir_event_register() != TRUE)
> +        return FALSE;
>  
>      if (!gvir_init_check(argc, argv, err))
>          return FALSE;
> diff --git a/python/libvirt-glib.c b/python/libvirt-glib.c
> index 1daca36..6b8c78c 100644
> --- a/python/libvirt-glib.c
> +++ b/python/libvirt-glib.c
> @@ -24,17 +24,36 @@ extern void initcygvirtglibmod(void);
>  #endif
>  
>  #define VIR_PY_NONE (Py_INCREF (Py_None), Py_None)
> +#define VIR_PY_INT_FAIL (libvirt_intWrap(-1))
> +#define VIR_PY_INT_SUCCESS (libvirt_intWrap(0))
> +
> +PyObject *
> +libvirt_intWrap(int val)
> +{
> +    PyObject *ret;
> +    ret = PyInt_FromLong((long) val);
> +    return ret;
> +}
>  
>  static PyObject *
>  libvirt_gvir_event_register(PyObject *self G_GNUC_UNUSED, PyObject *args G_GNUC_UNUSED) {
> -    gvir_event_register();
> +    if ( gvir_event_register() != TRUE)
> +        return VIR_PY_INT_FAIL;
> +
> +    return VIR_PY_INT_SUCCESS;
> +}
> +
> +
> +static PyObject *
> +libvirt_gvir_event_deregister(PyObject *self G_GNUC_UNUSED, PyObject 8args G_GNUC_UNUSED) {
> +    gvir_event_deregister();
>  
>      return VIR_PY_NONE;
>  }
>  
> -
>  static PyMethodDef libvirtGLibMethods[] = {
>      {(char *) "event_register", libvirt_gvir_event_register, METH_VARARGS, NULL},
> +    {(char *) "event_deregister", libvirt_gvir_event_deregister, METH_VARARGS, NULL},
>      {NULL, NULL, 0, NULL}
>  };
>  
> -- 
> 1.7.8.5
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: pgpaFQTaellpm.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]