[Spice-devel] [PATCH v2 2/3] tests: create and use a template file for events

[Frediano Ziglio]

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

Honestly I would like to have this constant some time.

> 
> > +#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).
> 

No, this currently does not work.
The function is called also outside the worker thread (I start wondering that
building displar and cursor channel inside the worker thread was right..
this was changed by one of first refactory patches).
Not much different that using a __thread variable for main context (just MUCH
slower).

> 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

I don't like too. At the moment the patch is not addressing this
issue (and well... it's perhaps too much for a single patch).
I think a much better solution would be to have an additional argument
to watch_add and timer_add but they are part of ABI :(

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

I think mostly does not agree with the template... I have a version without
the template.

> 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);
>  }
> 
> 

Frediano