Re: [libvirt PATCH v2 39/56] util: import an event loop impl based on GMainContext

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(a)redhat.com&gt; > --- > 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].

> +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?

> + 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]

> +{ > + 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.