Re: [PATCH] tests/qtest/vhost-user-test: Use g_timeout_add() to schedule connect

[Fabiano Rosas <farosas@suse.de>]

Peter Maydell <peter.maydell@linaro.org> writes:

> In vhost-user-test, we currently create a new g_thread to run the
> connect_thread() function.  This function sleeps for 1 second, and
> then calls test_server_create_chr() to create and configure a
> chardev:
>
>     chr = qemu_chr_new(server->chr_name, chr_path, server->context);
>     g_assert(chr);
>
>     qemu_chr_fe_init(&server->chr, chr, &error_abort);
>     qemu_chr_fe_set_handlers(&server->chr, chr_can_read, chr_read,
>                              chr_event, NULL, server, server->context, true);
>
> This has a race condition, because when we set the
> 'reconnect-ms=1000' option on the chardev the socket chardev's
> implementation handles the connect asynchronously, via a background
> thread and a callback invoked in the main-loop thread.  This means
> that that callback and the test_server_create_chr() call to
> qemu_chr_fe_set_handlers() can both enter the char-socket code
> simultaneously.  The result is random assertion failures and memory
> leaks reported by the clang address-sanitizer.
>
> Fix this by using g_timeout_source_new() to set up a GSource that
> will run test_server_connect() on the main-loop thread.  This ensures
> it can't execute in parallel with the callback that the socket
> chardev sets up.  This is similar to how we already handle the
> reconnect_cb() in test_reconnect().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Having finally tracked down the root cause of the leak reports in
> the vhost-user-tests, this seems to me a fairly straightforward
> way to deal with it.
>
>  tests/qtest/vhost-user-test.c | 36 ++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index d2bb21d5b9..c8b5f8ff71 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -930,26 +930,42 @@ reconnect_cb(gpointer user_data)
>      return FALSE;
>  }
>  
> -static gpointer
> -connect_thread(gpointer data)
> +static gboolean connect_cb(gpointer user_data)
>  {
> -    TestServer *s = data;
> +    TestServer *s = user_data;
>  
> -    /* wait for qemu to start before first try, to avoid extra warnings */
> -    g_usleep(G_USEC_PER_SEC);
>      test_server_connect(s);
>  
> -    return NULL;
> +    /* We only need to be called once */
> +    return G_SOURCE_REMOVE;
> +}
> +
> +/* Initial delay before connect, in milliseconds (1 second) */
> +#define INITIAL_CONNECT_DELAY_MS (1 * 1000)
> +
> +static void test_schedule_connect(TestServer *s)
> +{
> +    /*
> +     * Wait for a bit for QEMU to start before we first try to connect,
> +     * to avoid extra warnings. We must run the "connect" on the
> +     * main-loop thread so it doesn't race with a callback that
> +     * the socket-chardev sets up on the main-loop.
> +     */
> +    GSource *src = g_timeout_source_new(INITIAL_CONNECT_DELAY_MS);
> +    g_source_set_callback(src, connect_cb, s, NULL);
> +    g_source_attach(src, s->context);
> +    g_source_unref(src);
>  }
>  
>  static void *vhost_user_test_setup_reconnect(GString *cmd_line, void *arg)
>  {
>      TestServer *s = test_server_new("reconnect", arg);
>  
> -    g_thread_unref(g_thread_new("connect", connect_thread, s));
>      append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
>      s->vu_ops->append_opts(s, cmd_line, ",server=on");
>  
> +    test_schedule_connect(s);
> +
>      g_test_queue_destroy(vhost_user_test_cleanup, s);
>  
>      return s;
> @@ -983,10 +999,11 @@ static void *vhost_user_test_setup_connect_fail(GString *cmd_line, void *arg)
>  
>      s->test_fail = true;
>  
> -    g_thread_unref(g_thread_new("connect", connect_thread, s));
>      append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
>      s->vu_ops->append_opts(s, cmd_line, ",server=on");
>  
> +    test_schedule_connect(s);
> +
>      g_test_queue_destroy(vhost_user_test_cleanup, s);
>  
>      return s;
> @@ -998,10 +1015,11 @@ static void *vhost_user_test_setup_flags_mismatch(GString *cmd_line, void *arg)
>  
>      s->test_flags = TEST_FLAGS_DISCONNECT;
>  
> -    g_thread_unref(g_thread_new("connect", connect_thread, s));
>      append_mem_opts(s, cmd_line, 256, TEST_MEMFD_AUTO);
>      s->vu_ops->append_opts(s, cmd_line, ",server=on");
>  
> +    test_schedule_connect(s);
> +
>      g_test_queue_destroy(vhost_user_test_cleanup, s);
>  
>      return s;

Thanks!

Reviewed-by: Fabiano Rosas <farosas@suse.de>