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

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

[Peter Maydell]

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;
-- 
2.43.0

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

[Fabiano Rosas]

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>