On Fri, Jan 31, 2020 at 03:48:06PM +0100, Pavel Hrdina wrote: > 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]. Yep. > > +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? Yes & I'll pull this into helper methods to avoid repeating it. > > + 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] Ok > > +{ > > + 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. Yeah it is redundant. I also need to include the systemtap probe points we have in the current glib impl Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
