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

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.

+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);

+ + 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list