[Spice-devel] [PATCH] tests: add a new case for event loop timers

[Frediano Ziglio]

> 
> Check that cancelling a timer the timer callback is not called.
> 
> This can happen in latency code (red-channel.c).
> In red_channel_client_cancel_ping_timer latency timer is cancelled and
> state is set to PING_STATE_NONE however if timer was already active what
> happens is that the red_channel_client_ping_timer is called and the line
> 
>   spice_assert(rcc->latency_monitor.state == PING_STATE_TIMER);
> 
> is triggered causing spice-server to abort.
> This happens as GLib loop add all active sources to an array but if the
> timer is deactivated before the event is dispatched the event will be
> dispatched unless the source is destroyed.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Small note:
This tests make the "make check" fails, this is expected the current
code does not handle it. I had a small discussion with Christophe on how to
fix it. One way is revert timer handling to create and destroy the source
during timer_cancel.

Frediano

> ---
>  server/tests/test-loop.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/server/tests/test-loop.c b/server/tests/test-loop.c
> index 1c2f496..0bcc953 100644
> --- a/server/tests/test-loop.c
> +++ b/server/tests/test-loop.c
> @@ -74,19 +74,31 @@ static void *loop_func(void *arg)
>      return NULL;
>  }
>  
> -static SpiceTimer *twice_timers[2] = { NULL, NULL };
> -static int twice_called = 0;
> -static void timer_not_twice(void *opaque)
> +static SpiceTimer *twice_timers_remove[2] = { NULL, NULL };
> +static int twice_remove_called = 0;
> +static void timer_not_twice_remove(void *opaque)
>  {
> -    spice_assert(++twice_called == 1);
> +    spice_assert(++twice_remove_called == 1);
>  
>      /* delete timers, should not have another call */
> -    core->timer_remove(twice_timers[0]);
> -    core->timer_remove(twice_timers[1]);
> -    twice_timers[0] = NULL;
> -    twice_timers[1] = NULL;
> +    core->timer_remove(twice_timers_remove[0]);
> +    core->timer_remove(twice_timers_remove[1]);
> +    twice_timers_remove[0] = NULL;
> +    twice_timers_remove[1] = NULL;
> +}
> +
> +static SpiceTimer *twice_timers_cancel[2] = { NULL, NULL };
> +static int twice_cancel_called = 0;
> +static void timer_not_twice(void *opaque)
> +{
> +    spice_assert(++twice_cancel_called == 1);
> +
> +    /* cancel timers, should not have another call */
> +    core->timer_cancel(twice_timers_cancel[0]);
> +    core->timer_cancel(twice_timers_cancel[1]);
>  }
>  
> +
>  int main(int argc, char **argv)
>  {
>      SpiceTimer *timer, *timers[10];
> @@ -121,13 +133,21 @@ int main(int argc, char **argv)
>      core->timer_start(timer, 10);
>  
>      /* test events are not called when freed */
> -    timer = twice_timers[0] = core->timer_add(timer_not_twice, NULL);
> +    timer = twice_timers_remove[0] = core->timer_add(timer_not_twice_remove,
> NULL);
>      spice_assert(timer != NULL);
>      core->timer_start(timer, 2);
> -    timer = twice_timers[1] = core->timer_add(timer_not_twice, NULL);
> +    timer = twice_timers_remove[1] = core->timer_add(timer_not_twice_remove,
> NULL);
>      spice_assert(timer != NULL);
>      core->timer_start(timer, 2);
>  
> +    /* test events are not called when cancelled */
> +    timer = timers[i++] = twice_timers_cancel[0] =
> core->timer_add(timer_not_twice, core);
> +    spice_assert(timer != NULL);
> +    core->timer_start(timer, 4);
> +    timer = timers[i++] = twice_timers_cancel[1] =
> core->timer_add(timer_not_twice, core);
> +    spice_assert(timer != NULL);
> +    core->timer_start(timer, 4);
> +
>      /* run the loop */
>      loop = g_main_loop_new(basic_event_loop_get_context(), FALSE);
>      alarm(1);