Hey, On Thu, Dec 17, 2015 at 01:22:19PM +0000, Frediano Ziglio wrote: > This template will be reused for main loop > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > --- > server/Makefile.am | 1 + > server/event_loop.tmpl.c | 191 ++++++++++++++++++++++++++++++++++++++++ > server/tests/basic_event_loop.c | 159 ++------------------------------- > 3 files changed, 198 insertions(+), 153 deletions(-) > create mode 100644 server/event_loop.tmpl.c > > diff --git a/server/Makefile.am b/server/Makefile.am > index 32ab8eb..ce42fbb 100644 > --- a/server/Makefile.am > +++ b/server/Makefile.am > @@ -155,6 +155,7 @@ EXTRA_DIST = \ > cache-item.tmpl.c \ > glz-encode-match.tmpl.c \ > glz-encode.tmpl.c \ > + event_loop.tmpl.c \ > spice-server.syms \ > $(NULL) > > diff --git a/server/event_loop.tmpl.c b/server/event_loop.tmpl.c > new file mode 100644 > index 0000000..0790d36 > --- /dev/null > +++ b/server/event_loop.tmpl.c > @@ -0,0 +1,191 @@ > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ > +/* > + Copyright (C) 2009-2015 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/>. > +*/ > +#ifdef HAVE_CONFIG_H > +#include <config.h> > +#endif > + > +#include <glib.h> > +#include "spice/macros.h" > +#include "common/mem.h" > + > +#ifndef CORE_NAME > +#error CORE_NAME must be defined! > +#endif Given that most of these functions are static and are accessed through a function pointer, I don't think it's that important to have different prefixes for them. Just pick a correct namespace (or none), and just use static .. foo_timer_add(...); in every user of the template, I don't think this matters. Could be useful for CORE_NAME(core), but then this could just be a spice_mainloop_new() call or something like that. > +#ifndef CORE_CHANNEL_EVENT > +#define CORE_CHANNEL_EVENT NULL > +#endif This template variable is used in one place: > +static SpiceCoreInterface CORE_NAME(core) = { > + .base = { > + .major_version = SPICE_INTERFACE_CORE_MAJOR, > + .minor_version = SPICE_INTERFACE_CORE_MINOR, > + }, > + .timer_add = CORE_NAME(timer_add), > + .timer_start = CORE_NAME(timer_start), > + .timer_cancel = CORE_NAME(timer_cancel), > + .timer_remove = CORE_NAME(timer_remove), > + > + .watch_add = CORE_NAME(watch_add), > + .watch_update_mask = CORE_NAME(watch_update_mask), > + .watch_remove = CORE_NAME(watch_remove), > + > + .channel_event = CORE_CHANNEL_EVENT > +}; My understanding is that users of the template will only be interested in getting this initialized 'core' variable, so it's probably not too hard to add a core.channel_event = xxxx; in some init function if we want to remove the need for a template variable? > +#ifndef CORE_MAIN_CONTEXT > +#error CORE_MAIN_CONTEXT must be defined! > +#endif This is the 3rd template variable that is needed, it's used below: > +static void CORE_NAME(timer_start)(SpiceTimer *timer, uint32_t ms) > +{ > + CORE_NAME(timer_cancel)(timer); > + > + timer->source = g_timeout_source_new(ms); > + spice_assert(timer->source != NULL); > + > + g_source_set_callback(timer->source, CORE_NAME(timer_func), timer, NULL); > + > + g_source_attach(timer->source, CORE_MAIN_CONTEXT(timer->opaque)); > +} It's used to pick the correct main context so that things run in the correct thread when this is used from a worker thread. However, this looks like what g_main_context_get_thread_default()/g_main_context_push_thread_default() are meant to address (see proposed patch below). I don't really like making assumption about what the user-passed 'opaque' pointer is going to be, and as Marc-Andre, I'd prefer if we did not use a template. If the patch below works ok (I only compile-tested it), I think this should allow to get rid of the templating. diff --git a/server/event_loop.tmpl.c b/server/event_loop.tmpl.c index b85e6ee..f4aa659 100644 --- a/server/event_loop.tmpl.c +++ b/server/event_loop.tmpl.c @@ -27,10 +27,6 @@ #error CORE_NAME must be defined! #endif -#ifndef CORE_MAIN_CONTEXT -#error CORE_MAIN_CONTEXT must be defined! -#endif - #ifndef CORE_CHANNEL_EVENT #define CORE_CHANNEL_EVENT NULL #endif @@ -79,7 +75,7 @@ static void CORE_NAME(timer_start)(SpiceTimer *timer, uint32_t ms) g_source_set_callback(timer->source, timer_func, timer, NULL); - g_source_attach(timer->source, CORE_MAIN_CONTEXT(timer->opaque)); + g_source_attach(timer->source, g_main_context_get_thread_default()); } static void CORE_NAME(timer_remove)(SpiceTimer *timer) @@ -144,7 +140,7 @@ static void CORE_NAME(watch_update_mask)(SpiceWatch *watch, int event_mask) watch->source = g_io_create_watch(watch->channel, spice_event_to_giocondition(event_mask)); g_source_set_callback(watch->source, (GSourceFunc)watch_func, watch, NULL); - g_source_attach(watch->source, CORE_MAIN_CONTEXT(watch->opaque)); + g_source_attach(watch->source, g_main_context_get_thread_default()); } static SpiceWatch *CORE_NAME(watch_add)(int fd, int event_mask, SpiceWatchFunc func, void *opaque) diff --git a/server/red-worker.c b/server/red-worker.c index 26d4fe8..6886541 100644 --- a/server/red-worker.c +++ b/server/red-worker.c @@ -509,23 +509,7 @@ static int common_channel_config_socket(RedChannelClient *rcc) return TRUE; } -static inline GMainContext *worker_get_main_context_from_opaque(void *opaque) -{ - /* Since we are a channel core implementation, we always get called from - red_channel_client_create(), so opaque always is our rcc */ - RedChannelClient *rcc = opaque; - RedWorker *worker; - - spice_assert(rcc != NULL); - /* Since we are called from red_channel_client_create() - CommonChannelClient->worker has not been set yet! */ - worker = SPICE_CONTAINEROF(rcc->channel, CommonChannel, base)->worker; - - return worker->main_context; -} - #define CORE_NAME(name) worker_##name -#define CORE_MAIN_CONTEXT(opaque) worker_get_main_context_from_opaque(opaque) #include "event_loop.tmpl.c" @@ -1627,11 +1611,14 @@ SPICE_GNUC_NORETURN static void *red_worker_main(void *arg) RED_CHANNEL(worker->cursor_channel)->thread_id = pthread_self(); RED_CHANNEL(worker->display_channel)->thread_id = pthread_self(); + g_main_context_push_thread_default(worker->main_context); GMainLoop *loop = g_main_loop_new(worker->main_context, FALSE); g_main_loop_run(loop); g_main_loop_unref(loop); + g_main_context_pop_thread_default(worker->main_context); + /* FIXME: free worker, and join threads */ exit(0); }
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel