>
> 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?
>
Is intentional to delete the source and create a new one.
> > +
> > + watch->source = g_io_create_watch(watch->channel,
> > spice_event_to_giocondition(event_mask));
actually I cannot find any other way to change the event_mask
without destroying the old and creating a new one.
> > + 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?
>
yes, calling with 0 as mask cause the source to be destroyed.
> > + 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?
>
No, reduce difference on next patch but can be avoided.
Also the move of channel_event can be avoided.
> The other changes looks good,
> toso
>
Frediano
> > -
> > 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();
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel