[Spice-devel] [PATCH spice-server v3 07/12] test-sasl: Add tests for different mechanism names

[Frediano Ziglio]

> 
> On Fri, Dec 22, 2017 at 10:07:08AM +0000, Frediano Ziglio wrote:
> > @@ -395,16 +427,32 @@ idle_next_test(void *arg)
> >  {
> >      // end previous test
> >      if (test_num >= 0) {
> > -        g_assert(encode_called);
> > +        const TestData *data = &tests_data[test_num];
> > +        if (data->success) {
> > +            g_assert(encode_called);
> > +        } else {
> > +            g_assert(mechlist_called);
> > +            g_assert(!encode_called);
> > +        }
> >          reset_test();
> > -        basic_event_loop_quit();
> > -        return FALSE;
> >      }
> >  
> >      // start next test
> >      ++test_num;
> >      alarm(4);
> >  
> > +    if (test_num >= G_N_ELEMENTS(tests_data)) {
> > +        basic_event_loop_quit();
> > +        return FALSE;
> > +    }
> > +
> > +    TestData *data = &tests_data[test_num];
> > +    if (data->mechlen < 0) {
> > +        data->mechlen = strlen(data->mechname);
> > +    }
> > +    int len = data->mechlen;
> > +    printf("\nRunning test %d ('%*.*s' %d)\n", test_num, len, len,
> > data->mechname, len);
> > +
> >      int sv[2];
> >      g_assert_cmpint(socketpair(AF_LOCAL, SOCK_STREAM, 0, sv), ==, 0);
> 
> I haven't read over the whole tests yet, but I find the way you iterate
> over the tests through idle_next_test() quite hard to read. Adding the
> changes below on top of this series make things easier to follow in my
> opinion.
> 
> diff --git a/server/tests/basic-event-loop.c
> b/server/tests/basic-event-loop.c
> index 6fd8a2934..12205ecef 100644
> --- a/server/tests/basic-event-loop.c
> +++ b/server/tests/basic-event-loop.c
> @@ -130,7 +130,7 @@ SpiceCoreInterface *basic_event_loop_init(void)
>  {
>      ignore_sigpipe();
>      spice_assert(main_context == NULL);
> -    main_context = g_main_context_new();
> +    main_context = g_main_context_default();
>      base_core_interface = event_loop_core;
>      base_core_interface.main_context = main_context;
>  

I would strongly avoid this. Qemu can use different contexts and using the default
can lead tests works while spice-server won't work on real world cases.
I would retain my idle_add function to deal with this. Rest of proposal make sense,
I'll merge it.
Would be also better to put some comments why not using the default context for
future reference.

> diff --git a/server/tests/test-sasl.c b/server/tests/test-sasl.c
> index 90f9dc2e6..02317e038 100644
> --- a/server/tests/test-sasl.c
> +++ b/server/tests/test-sasl.c
> @@ -64,9 +64,9 @@ static unsigned test_flags;
>  static SpiceCoreInterface *core;
>  static SpiceServer *server;
>  
> -static int test_num = -1;
> +static unsigned int test_num;
>  
> -static gboolean idle_next_test(void *arg);
> +static gboolean idle_end_test(void *arg);
>  
>  static void
>  check_sasl_conn(sasl_conn_t *conn)
> @@ -298,7 +298,7 @@ start_test(void)
>  }
>  
>  static void
> -end_test(void)
> +end_tests(void)
>  {
>      spice_server_destroy(server);
>      server = NULL;
> @@ -363,15 +363,6 @@ write_u32_err(int fd, uint32_t val)
>  #define write_u32(fd, val) \
>      g_assert_cmpint(write_u32_err(fd, val), ==, sizeof(uint32_t))
>  
> -static void
> -idle_add(GSourceFunc func, void *arg)
> -{
> -    GSource *source = g_idle_source_new();
> -    g_source_set_callback(source, func, NULL, NULL);
> -    g_source_attach(source, basic_event_loop_get_context());
> -    g_source_unref(source);
> -}
> -
>  typedef enum {
>      STEP_NONE,
>      STEP_READ_MECHLIST_LEN,
> @@ -525,36 +516,13 @@ client_emulator(void *arg)
>      shutdown(sock, SHUT_RDWR);
>      close(sock);
>  
> -    idle_add(idle_next_test, NULL);
> +    g_idle_add(idle_end_test, NULL);
>  
>      return NULL;
>  }
>  
> -// called when a new test has to be run
> -static gboolean
> -idle_next_test(void *arg)
> +static GThread *setup_thread(unsigned int test_num)
>  {
> -    // end previous test
> -    if (test_num >= 0) {
> -        const TestData *data = &tests_data[test_num];
> -        if (data->success) {
> -            g_assert(encode_called);
> -        } else {
> -            g_assert(mechlist_called);
> -            g_assert(!encode_called);
> -        }
> -        reset_test();
> -    }
> -
> -    // start next test
> -    ++test_num;
> -    alarm(4);
> -
> -    if (test_num >= G_N_ELEMENTS(tests_data)) {
> -        basic_event_loop_quit();
> -        return FALSE;
> -    }
> -
>      TestData *data = &tests_data[test_num];
>      if (data->mechlen < 0) {
>          data->mechlen = strlen(data->mechname);
> @@ -568,11 +536,29 @@ idle_next_test(void *arg)
>  
>      g_assert(spice_server_add_client(server, sv[0], 0) == 0);
>  
> -    pthread_t thread;
> -    g_assert_cmpint(pthread_create(&thread, NULL, client_emulator,
> GINT_TO_POINTER(sv[1])), ==, 0);
> -    g_assert_cmpint(pthread_detach(thread), ==, 0);
> +    GThread *thread = g_thread_new("sasl-thread", client_emulator,
> GINT_TO_POINTER(sv[1]));
> +    return thread;
> +}
>  
> -    return FALSE;
> +// called when the next test has to be run
> +static gboolean
> +idle_end_test(void *arg)
> +{
> +    basic_event_loop_quit();
> +
> +    return G_SOURCE_REMOVE;
> +}
> +
> +static void
> +check_test_results(unsigned int test_num)
> +{
> +    const TestData *data = &tests_data[test_num];
> +    if (data->success) {
> +        g_assert(encode_called);
> +    } else {
> +        g_assert(mechlist_called);
> +        g_assert(!encode_called);
> +    }
>  }
>  
>  static void
> @@ -582,11 +568,17 @@ sasl_mechs(void)
>  
>      memset(long_mechname, 'X', sizeof(long_mechname));
>  
> -    idle_add(idle_next_test, NULL);
> -    alarm(4);
> -    basic_event_loop_mainloop();
> +    for (test_num = 0; test_num < G_N_ELEMENTS(tests_data); test_num++) {
> +        GThread *thread;
> +        thread = setup_thread(test_num);
> +        alarm(4);
> +        basic_event_loop_mainloop();
> +        g_thread_join(thread);
> +        check_test_results(test_num);
> +        reset_test();
> +    }
>  
> -    end_test();
> +    end_tests();
>  }
>  
>  int
> 

Frediano