On Tue, Jan 28, 2020 at 01:11:20PM +0000, Daniel P. Berrangé wrote: > The libvirt-glib project has provided a GMainContext based > event loop impl for applications. This imports it and sets > it up for use by libvirt as the primary event loop. This > remains a private impl detail of libvirt. > > IOW, applications must *NOT* assume that a call to > "virEventRegisterDefaultImpl" results in a GLib based > event loop. They should continue to use the libvirt-glib > API gvir_event_register() if they explicitly want to > guarantee a GLib event loop. > > This follows the general principal that the libvirt public > API should not expose the fact that GLib is being used > internally. > > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx> > --- > build-aux/syntax-check.mk | 2 +- > po/POTFILES.in | 1 + > src/libvirt_private.syms | 5 + > src/util/Makefile.inc.am | 2 + > src/util/vireventglib.c | 455 ++++++++++++++++++++++++++++++++++++++ > src/util/vireventglib.h | 28 +++ > 6 files changed, 492 insertions(+), 1 deletion(-) > create mode 100644 src/util/vireventglib.c > create mode 100644 src/util/vireventglib.h [...] > diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c > new file mode 100644 > index 0000000000..be057c8e3c > --- /dev/null > +++ b/src/util/vireventglib.c > @@ -0,0 +1,455 @@ > +/* > + * vireventglib.c: GMainContext based event loop > + * > + * Copyright (C) 2008 Daniel P. Berrange > + * Copyright (C) 2010-2019 Red Hat, Inc. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>. > + */ > + > +#include <config.h> > + > +#include <stdio.h> > +#include <string.h> > +#include <stdlib.h> > + > +#include "vireventglib.h" > +#include "vireventglibwatch.h" > +#include "virerror.h" > +#include "virlog.h" > + > +#ifdef G_OS_WIN32 > +# include <io.h> > +#endif > + > +#define VIR_FROM_THIS VIR_FROM_EVENT > + > +VIR_LOG_INIT("util.eventglib"); > + > +struct virEventGLibHandle > +{ > + int watch; > + int fd; > + int events; > + int removed; > + guint source; > + virEventHandleCallback cb; > + void *opaque; > + virFreeCallback ff; > +}; > + > +struct virEventGLibTimeout > +{ > + int timer; > + int interval; > + int removed; > + guint source; > + virEventTimeoutCallback cb; > + void *opaque; > + virFreeCallback ff; > +}; > + > +static GMutex *eventlock; > + > +static int nextwatch = 1; > +static GPtrArray *handles; > + > +static int nexttimer = 1; > +static GPtrArray *timeouts; > + > +static gboolean > +virEventGLibHandleDispatch(int fd G_GNUC_UNUSED, > + GIOCondition condition, > + gpointer opaque) > +{ > + struct virEventGLibHandle *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; > + > + VIR_DEBUG("Dispatch handler %p %d %d %d %p", data, data->watch, data->fd, events, data->opaque); Missing names for the formatted data: "Dispatch handler data=%p watch=%d fd=%d evetns=%d opaque=%p". The same thing is done for timer and it improves the debug log. There are some more places that should be fixed [1]. > + > + (data->cb)(data->watch, data->fd, events, data->opaque); > + > + return TRUE; > +} > + > + > +static int > +virEventGLibHandleAdd(int fd, > + int events, > + virEventHandleCallback cb, > + void *opaque, > + virFreeCallback ff) > +{ > + struct virEventGLibHandle *data; > + GIOCondition cond = 0; > + int ret; > + > + g_mutex_lock(eventlock); > + > + data = g_new0(struct virEventGLibHandle, 1); > + > + if (events & VIR_EVENT_HANDLE_READABLE) > + cond |= G_IO_IN; > + if (events & VIR_EVENT_HANDLE_WRITABLE) > + cond |= G_IO_OUT; Aren't we missing VIR_EVENT_HANDLE_ERROR and VIR_EVENT_HANDLE_HANGUP? > + > + data->watch = nextwatch++; > + data->fd = fd; > + data->events = events; > + data->cb = cb; > + data->opaque = opaque; > + data->ff = ff; > + > + VIR_DEBUG("Add handle %p %d %d %d %p", data, data->watch, data->fd, events, data->opaque); [1] > + if (events != 0) { > + data->source = virEventGLibAddSocketWatch( > + fd, cond, NULL, virEventGLibHandleDispatch, data, NULL); > + } > + > + g_ptr_array_add(handles, data); > + > + ret = data->watch; > + > + g_mutex_unlock(eventlock); > + > + return ret; > +} > + > +static struct virEventGLibHandle * > +virEventGLibHandleFind(int watch) > +{ > + guint i; > + > + for (i = 0; i < handles->len; i++) { > + struct virEventGLibHandle *h = g_ptr_array_index(handles, i); > + > + if (h == NULL) { > + g_warn_if_reached(); > + continue; > + } > + > + if ((h->watch == watch) && !h->removed) > + return h; > + } > + > + return NULL; > +} > + > +static void > +virEventGLibHandleUpdate(int watch, > + int events) > +{ > + struct virEventGLibHandle *data; > + > + g_mutex_lock(eventlock); > + > + data = virEventGLibHandleFind(watch); > + if (!data) { > + VIR_DEBUG("Update for missing handle watch %d", watch); > + goto cleanup; > + } > + > + VIR_DEBUG("Update handle %p %d %d %d", data, watch, data->fd, events); [1] > + if (events) { > + GIOCondition cond = 0; > + if (events == data->events) > + goto cleanup; > + > + if (data->source) { I would change the condition to "data->source != 0" to be consistent with other checks in this file and we also prefer this form in the case where the value is not boolean or pointer. There are more places that should be fixed as well. [2] > + VIR_DEBUG("Removed old watch %d", 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; Same as for the virEventGLibHandleAdd, what about VIR_EVENT_HANDLE_ERROR and VIR_EVENT_HANDLE_HANGUP? > + data->source = virEventGLibAddSocketWatch( > + data->fd, cond, NULL, virEventGLibHandleDispatch, data, NULL); > + > + data->events = events; > + VIR_DEBUG("Added new watch %d", data->source); > + } else { > + if (!data->source) [2] > + goto cleanup; > + > + VIR_DEBUG("Removed old watch"); > + g_source_remove(data->source); > + data->source = 0; > + data->events = 0; > + } > + > + cleanup: > + g_mutex_unlock(eventlock); > +} > + > +static gboolean > +virEventGLibHandleRemoveIdle(gpointer data) > +{ > + struct virEventGLibHandle *h = data; > + > + if (h->ff) > + (h->ff)(h->opaque); > + > + g_mutex_lock(eventlock); > + g_ptr_array_remove_fast(handles, h); > + g_mutex_unlock(eventlock); > + > + return FALSE; > +} > + > +static int > +virEventGLibHandleRemove(int watch) > +{ > + struct virEventGLibHandle *data; > + int ret = -1; > + > + g_mutex_lock(eventlock); > + > + data = virEventGLibHandleFind(watch); > + if (!data) { > + VIR_DEBUG("Remove of missing watch %d", watch); > + goto cleanup; > + } > + > + VIR_DEBUG("Remove handle %p %d %d", data, watch, data->fd); [1] > + if (data->source != 0) { > + g_source_remove(data->source); > + data->source = 0; > + data->events = 0; > + } > + > + /* since the actual watch deletion is done asynchronously, a handleUpdate call may > + * reschedule the watch before it's fully deleted, that's why we need to mark it as > + * 'removed' to prevent reuse > + */ > + data->removed = TRUE; > + g_idle_add(virEventGLibHandleRemoveIdle, data); > + > + ret = 0; > + > + cleanup: > + g_mutex_unlock(eventlock); > + return ret; > +} > + > + > +static gboolean > +virEventGLibTimeoutDispatch(void *opaque) > +{ > + struct virEventGLibTimeout *data = opaque; Nit: I would put empty line here and > + VIR_DEBUG("Dispatch timeout data=%p cb=%p timer=%d opaque=%p", data, data->cb, data->timer, data->opaque); here as well. > + (data->cb)(data->timer, data->opaque); > + > + return TRUE; > +} > + > +static int > +virEventGLibTimeoutAdd(int interval, > + virEventTimeoutCallback cb, > + void *opaque, > + virFreeCallback ff) > +{ > + struct virEventGLibTimeout *data; > + int ret; > + > + g_mutex_lock(eventlock); > + > + data = g_new0(struct virEventGLibTimeout, 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, > + virEventGLibTimeoutDispatch, > + data); > + > + g_ptr_array_add(timeouts, data); > + > + VIR_DEBUG("Add timeout data=%p interval=%d ms cb=%p opaque=%p timer=%d", data, interval, cb, opaque, data->timer); ^ Extra space here. > + > + ret = data->timer; > + > + g_mutex_unlock(eventlock); > + > + return ret; > +} > + > + > +static struct virEventGLibTimeout * > +virEventGLibTimeoutFind(int timer) > +{ > + guint i; > + > + g_return_val_if_fail(timeouts != NULL, NULL); > + > + for (i = 0; i < timeouts->len; i++) { > + struct virEventGLibTimeout *t = g_ptr_array_index(timeouts, i); > + > + if (t == NULL) { > + g_warn_if_reached(); > + continue; > + } > + > + if ((t->timer == timer) && !t->removed) > + return t; > + } > + > + return NULL; > +} > + > + > +static void > +virEventGLibTimeoutUpdate(int timer, > + int interval) > +{ > + struct virEventGLibTimeout *data; > + > + g_mutex_lock(eventlock); > + > + data = virEventGLibTimeoutFind(timer); > + if (!data) { > + VIR_DEBUG("Update of missing timer %d", timer); > + goto cleanup; > + } > + > + VIR_DEBUG("Update timeout %p %d %d", data, timer, interval); [1] > + > + if (interval >= 0) { > + if (data->source) [2] > + g_source_remove(data->source); > + > + data->interval = interval; > + data->source = g_timeout_add(data->interval, > + virEventGLibTimeoutDispatch, > + data); > + } else { > + if (!data->source) [2] > + goto cleanup; > + > + g_source_remove(data->source); > + data->source = 0; > + } > + > + cleanup: > + g_mutex_unlock(eventlock); > +} > + > +static gboolean > +virEventGLibTimeoutRemoveIdle(gpointer data) > +{ > + struct virEventGLibTimeout *t = data; > + > + if (t->ff) > + (t->ff)(t->opaque); > + > + g_mutex_lock(eventlock); > + g_ptr_array_remove_fast(timeouts, t); > + g_mutex_unlock(eventlock); > + > + return FALSE; > +} > + > +static int > +virEventGLibTimeoutRemove(int timer) > +{ > + struct virEventGLibTimeout *data; > + int ret = -1; > + > + g_mutex_lock(eventlock); > + > + data = virEventGLibTimeoutFind(timer); > + if (!data) { > + VIR_DEBUG("Remove of missing timer %d", timer); > + goto cleanup; > + } > + > + VIR_DEBUG("Remove timeout %p %d", data, timer); > + > + if (data->source != 0) { > + g_source_remove(data->source); > + data->source = 0; > + } > + > + /* since the actual timeout deletion is done asynchronously, a timeoutUpdate call may > + * reschedule the timeout before it's fully deleted, that's why we need to mark it as > + * 'removed' to prevent reuse > + */ > + data->removed = TRUE; > + g_idle_add(virEventGLibTimeoutRemoveIdle, data); > + > + ret = 0; > + > + cleanup: > + g_mutex_unlock(eventlock); > + return ret; > +} > + > + > +static gpointer virEventGLibRegisterOnce(gpointer data G_GNUC_UNUSED) > +{ > + eventlock = g_new0(GMutex, 1); > + timeouts = g_ptr_array_new_with_free_func(g_free); > + handles = g_ptr_array_new_with_free_func(g_free); > + virEventRegisterImpl(virEventGLibHandleAdd, > + virEventGLibHandleUpdate, > + virEventGLibHandleRemove, > + virEventGLibTimeoutAdd, > + virEventGLibTimeoutUpdate, > + virEventGLibTimeoutRemove); > + return NULL; > +} > + > + > +void virEventGLibRegister(void) > +{ > + static GOnce once = G_ONCE_INIT; > + > + g_once(&once, virEventGLibRegisterOnce, NULL); > +} > + > + > +int virEventGLibRunOnce(void) > +{ > + GMainContext *ctx = g_main_context_default(); > + > + if (!g_main_context_acquire(ctx)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Another thread has acquired the main loop context")); > + return -1; > + } > + > + g_main_context_iteration(ctx, TRUE); > + > + g_main_context_release(ctx); No need to acquire and release the GMainContext here as it's done by g_main_context_iteration() as well. In case you want to have the explicit error some comment would be nice. Otherwise looks good, the only issue I have is the missing events for handle. Pavel
Attachment:
signature.asc
Description: PGP signature
