[Spice-devel] [PATCH v2 2/3] tests: create and use a template file for events
Christophe Fergeau
cfergeau at redhat.com
Thu Dec 17 06:42:31 PST 2015
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 at redhat.com>
> ---
> 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);
}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151217/f27d94bf/attachment.sig>
More information about the Spice-devel
mailing list