[Spice-devel] [PATCH 1/5] worker: use glib main loop

Frediano Ziglio fziglio at redhat.com
Fri Oct 16 04:04:22 PDT 2015


> 
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> Clean up, more extensible.
> 
> Avoid server hanging when no client are connected.
> ---
>  server/Makefile.am         |   2 -
>  server/red_worker.c        | 439
>  +++++++++++++++++++++++++--------------------
>  server/spice_timer_queue.c | 273 ----------------------------
>  server/spice_timer_queue.h |  43 -----
>  4 files changed, 242 insertions(+), 515 deletions(-)
>  delete mode 100644 server/spice_timer_queue.c
>  delete mode 100644 server/spice_timer_queue.h
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index fad1cbc..78ccac9 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -119,8 +119,6 @@ libspice_server_la_SOURCES =			\
>  	snd_worker.h				\
>  	stat.h					\
>  	spicevmc.c				\
> -	spice_timer_queue.c			\
> -	spice_timer_queue.h			\
>  	zlib_encoder.c				\
>  	zlib_encoder.h				\
>  	spice_bitmap_utils.h		\
> diff --git a/server/red_worker.c b/server/red_worker.c
> index c96c60a..6cc5a35 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -86,7 +86,6 @@
>  #include "dispatcher.h"
>  #include "main_channel.h"
>  #include "migration_protocol.h"
> -#include "spice_timer_queue.h"
>  #include "main_dispatcher.h"
>  #include "spice_server_utils.h"
>  #include "red_time.h"
> @@ -270,13 +269,6 @@ static inline double stat_byte_to_mega(uint64_t size)
>  #endif
>  
>  #define MAX_EVENT_SOURCES 20
> -#define INF_EVENT_WAIT ~0
> -
> -struct SpiceWatch {
> -    struct RedWorker *worker;
> -    SpiceWatchFunc watch_func;
> -    void *watch_func_opaque;
> -};
>  
>  enum {
>      BUF_TYPE_RAW = 1,
> @@ -917,6 +909,7 @@ typedef struct ItemTrace {
>  #define NUM_CURSORS 100
>  
>  typedef struct RedWorker {
> +    GMainContext *main_context;
>      DisplayChannel *display_channel;
>      CursorChannel *cursor_channel;
>      QXLInstance *qxl;
> @@ -926,9 +919,7 @@ typedef struct RedWorker {
>      int id;
>      int running;
>      uint32_t *pending;
> -    struct pollfd poll_fds[MAX_EVENT_SOURCES];
> -    struct SpiceWatch watches[MAX_EVENT_SOURCES];
> -    unsigned int event_timeout;
> +    gint timeout;
>      uint32_t repoll_cmd_ring;
>      uint32_t repoll_cursor_ring;
>      uint32_t num_renderers;
> @@ -2766,48 +2757,6 @@ static void
> red_streams_update_visible_region(RedWorker *worker, Drawable *drawa
>      }
>  }
>  
> -static inline unsigned int red_get_streams_timout(RedWorker *worker)
> -{
> -    unsigned int timout = -1;
> -    Ring *ring = &worker->streams;
> -    RingItem *item = ring;
> -    struct timespec time;
> -
> -    clock_gettime(CLOCK_MONOTONIC, &time);
> -    red_time_t now = timespec_to_red_time(&time);
> -    while ((item = ring_next(ring, item))) {
> -        Stream *stream;
> -
> -        stream = SPICE_CONTAINEROF(item, Stream, link);
> -        red_time_t delta = (stream->last_time + RED_STREAM_TIMOUT) - now;
> -
> -        if (delta < 1000 * 1000) {
> -            return 0;
> -        }
> -        timout = MIN(timout, (unsigned int)(delta / (1000 * 1000)));
> -    }
> -    return timout;
> -}
> -
> -static inline void red_handle_streams_timout(RedWorker *worker)
> -{
> -    Ring *ring = &worker->streams;
> -    struct timespec time;
> -    RingItem *item;
> -
> -    clock_gettime(CLOCK_MONOTONIC, &time);
> -    red_time_t now = timespec_to_red_time(&time);
> -    item = ring_get_head(ring);
> -    while (item) {
> -        Stream *stream = SPICE_CONTAINEROF(item, Stream, link);
> -        item = ring_next(ring, item);
> -        if (now >= (stream->last_time + RED_STREAM_TIMOUT)) {
> -            red_detach_stream_gracefully(worker, stream, NULL);
> -            red_stop_stream(worker, stream);
> -        }
> -    }
> -}
> -
>  static void red_display_release_stream(RedWorker *worker, StreamAgent
>  *agent)
>  {
>      spice_assert(agent->stream);
> @@ -4722,7 +4671,9 @@ static int red_process_cursor(RedWorker *worker,
> uint32_t max_pipe_size, int *ri
>              *ring_is_empty = TRUE;
>              if (worker->repoll_cursor_ring < CMD_RING_POLL_RETRIES) {
>                  worker->repoll_cursor_ring++;
> -                worker->event_timeout = MIN(worker->event_timeout,
> CMD_RING_POLL_TIMEOUT);
> +                worker->timeout = worker->timeout == -1 ?
> +                    CMD_RING_POLL_TIMEOUT :
> +                    MIN(worker->timeout, CMD_RING_POLL_TIMEOUT);
>                  return n;
>              }
>              if (worker->repoll_cursor_ring > CMD_RING_POLL_RETRIES ||

Perhaps could be better to initialize the timeout to a dummy huge (like INT_MAX)
value instead of adding the check here. This would remove this change.

> @@ -4768,7 +4719,6 @@ static int red_process_commands(RedWorker *worker,
> uint32_t max_pipe_size, int *
>  {
>      QXLCommandExt ext_cmd;
>      int n = 0;
> -    uint64_t start = red_now();
>  
>      if (!worker->running) {
>          *ring_is_empty = TRUE;
> @@ -4777,14 +4727,30 @@ static int red_process_commands(RedWorker *worker,
> uint32_t max_pipe_size, int *
>  
>      worker->process_commands_generation++;
>      *ring_is_empty = FALSE;
> -    while (!display_is_connected(worker) ||
> -           // TODO: change to average pipe size?
> -           red_channel_min_pipe_size(&worker->display_channel->common.base)
> <= max_pipe_size) {
> +    for (;;) {
> +
> +        if (display_is_connected(worker)) {
> +
> +            if
> (red_channel_all_blocked(&worker->display_channel->common.base)) {
> +                spice_info("all display clients are blocking");
> +                return n;
> +            }
> +
> +
> +            // TODO: change to average pipe size?
> +            if
> (red_channel_min_pipe_size(&worker->display_channel->common.base) >
> max_pipe_size) {
> +                spice_info("too much item in the display clients pipe
> already");
> +                return n;
> +            }
> +        }
> +

Does this change is necessary just for the main loop?

>          if (!worker->qxl->st->qif->get_command(worker->qxl, &ext_cmd)) {
>              *ring_is_empty = TRUE;;
>              if (worker->repoll_cmd_ring < CMD_RING_POLL_RETRIES) {
>                  worker->repoll_cmd_ring++;
> -                worker->event_timeout = MIN(worker->event_timeout,
> CMD_RING_POLL_TIMEOUT);
> +                worker->timeout = worker->timeout == -1 ?
> +                    CMD_RING_POLL_TIMEOUT :
> +                    MIN(worker->timeout, CMD_RING_POLL_TIMEOUT);

See above

>                  return n;
>              }
>              if (worker->repoll_cmd_ring > CMD_RING_POLL_RETRIES ||
> @@ -4866,13 +4832,8 @@ static int red_process_commands(RedWorker *worker,
> uint32_t max_pipe_size, int *
>              spice_error("bad command type");
>          }
>          n++;
> -        if ((worker->display_channel &&
> -             red_channel_all_blocked(&worker->display_channel->common.base))
> -            || red_now() - start > 10 * 1000 * 1000) {
> -            worker->event_timeout = 0;
> -            return n;
> -        }
>      }
> +
>      return n;
>  }
>  
> @@ -10088,81 +10049,159 @@ static int
> common_channel_config_socket(RedChannelClient *rcc)
>      return TRUE;
>  }
>  
> -static void worker_watch_update_mask(SpiceWatch *watch, int event_mask)
> +typedef struct SpiceTimer {
> +    SpiceTimerFunc func;
> +    void *opaque;
> +    guint source_id;
> +} SpiceTimer;
> +
> +static SpiceTimer* worker_timer_add(SpiceTimerFunc func, void *opaque)
>  {
> -    struct RedWorker *worker;
> -    int i;
> +    SpiceTimer *timer = g_new0(SpiceTimer, 1);
> +
> +    timer->func = func;
> +    timer->opaque = opaque;
> +
> +    return timer;
> +}
> +
> +static gboolean worker_timer_func(gpointer user_data)
> +{
> +    SpiceTimer *timer = user_data;
>  
> -    if (!watch) {
> +    timer->source_id = 0;
> +    timer->func(timer->opaque);
> +    /* timer might be free after func(), don't touch */
> +
> +    return FALSE;
> +}
> +
> +static void worker_timer_cancel(SpiceTimer *timer)
> +{
> +    if (timer->source_id == 0)
>          return;
> -    }
>  
> -    worker = watch->worker;
> -    i = watch - worker->watches;
> +    g_source_remove(timer->source_id);
> +    timer->source_id = 0;
> +}
>  
> -    worker->poll_fds[i].events = 0;
> -    if (event_mask & SPICE_WATCH_EVENT_READ) {
> -        worker->poll_fds[i].events |= POLLIN;
> -    }
> -    if (event_mask & SPICE_WATCH_EVENT_WRITE) {
> -        worker->poll_fds[i].events |= POLLOUT;
> +static void worker_timer_start(SpiceTimer *timer, uint32_t ms)
> +{
> +    worker_timer_cancel(timer);
> +
> +    timer->source_id = g_timeout_add(ms, worker_timer_func, timer);
> +}
> +
> +static void worker_timer_remove(SpiceTimer *timer)
> +{
> +    worker_timer_cancel(timer);
> +    g_free(timer);
> +}
> +
> +static GIOCondition spice_event_to_giocondition(int event_mask)
> +{
> +    GIOCondition condition = 0;
> +
> +    if (event_mask & SPICE_WATCH_EVENT_READ)
> +        condition |= G_IO_IN;
> +    if (event_mask & SPICE_WATCH_EVENT_WRITE)
> +        condition |= G_IO_OUT;
> +
> +    return condition;
> +}
> +
> +static int giocondition_to_spice_event(GIOCondition condition)
> +{
> +    int event = 0;
> +
> +    if (condition & G_IO_IN)
> +        event |= SPICE_WATCH_EVENT_READ;
> +    if (condition & G_IO_OUT)
> +        event |= SPICE_WATCH_EVENT_WRITE;
> +
> +    return event;
> +}
> +
> +struct SpiceWatch {
> +    GIOChannel *channel;
> +    GSource *source;
> +    RedChannelClient *rcc;
> +    SpiceWatchFunc func;
> +};
> +
> +static gboolean watch_func(GIOChannel *source, GIOCondition condition,
> +                           gpointer data)
> +{
> +    SpiceWatch *watch = data;
> +    int fd = g_io_channel_unix_get_fd(source);
> +
> +    watch->func(fd, giocondition_to_spice_event(condition), watch->rcc);
> +
> +    return TRUE;
> +}
> +
> +static void worker_watch_update_mask(SpiceWatch *watch, int events)
> +{
> +    RedWorker *worker;
> +
> +    spice_return_if_fail(watch != NULL);
> +    worker = SPICE_CONTAINEROF(watch->rcc->channel, CommonChannel,
> base)->worker;
> +
> +    if (watch->source) {
> +        g_source_destroy(watch->source);
> +        watch->source = NULL;
>      }
> +
> +    if (!events)
> +        return;
> +
> +    watch->source = g_io_create_watch(watch->channel,
> spice_event_to_giocondition(events));
> +    g_source_set_callback(watch->source, (GSourceFunc)watch_func, watch,
> NULL);
> +    g_source_attach(watch->source, worker->main_context);
>  }
>  
> -static SpiceWatch *worker_watch_add(int fd, int event_mask, SpiceWatchFunc
> func, void *opaque)
> +static SpiceWatch* worker_watch_add(int fd, int events, SpiceWatchFunc func,
> 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;
> -    struct RedWorker *worker;
> -    int i;
> +    RedWorker *worker;
> +    SpiceWatch *watch;
> +
> +    spice_return_val_if_fail(rcc != NULL, NULL);
> +    spice_return_val_if_fail(fd != -1, NULL);
> +    spice_return_val_if_fail(func != NULL, 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;
> +    spice_return_val_if_fail(worker != NULL, NULL);
> +    spice_return_val_if_fail(worker->main_context != NULL, NULL);
>  
> -    /* Search for a free slot in our poll_fds & watches arrays */
> -    for (i = 0; i < MAX_EVENT_SOURCES; i++) {
> -        if (worker->poll_fds[i].fd == -1) {
> -            break;
> -        }
> -    }
> -    if (i == MAX_EVENT_SOURCES) {
> -        spice_warning("could not add a watch for channel type %u id %u",
> -                      rcc->channel->type, rcc->channel->id);
> -        return NULL;
> -    }
> +    watch = g_new0(SpiceWatch, 1);
> +    watch->channel = g_io_channel_unix_new(fd);
> +    watch->rcc = rcc;
> +    watch->func = func;
>  
> -    worker->poll_fds[i].fd = fd;
> -    worker->watches[i].worker = worker;
> -    worker->watches[i].watch_func = func;
> -    worker->watches[i].watch_func_opaque = opaque;
> -    worker_watch_update_mask(&worker->watches[i], event_mask);
> +    worker_watch_update_mask(watch, events);
>  
> -    return &worker->watches[i];
> +    return watch;
>  }
>  
>  static void worker_watch_remove(SpiceWatch *watch)
>  {
> -    if (!watch) {
> -        return;
> -    }
> +    spice_return_if_fail(watch != NULL);
>  
> -    /* Note we don't touch the poll_fd here, to avoid the
> -       poll_fds/watches table entry getting re-used in the same
> -       red_worker_main loop over the fds as it is removed.
> +    if (watch->source)
> +        g_source_destroy(watch->source);
>  
> -       This is done because re-using it while events were pending on
> -       the fd previously occupying the slot would lead to incorrectly
> -       calling the watch_func for the new fd. */
> -    memset(watch, 0, sizeof(SpiceWatch));
> +    g_io_channel_unref(watch->channel);
> +    g_free(watch);
>  }
>  
> -SpiceCoreInterface worker_core = {
> -    .timer_add = spice_timer_queue_add,
> -    .timer_start = spice_timer_set,
> -    .timer_cancel = spice_timer_cancel,
> -    .timer_remove = spice_timer_remove,
> +static const SpiceCoreInterface worker_core = {
> +    .timer_add = worker_timer_add,
> +    .timer_start = worker_timer_start,
> +    .timer_cancel = worker_timer_cancel,
> +    .timer_remove = worker_timer_remove,
>  
>      .watch_update_mask = worker_watch_update_mask,
>      .watch_add = worker_watch_add,
> @@ -11831,23 +11870,91 @@ static void register_callbacks(Dispatcher
> *dispatcher)
>  
>  
>  
> -static void handle_dev_input(int fd, int event, void *opaque)
> +static gboolean worker_dispatcher_cb(GIOChannel *source, GIOCondition
> condition,
> +                                     gpointer data)
>  {
> -    RedWorker *worker = opaque;
> +    RedWorker *worker = data;
>  
> +    spice_debug(NULL);

Why this debug ?

>      dispatcher_handle_recv_read(red_dispatcher_get_dispatcher(worker->red_dispatcher));
> +
> +    return TRUE;
> +}
> +
> +typedef struct _RedWorkerSource {
> +    GSource source;
> +    RedWorker *worker;
> +} RedWorkerSource;
> +
> +static gboolean worker_source_prepare(GSource *source, gint *timeout)
> +{
> +    RedWorkerSource *wsource = (RedWorkerSource *)source;
> +    RedWorker *worker = wsource->worker;
> +
> +    *timeout = worker->timeout;
> +
> +    return FALSE; /* do no timeout poll */
> +}
> +
> +static gboolean worker_source_check(GSource *source)
> +{
> +    RedWorkerSource *wsource = (RedWorkerSource *)source;
> +    RedWorker *worker = wsource->worker;
> +
> +    return worker->running /* TODO && worker->pending_process */;
> +}
> +

Here is slightly different, in the old code the dispatch part was
always executed no matter is running or not. So could be
this function should return TRUE always.

> +static void red_display_cc_free_glz_drawables(RedChannelClient *rcc)
> +{
> +    DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> +
> +    red_display_handle_glz_drawables_to_free(dcc);
>  }
>  

Why moving this function? A declaration was not enough?

> +static gboolean worker_source_dispatch(GSource *source, GSourceFunc
> callback,
> +                                       gpointer user_data)
> +{
> +    RedWorkerSource *wsource = (RedWorkerSource *)source;
> +    RedWorker *worker = wsource->worker;
> +    int ring_is_empty;
> +
> +    if (worker->display_channel) {
> +        /* during migration, in the dest, the display channel can be
> initialized
> +           while the global lz data not since migrate data msg hasn't been
> +           received yet */
> +        /* FIXME: why is this here, and not in display_channel_create */
> +        red_channel_apply_clients(&worker->display_channel->common.base,
> +                                  red_display_cc_free_glz_drawables);
> +    }
> +

This part was always executed, now only if running is true.

> +    worker->timeout = -1;
> +    red_process_cursor(worker, MAX_PIPE_SIZE, &ring_is_empty);
> +    red_process_commands(worker, MAX_PIPE_SIZE, &ring_is_empty);
> +

These two lines were executed only if running

> +    /* FIXME: remove me? that should be handled by watch out condition */
> +    red_push(worker);
> +

I don't understand the FIXME.

> +    return TRUE;
> +}
> +
> +/* cannot be const */
> +static GSourceFuncs worker_source_funcs = {
> +    .prepare = worker_source_prepare,
> +    .check = worker_source_check,
> +    .dispatch = worker_source_dispatch,
> +};
> +
>  static RedWorker* red_worker_new(WorkerInitData *init_data)
>  {
>      RedWorker *worker = spice_new0(RedWorker, 1);
>      RedWorkerMessage message;
>      Dispatcher *dispatcher;
> -    int i;
>      const char *record_filename;
>  
>      spice_assert(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
>  
> +    worker->main_context = g_main_context_new();
> +
>      record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
>      if (record_filename) {
>          static const char header[] = "SPICE_REPLAY 1\n";
> @@ -11898,15 +12005,18 @@ static RedWorker* red_worker_new(WorkerInitData
> *init_data)
>      worker->wakeup_counter = stat_add_counter(worker->stat, "wakeups",
>      TRUE);
>      worker->command_counter = stat_add_counter(worker->stat, "commands",
>      TRUE);
>  #endif
> -    for (i = 0; i < MAX_EVENT_SOURCES; i++) {
> -        worker->poll_fds[i].fd = -1;
> -    }
>  
> -    worker->poll_fds[0].fd = worker->channel;
> -    worker->poll_fds[0].events = POLLIN;
> -    worker->watches[0].worker = worker;
> -    worker->watches[0].watch_func = handle_dev_input;
> -    worker->watches[0].watch_func_opaque = worker;
> +    GIOChannel *channel =
> g_io_channel_unix_new(dispatcher_get_recv_fd(dispatcher));
> +    GSource *source = g_io_create_watch(channel, G_IO_IN);
> +    g_source_set_callback(source, (GSourceFunc)worker_dispatcher_cb, worker,
> NULL);

Cast here should not be needed, function should be declared with correct types.
Style: declaration inside code.

> +    g_source_attach(source, worker->main_context);
> +    g_source_unref(source);
> +
> +    source = g_source_new(&worker_source_funcs, sizeof(RedWorkerSource));
> +    RedWorkerSource *wsource = (RedWorkerSource *)source;
> +    wsource->worker = worker;

usually we don't use declarations inside code, could be replaced with 

   ((RedWorkerSource *) source)->worker = worker;

> +    g_source_attach(source, worker->main_context);
> +    g_source_unref(source);
>  
>      red_memslot_info_init(&worker->mem_slots,
>                            init_data->num_memslots_groups,
> @@ -11918,9 +12028,6 @@ static RedWorker* red_worker_new(WorkerInitData
> *init_data)
>      spice_warn_if(init_data->n_surfaces > NUM_SURFACES);
>      worker->n_surfaces = init_data->n_surfaces;
>  
> -    if (!spice_timer_queue_create()) {
> -        spice_error("failed to create timer queue");
> -    }
>      message = RED_WORKER_MESSAGE_READY;
>      write_message(worker->channel, &message);
>  
> @@ -11931,18 +12038,11 @@ static RedWorker* red_worker_new(WorkerInitData
> *init_data)
>      red_init_lz4(worker);
>  #endif
>      red_init_zlib(worker);
> -    worker->event_timeout = INF_EVENT_WAIT;
> +    worker->timeout = -1;

Use "infinite" value and check on callback? See comment above.

>  
>      return worker;
>  }
>  
> -static void red_display_cc_free_glz_drawables(RedChannelClient *rcc)
> -{
> -    DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> -
> -    red_display_handle_glz_drawables_to_free(dcc);
> -}
> -

Just required as function was moved.

>  SPICE_GNUC_NORETURN void *red_worker_main(void *arg)
>  {
>      RedWorker *worker = red_worker_new(arg);
> @@ -11955,65 +12055,10 @@ SPICE_GNUC_NORETURN void *red_worker_main(void
> *arg)
>          spice_error("pthread_getcpuclockid failed");
>      }
>  
> -    for (;;) {
> -        int i, num_events;
> -        unsigned int timeout;
> -
> -        timeout = spice_timer_queue_get_timeout_ms();
> -        worker->event_timeout = MIN(timeout, worker->event_timeout);
> -        timeout = red_get_streams_timout(worker);
> -        worker->event_timeout = MIN(timeout, worker->event_timeout);
> -        num_events = poll(worker->poll_fds, MAX_EVENT_SOURCES,
> worker->event_timeout);
> -        red_handle_streams_timout(worker);
> -        spice_timer_queue_cb();
> -
> -        if (worker->display_channel) {
> -            /* during migration, in the dest, the display channel can be
> initialized
> -               while the global lz data not since migrate data msg hasn't
> been
> -               received yet */
> -            red_channel_apply_clients(&worker->display_channel->common.base,
> -                                      red_display_cc_free_glz_drawables);
> -        }
> -
> -        worker->event_timeout = INF_EVENT_WAIT;
> -        if (num_events == -1) {
> -            if (errno != EINTR) {
> -                spice_error("poll failed, %s", strerror(errno));
> -            }
> -        }
> -
> -        for (i = 0; i < MAX_EVENT_SOURCES; i++) {
> -            /* The watch may have been removed by the watch-func from
> -               another fd (ie a disconnect through the dispatcher),
> -               in this case watch_func is NULL. */
> -            if (worker->poll_fds[i].revents &&
> worker->watches[i].watch_func) {
> -                int events = 0;
> -                if (worker->poll_fds[i].revents & POLLIN) {
> -                    events |= SPICE_WATCH_EVENT_READ;
> -                }
> -                if (worker->poll_fds[i].revents & POLLOUT) {
> -                    events |= SPICE_WATCH_EVENT_WRITE;
> -                }
> -                worker->watches[i].watch_func(worker->poll_fds[i].fd,
> events,
> -
> worker->watches[i].watch_func_opaque);
> -            }
> -        }
> -
> -        /* Clear the poll_fd for any removed watches, see the comment in
> -           watch_remove for why we don't do this there. */
> -        for (i = 0; i < MAX_EVENT_SOURCES; i++) {
> -            if (!worker->watches[i].watch_func) {
> -                worker->poll_fds[i].fd = -1;
> -            }
> -        }
> -
> -        if (worker->running) {
> -            int ring_is_empty;
> -            red_process_cursor(worker, MAX_PIPE_SIZE, &ring_is_empty);
> -            red_process_commands(worker, MAX_PIPE_SIZE, &ring_is_empty);
> -        }
> -        red_push(worker);
> -    }
> +    GMainLoop *loop = g_main_loop_new(worker->main_context, FALSE);
> +    g_main_loop_run(loop);
> +    g_main_loop_unref(loop);
>  
> -    spice_warn_if_reached();
> +    /* FIXME: free worker, and join threads */
> +    abort();

why replacing a spice_warn_if_reached with an abort?

>  }
> diff --git a/server/spice_timer_queue.c b/server/spice_timer_queue.c
> deleted file mode 100644
> index c4f2f6e..0000000
> --- a/server/spice_timer_queue.c
> +++ /dev/null
> @@ -1,273 +0,0 @@
> -/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> -/*
> -   Copyright (C) 2013 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/>.
> -*/
> -#include <config.h>
> -#include <pthread.h>
> -#include "red_common.h"
> -#include "spice_timer_queue.h"
> -#include "common/ring.h"
> -
> -static Ring timer_queue_list;
> -static int queue_count = 0;
> -static pthread_mutex_t queue_list_lock = PTHREAD_MUTEX_INITIALIZER;
> -
> -static void spice_timer_queue_init(void)
> -{
> -    ring_init(&timer_queue_list);
> -}
> -
> -struct SpiceTimer {
> -    RingItem link;
> -    RingItem active_link;
> -
> -    SpiceTimerFunc func;
> -    void *opaque;
> -
> -    SpiceTimerQueue *queue;
> -
> -    int is_active;
> -    uint32_t ms;
> -    uint64_t expiry_time;
> -};
> -
> -struct SpiceTimerQueue {
> -    RingItem link;
> -    pthread_t thread;
> -    Ring timers;
> -    Ring active_timers;
> -};
> -
> -static SpiceTimerQueue *spice_timer_queue_find(void)
> -{
> -    pthread_t self = pthread_self();
> -    RingItem *queue_item;
> -
> -    RING_FOREACH(queue_item, &timer_queue_list) {
> -         SpiceTimerQueue *queue = SPICE_CONTAINEROF(queue_item,
> SpiceTimerQueue, link);
> -
> -         if (pthread_equal(self, queue->thread) != 0) {
> -            return queue;
> -         }
> -    }
> -
> -    return NULL;
> -}
> -
> -static SpiceTimerQueue *spice_timer_queue_find_with_lock(void)
> -{
> -    SpiceTimerQueue *queue;
> -
> -    pthread_mutex_lock(&queue_list_lock);
> -    queue = spice_timer_queue_find();
> -    pthread_mutex_unlock(&queue_list_lock);
> -    return queue;
> -}
> -
> -int spice_timer_queue_create(void)
> -{
> -    SpiceTimerQueue *queue;
> -
> -    pthread_mutex_lock(&queue_list_lock);
> -    if (queue_count == 0) {
> -        spice_timer_queue_init();
> -    }
> -
> -    if (spice_timer_queue_find() != NULL) {
> -        spice_printerr("timer queue was already created for the thread");
> -        return FALSE;
> -    }
> -
> -    queue = spice_new0(SpiceTimerQueue, 1);
> -    queue->thread = pthread_self();
> -    ring_init(&queue->timers);
> -    ring_init(&queue->active_timers);
> -
> -    ring_add(&timer_queue_list, &queue->link);
> -    queue_count++;
> -
> -    pthread_mutex_unlock(&queue_list_lock);
> -
> -    return TRUE;
> -}
> -
> -void spice_timer_queue_destroy(void)
> -{
> -    RingItem *item;
> -    SpiceTimerQueue *queue;
> -
> -    pthread_mutex_lock(&queue_list_lock);
> -    queue = spice_timer_queue_find();
> -
> -    spice_assert(queue != NULL);
> -
> -    while ((item = ring_get_head(&queue->timers))) {
> -        SpiceTimer *timer;
> -
> -        timer = SPICE_CONTAINEROF(item, SpiceTimer, link);
> -        spice_timer_remove(timer);
> -    }
> -
> -    ring_remove(&queue->link);
> -    free(queue);
> -    queue_count--;
> -
> -    pthread_mutex_unlock(&queue_list_lock);
> -}
> -
> -SpiceTimer *spice_timer_queue_add(SpiceTimerFunc func, void *opaque)
> -{
> -    SpiceTimer *timer = spice_new0(SpiceTimer, 1);
> -    SpiceTimerQueue *queue = spice_timer_queue_find_with_lock();
> -
> -    spice_assert(queue != NULL);
> -
> -    ring_item_init(&timer->link);
> -    ring_item_init(&timer->active_link);
> -
> -    timer->opaque = opaque;
> -    timer->func = func;
> -    timer->queue = queue;
> -
> -    ring_add(&queue->timers, &timer->link);
> -
> -    return timer;
> -}
> -
> -static void _spice_timer_set(SpiceTimer *timer, uint32_t ms, uint64_t now)
> -{
> -    RingItem *next_item;
> -    SpiceTimerQueue *queue;
> -
> -    if (timer->is_active) {
> -        spice_timer_cancel(timer);
> -    }
> -
> -    queue = timer->queue;
> -    timer->expiry_time = now + ms;
> -    timer->ms = ms;
> -
> -    RING_FOREACH(next_item, &queue->active_timers) {
> -        SpiceTimer *next_timer = SPICE_CONTAINEROF(next_item, SpiceTimer,
> active_link);
> -
> -        if (timer->expiry_time <= next_timer->expiry_time) {
> -            break;
> -        }
> -    }
> -
> -    if (next_item) {
> -        ring_add_before(&timer->active_link, next_item);
> -    } else {
> -        ring_add_before(&timer->active_link, &queue->active_timers);
> -    }
> -    timer->is_active = TRUE;
> -}
> -
> -void spice_timer_set(SpiceTimer *timer, uint32_t ms)
> -{
> -    struct timespec now;
> -
> -    spice_assert(pthread_equal(timer->queue->thread, pthread_self()) != 0);
> -
> -    clock_gettime(CLOCK_MONOTONIC, &now);
> -    _spice_timer_set(timer, ms,
> -                     (uint64_t)now.tv_sec * 1000 + (now.tv_nsec / 1000 /
> 1000));
> -}
> -
> -void spice_timer_cancel(SpiceTimer *timer)
> -{
> -    spice_assert(pthread_equal(timer->queue->thread, pthread_self()) != 0);
> -
> -    if (!ring_item_is_linked(&timer->active_link)) {
> -        spice_assert(!timer->is_active);
> -        return;
> -    }
> -
> -    spice_assert(timer->is_active);
> -    ring_remove(&timer->active_link);
> -    timer->is_active = FALSE;
> -}
> -
> -void spice_timer_remove(SpiceTimer *timer)
> -{
> -    spice_assert(timer->queue);
> -    spice_assert(ring_item_is_linked(&timer->link));
> -    spice_assert(pthread_equal(timer->queue->thread, pthread_self()) != 0);
> -
> -    if (timer->is_active) {
> -        spice_assert(ring_item_is_linked(&timer->active_link));
> -        ring_remove(&timer->active_link);
> -    }
> -    ring_remove(&timer->link);
> -    free(timer);
> -}
> -
> -unsigned int spice_timer_queue_get_timeout_ms(void)
> -{
> -    struct timespec now;
> -    int64_t now_ms;
> -    RingItem *head;
> -    SpiceTimer *head_timer;
> -    SpiceTimerQueue *queue = spice_timer_queue_find_with_lock();
> -
> -    spice_assert(queue != NULL);
> -
> -    if (ring_is_empty(&queue->active_timers)) {
> -        return -1;
> -    }
> -
> -    head = ring_get_head(&queue->active_timers);
> -    head_timer = SPICE_CONTAINEROF(head, SpiceTimer, active_link);
> -
> -    clock_gettime(CLOCK_MONOTONIC, &now);
> -    now_ms = ((int64_t)now.tv_sec * 1000) + (now.tv_nsec / 1000 / 1000);
> -
> -    return MAX(0, ((int64_t)head_timer->expiry_time - now_ms));
> -}
> -
> -
> -void spice_timer_queue_cb(void)
> -{
> -    struct timespec now;
> -    uint64_t now_ms;
> -    RingItem *head;
> -    SpiceTimerQueue *queue = spice_timer_queue_find_with_lock();
> -
> -    spice_assert(queue != NULL);
> -
> -    if (ring_is_empty(&queue->active_timers)) {
> -        return;
> -    }
> -
> -    clock_gettime(CLOCK_MONOTONIC, &now);
> -    now_ms = ((uint64_t)now.tv_sec * 1000) + (now.tv_nsec / 1000 / 1000);
> -
> -    while ((head = ring_get_head(&queue->active_timers))) {
> -        SpiceTimer *timer = SPICE_CONTAINEROF(head, SpiceTimer,
> active_link);
> -
> -        if (timer->expiry_time > now_ms) {
> -            break;
> -        } else {
> -            /* Remove active timer before calling the timer function.
> -             * Timer function could delete the timer making the timer
> -             * pointer point to freed data.
> -             */
> -            spice_timer_cancel(timer);
> -            timer->func(timer->opaque);
> -            /* timer could now be invalid ! */
> -        }
> -    }
> -}
> diff --git a/server/spice_timer_queue.h b/server/spice_timer_queue.h
> deleted file mode 100644
> index a84f6cd..0000000
> --- a/server/spice_timer_queue.h
> +++ /dev/null
> @@ -1,43 +0,0 @@
> -/*
> -   Copyright (C) 2013 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/>.
> -*/
> -
> -#ifndef _H_SPICE_TIMER_QUEUE
> -#define _H_SPICE_TIMER_QUEUE
> -
> -#include  <stdint.h>
> -#include "spice.h"
> -
> -typedef struct SpiceTimerQueue SpiceTimerQueue;
> -
> -/* create/destroy a timer queue for the current thread.
> - * In order to execute the timers functions, spice_timer_queue_cb should be
> called
> - * periodically, according to spice_timer_queue_get_timeout_ms */
> -int spice_timer_queue_create(void);
> -void spice_timer_queue_destroy(void);
> -
> -SpiceTimer *spice_timer_queue_add(SpiceTimerFunc func, void *opaque);
> -void spice_timer_set(SpiceTimer *timer, uint32_t ms);
> -void spice_timer_cancel(SpiceTimer *timer);
> -void spice_timer_remove(SpiceTimer *timer);
> -
> -/* returns the time left till the earliest timer in the queue expires.
> - * returns (unsigned)-1 if there are no active timers */
> -unsigned int spice_timer_queue_get_timeout_ms(void);
> -/* call the timeout callbacks of all the expired timers */
> -void spice_timer_queue_cb(void);
> -
> -#endif


Frediano


More information about the Spice-devel mailing list