>
> On Tue, Jan 19, 2016 at 11:54:47AM +0000, Frediano Ziglio wrote:
> > These set of patches try to improve current event loop code.
> >
> > Patches improve test event loop, make a template from
> > them and use for main loop.
> > These patches also improve glib main loop patch from Marc and
> > supercede fixes for timers reducing original patch size and making
> > easier to test code.
> >
> > Changes from v3:
> > - reintroduced template file. This is necessary after libserver library.
> > The reason is that the module file was using some function exported
> > by red-worker.c (and so libserver) while the same module is used by
> > libtest, included by all tests so using libtest and libserver was
> > not possible;
>
> Not exactly sure what issue you are describing here. The patch below
> seems to be working for me (both make check and the VMs I've tested
> with). The tests/Makefile.am changes are probably a bit rough. I have a
> split series I can send if this makes sense.
>
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 5d65526..411a0d9 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -70,6 +70,7 @@ libserver_la_SOURCES = \
> char-device.c \
> char-device.h \
> demarshallers.h \
> + event-loop.c \
> glz-encoder.c \
> glz-encoder.h \
> glz-encoder-dict.c \
> @@ -157,7 +158,6 @@ 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.c
> similarity index 91%
> rename from server/event-loop.tmpl.c
> rename to server/event-loop.c
> index 9d253d9..7581437 100644
> --- a/server/event-loop.tmpl.c
> +++ b/server/event-loop.c
> @@ -23,11 +23,6 @@
> * This file export a variable:
> *
> * SpiceCoreInterfaceInternal event_loop_core;
> - *
> - * You should also define some functions like:
> - *
> - * GMainContext *event_loop_context_from_iface(const
> SpiceCoreInterfaceInternal *opaque);
> - * void event_loop_channel_event(int event, SpiceChannelEventInfo *info);
> */
>
> #include "red-common.h"
> @@ -44,7 +39,7 @@ static SpiceTimer* timer_add(const
> SpiceCoreInterfaceInternal *iface,
> {
> SpiceTimer *timer = spice_malloc0(sizeof(SpiceTimer));
>
> - timer->context = event_loop_context_from_iface(iface);
> + timer->context = iface->main_context;
> timer->func = func;
> timer->opaque = opaque;
>
> @@ -157,7 +152,7 @@ static SpiceWatch *watch_add(const
> SpiceCoreInterfaceInternal *iface,
> spice_return_val_if_fail(func != NULL, NULL);
>
> watch = spice_malloc0(sizeof(SpiceWatch));
> - watch->context = event_loop_context_from_iface(iface);
> + watch->context = iface->main_context;
> watch->channel = g_io_channel_unix_new(fd);
> watch->func = func;
> watch->opaque = opaque;
> @@ -176,7 +171,7 @@ static void watch_remove(SpiceWatch *watch)
> free(watch);
> }
>
> -static SpiceCoreInterfaceInternal event_loop_core = {
> +SpiceCoreInterfaceInternal event_loop_core = {
> .timer_add = timer_add,
> .timer_start = timer_start,
> .timer_cancel = timer_cancel,
> @@ -185,6 +180,4 @@ static SpiceCoreInterfaceInternal event_loop_core = {
> .watch_add = watch_add,
> .watch_update_mask = watch_update_mask,
> .watch_remove = watch_remove,
> -
> - .channel_event = event_loop_channel_event
> };
> diff --git a/server/red-common.h b/server/red-common.h
> index 253dc45..90a7d20 100644
> --- a/server/red-common.h
> +++ b/server/red-common.h
> @@ -52,6 +52,10 @@ struct SpiceCoreInterfaceInternal {
> void (*watch_remove)(SpiceWatch *watch);
>
> void (*channel_event)(int event, SpiceChannelEventInfo *info);
> +
> + GMainContext *main_context;
> };
>
> +extern SpiceCoreInterfaceInternal event_loop_core;
> +
> #endif
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 24bb435..3d48968 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -516,12 +516,6 @@ static inline GMainContext
> *event_loop_context_from_iface(const SpiceCoreInterfa
> return worker->main_context;
> }
>
> -static void event_loop_channel_event(int event, SpiceChannelEventInfo *info)
> -{
> -}
> -
> -#include "event-loop.tmpl.c"
> -
> CommonChannelClient *common_channel_new_client(CommonChannel *common,
> int size,
> RedClient *client,
> @@ -1541,8 +1535,9 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> RedDispatcher *red_dispatcher)
> qxl->st->qif->get_init_info(qxl, &init_info);
>
> worker = spice_new0(RedWorker, 1);
> - worker->core = event_loop_core;
> worker->main_context = g_main_context_new();
> + event_loop_core.main_context = worker->main_context;
> + worker->core = event_loop_core;
>
This should be
worker = spice_new0(RedWorker, 1);
worker->core = event_loop_core;
worker->core.main_context = g_main_context_new();
> record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
> if (record_filename) {
> diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> index 6f02c99..000b097 100644
> --- a/server/tests/Makefile.am
> +++ b/server/tests/Makefile.am
> @@ -27,10 +27,15 @@ libtest_a_SOURCES = \
> test_display_base.h \
> $(NULL)
>
> +libtest_a_LIBADD = \
> + $(top_builddir)/server/libserver.la \
> + $(top_builddir)/spice-common/common/libspice-common.la \
> + $(NULL)
> +
I think this is void for a static library
> LDADD = \
> libtest.a \
> + $(top_builddir)/server/libserver.la \
> $(top_builddir)/spice-common/common/libspice-common.la \
> - $(top_builddir)/server/libspice-server.la \
> $(GLIB2_LIBS) \
> $(SPICE_NONPKGCONFIG_LIBS) \
> $(NULL)
So by default you add any possible library?
Isn't a bit overkill? What if you want to test a specific source?
I suppose we can just override the LDADD for specific tests.
> @@ -70,8 +75,6 @@ noinst_LIBRARIES += \
>
> spice_server_replay_SOURCES = replay.c
>
> -stream_test_LDADD = ../libserver.la $(LDADD)
> -
> stat_test_SOURCES = stat-main.c
> stat_test_LDADD = \
> libstat_test1.a \
> diff --git a/server/tests/basic_event_loop.c
> b/server/tests/basic_event_loop.c
> index 997d251..3f1bc71 100644
> --- a/server/tests/basic_event_loop.c
> +++ b/server/tests/basic_event_loop.c
> @@ -43,19 +43,12 @@ GMainContext *basic_event_loop_get_context(void)
> return main_context;
> }
>
> -static inline GMainContext *event_loop_context_from_iface(const
> SpiceCoreInterfaceInternal *iface)
> -{
> - return main_context;
> -}
> -
> static void event_loop_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);
> }
>
> -#include "../event-loop.tmpl.c"
> -
> void basic_event_loop_mainloop(void)
> {
> GMainLoop *loop = g_main_loop_new(main_context, FALSE);
> @@ -76,12 +69,12 @@ static void ignore_sigpipe(void)
>
> static SpiceTimer* base_timer_add(SpiceTimerFunc func, void *opaque)
> {
> - return event_loop_core.timer_add(NULL, func, opaque);
> + return event_loop_core.timer_add(&event_loop_core, func, opaque);
> }
>
> static SpiceWatch *base_watch_add(int fd, int event_mask, SpiceWatchFunc
> func, void *opaque)
> {
> - return event_loop_core.watch_add(NULL, fd, event_mask, func, opaque);
> + return event_loop_core.watch_add(&event_loop_core, fd, event_mask, func,
> opaque);
> }
>
> static SpiceCoreInterface core = {
> @@ -91,7 +84,6 @@ static SpiceCoreInterface core = {
> },
> .timer_add = base_timer_add,
> .watch_add = base_watch_add,
> - .channel_event = event_loop_channel_event,
> };
>
> SpiceCoreInterface *basic_event_loop_init(void)
> @@ -104,6 +96,9 @@ SpiceCoreInterface *basic_event_loop_init(void)
> core.timer_remove = event_loop_core.timer_remove;
> core.watch_update_mask = event_loop_core.watch_update_mask;
> core.watch_remove = event_loop_core.watch_remove;
> + event_loop_core.channel_event = core.channel_event =
> event_loop_channel_event;
> + event_loop_core.main_context = main_context;
> +
> return &core;
> }
>
>
>
Otherwise the patch is really good!
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel