On Tue, Dec 15, 2015 at 12:15:11PM +0000, Frediano Ziglio wrote:
> Make sure we don't handle event reserved to other loop contexts.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/tests/basic_event_loop.c | 118 ++++++++++++++++++++++++----------------
> server/tests/basic_event_loop.h | 2 +
> server/tests/replay.c | 18 ++++--
> 3 files changed, 86 insertions(+), 52 deletions(-)
>
> diff --git a/server/tests/basic_event_loop.c b/server/tests/basic_event_loop.c
> index c9c2637..b1b19ac 100644
> --- a/server/tests/basic_event_loop.c
> +++ b/server/tests/basic_event_loop.c
> @@ -21,7 +21,6 @@
> #include <sys/time.h>
> #include <signal.h>
> #include <string.h>
> -#include <glib.h>
>
> #include "spice/macros.h"
> #include "common/ring.h"
> @@ -39,11 +38,23 @@ int debug = 0;
>
> #define NOT_IMPLEMENTED printf("%s not implemented\n", __func__);
>
> +static GMainContext *main_context = NULL;
> +
> +GMainContext *basic_event_loop_get_context(void)
> +{
> + return main_context;
> +}
> +
> +static void channel_event(int event, SpiceChannelEventInfo *info)
> +{
> + DPRINTF(0, "channel event con, type, id, event: %d, %d, %d, %d",
> + info->connection_id, info->type, info->id, event);
> +}
>
> struct SpiceTimer {
> SpiceTimerFunc func;
> void *opaque;
> - guint source_id;
> + GSource *source;
> };
>
> static SpiceTimer* timer_add(SpiceTimerFunc func, void *opaque)
> @@ -60,7 +71,6 @@ static gboolean timer_func(gpointer user_data)
> {
> SpiceTimer *timer = user_data;
>
> - timer->source_id = 0;
> timer->func(timer->opaque);
> /* timer might be free after func(), don't touch */
>
> @@ -69,34 +79,40 @@ static gboolean timer_func(gpointer user_data)
>
> static void timer_cancel(SpiceTimer *timer)
> {
> - if (timer->source_id == 0)
> - return;
> -
> - g_source_remove(timer->source_id);
> - timer->source_id = 0;
> + if (timer->source) {
> + g_source_destroy(timer->source);
> + g_source_unref(timer->source);
> + timer->source = NULL;
> + }
> }
>
> static void timer_start(SpiceTimer *timer, uint32_t ms)
> {
> timer_cancel(timer);
>
> - timer->source_id = g_timeout_add(ms, timer_func, timer);
> + timer->source = g_timeout_source_new(ms);
> + spice_assert(timer->source != NULL);
> +
> + g_source_set_callback(timer->source, timer_func, timer, NULL);
> +
> + g_source_attach(timer->source, main_context);
> }
>
> static void timer_remove(SpiceTimer *timer)
> {
> timer_cancel(timer);
> + spice_assert(timer->source == NULL);
> free(timer);
> }
>
> struct SpiceWatch {
> void *opaque;
> - guint source_id;
> + GSource *source;
> GIOChannel *channel;
> SpiceWatchFunc func;
> };
>
> -static GIOCondition spice_event_to_condition(int event_mask)
> +static GIOCondition spice_event_to_giocondition(int event_mask)
> {
> GIOCondition condition = 0;
>
> @@ -108,7 +124,7 @@ static GIOCondition spice_event_to_condition(int event_mask)
> return condition;
> }
>
> -static int condition_to_spice_event(GIOCondition condition)
> +static int giocondition_to_spice_event(GIOCondition condition)
> {
> int event = 0;
>
> @@ -126,50 +142,73 @@ static gboolean watch_func(GIOChannel *source, GIOCondition condition,
> SpiceWatch *watch = data;
> int fd = g_io_channel_unix_get_fd(source);
>
> - watch->func(fd, condition_to_spice_event(condition), watch->opaque);
> + watch->func(fd, giocondition_to_spice_event(condition), watch->opaque);
>
> return TRUE;
> }
>
> +static void watch_update_mask(SpiceWatch *watch, int event_mask)
> +{
> + if (watch->source) {
> + g_source_destroy(watch->source);
> + g_source_unref(watch->source);
> + watch->source = NULL;
> + }
> +
> + if (!event_mask)
> + return;
I would expect that this two conditions above would be in different
order, if !event_mask you should not release watch->source, right?
> +
> + 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, main_context);
> +}
> +
> static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
> {
> SpiceWatch *watch;
> - GIOCondition condition = spice_event_to_condition(event_mask);
> +
> + spice_return_val_if_fail(fd != -1, NULL);
> + spice_return_val_if_fail(func != NULL, NULL);
>
> watch = spice_malloc0(sizeof(SpiceWatch));
> watch->channel = g_io_channel_unix_new(fd);
> - watch->source_id = g_io_add_watch(watch->channel, condition, watch_func, watch);
> watch->func = func;
> watch->opaque = opaque;
>
> - return watch;
> -}
> -
> -static void watch_update_mask(SpiceWatch *watch, int event_mask)
> -{
> - GIOCondition condition = spice_event_to_condition(event_mask);
> + watch_update_mask(watch, event_mask);
>
> - g_source_remove(watch->source_id);
> - if (condition != 0)
> - watch->source_id = g_io_add_watch(watch->channel, condition, watch_func, watch);
> + return watch;
> }
>
> static void watch_remove(SpiceWatch *watch)
> {
> - g_source_remove(watch->source_id);
> + watch_update_mask(watch, 0);
you are using watch_update_mask to release the source here?
> + spice_assert(watch->source == NULL);
> +
> g_io_channel_unref(watch->channel);
> free(watch);
> }
>
> -static void channel_event(int event, SpiceChannelEventInfo *info)
> -{
> - DPRINTF(0, "channel event con, type, id, event: %d, %d, %d, %d",
> - info->connection_id, info->type, info->id, event);
> -}
> +static SpiceCoreInterface core = {
> + .base = {
> + .major_version = SPICE_INTERFACE_CORE_MAJOR,
> + .minor_version = SPICE_INTERFACE_CORE_MINOR,
> + },
> + .timer_add = timer_add,
> + .timer_start = timer_start,
> + .timer_cancel = timer_cancel,
> + .timer_remove = timer_remove,
> +
> + .watch_add = watch_add,
> + .watch_update_mask = watch_update_mask,
> + .watch_remove = watch_remove,
> +
> + .channel_event = channel_event,
> +};
>
> void basic_event_loop_mainloop(void)
> {
> - GMainLoop *loop = g_main_loop_new(NULL, FALSE);
> + GMainLoop *loop = g_main_loop_new(main_context, FALSE);
>
> g_main_loop_run(loop);
> g_main_loop_unref(loop);
> @@ -185,23 +224,10 @@ static void ignore_sigpipe(void)
> sigaction(SIGPIPE, &act, NULL);
> }
>
> -static SpiceCoreInterface core = {
> - .base = {
> - .major_version = SPICE_INTERFACE_CORE_MAJOR,
> - .minor_version = SPICE_INTERFACE_CORE_MINOR,
> - },
> - .timer_add = timer_add,
> - .timer_start = timer_start,
> - .timer_cancel = timer_cancel,
> - .timer_remove = timer_remove,
> - .watch_add = watch_add,
> - .watch_update_mask = watch_update_mask,
> - .watch_remove = watch_remove,
> - .channel_event = channel_event,
> -};
Was it necessary to move the core?
The other changes looks good,
toso
> -
> SpiceCoreInterface *basic_event_loop_init(void)
> {
> ignore_sigpipe();
> + spice_assert(main_context == NULL);
> + main_context = g_main_context_new();
> return &core;
> }
> diff --git a/server/tests/basic_event_loop.h b/server/tests/basic_event_loop.h
> index 8220893e..2ec9446 100644
> --- a/server/tests/basic_event_loop.h
> +++ b/server/tests/basic_event_loop.h
> @@ -19,7 +19,9 @@
> #define __BASIC_EVENT_LOOP_H__
>
> #include <spice.h>
> +#include <glib.h>
>
> +GMainContext *basic_event_loop_get_context(void);
> SpiceCoreInterface *basic_event_loop_init(void);
> void basic_event_loop_mainloop(void);
>
> diff --git a/server/tests/replay.c b/server/tests/replay.c
> index ef6c7fb..a652efd 100644
> --- a/server/tests/replay.c
> +++ b/server/tests/replay.c
> @@ -53,7 +53,7 @@ static GAsyncQueue *aqueue = NULL;
> static long total_size;
>
> static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> -static guint fill_source_id = 0;
> +static GSource *fill_source = NULL;
>
>
> #define MEM_SLOT_GROUP_ID 0
> @@ -125,7 +125,11 @@ static gboolean fill_queue_idle(gpointer user_data)
> end:
> if (!keep) {
> pthread_mutex_lock(&mutex);
> - fill_source_id = 0;
> + if (fill_source) {
> + g_source_destroy(fill_source);
> + g_source_unref(fill_source);
> + fill_source = NULL;
> + }
> pthread_mutex_unlock(&mutex);
> }
> spice_qxl_wakeup(&display_sin);
> @@ -140,10 +144,12 @@ static void fill_queue(void)
> if (!started)
> goto end;
>
> - if (fill_source_id != 0)
> + if (fill_source)
> goto end;
>
> - fill_source_id = g_idle_add(fill_queue_idle, NULL);
> + fill_source = g_idle_source_new();
> + g_source_set_callback(fill_source, fill_queue_idle, NULL, NULL);
> + g_source_attach(fill_source, basic_event_loop_get_context());
>
> end:
> pthread_mutex_unlock(&mutex);
> @@ -178,7 +184,7 @@ static int req_cmd_notification(QXLInstance *qin)
> return TRUE;
>
> g_printerr("id: %d, queue length: %d",
> - fill_source_id, g_async_queue_length(aqueue));
> + g_source_get_id(fill_source), g_async_queue_length(aqueue));
>
> return TRUE;
> }
> @@ -352,7 +358,7 @@ int main(int argc, char **argv)
> fill_queue();
> }
>
> - loop = g_main_loop_new(NULL, FALSE);
> + loop = g_main_loop_new(basic_event_loop_get_context(), FALSE);
> g_main_loop_run(loop);
>
> end_replay();
> --
> 2.4.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel