[libvirt] [PATCH][libvirt-glib] Don't misuse GMainLoop for libvirt event loop
Christophe Fergeau
cfergeau at redhat.com
Thu May 24 09:38:39 UTC 2012
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120524/5c8a5c4f/attachment-0001.sig>
More information about the libvir-list
mailing list