[Qemu-devel] [PATCH v2 0/3] vhost-user-test fixes

[Qemu-devel] [PATCH v2 0/3] vhost-user-test fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This series fixes a number of races and crashes on glib < 2.36
(with Travis build for ex).

v1->v2:
- fix the last patch to not end up with check() callback instead of
  prepare(): use named initializer.

Marc-André Lureau (3):
  vhost-user-test: fix chardriver race
  vhost-user-test: use unix port for migration
  vhost-user-test: fix crash with glib < 2.36

 tests/vhost-user-test.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 9 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 1/3] vhost-user-test: fix chardriver race

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost-user-tests uses a helper thread to dispatch the vhost-user servers
sources. However the CharDriverState is not thread-safe. Therefore, when
it's given to the thread, it shouldn't be manipulated concurrently.

We dispatch cleaning the server in an idle source. By the end of the
test, we ensure not to leave anything behind by joining the thread and
finishing the sources dispatch.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/vhost-user-test.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index e4c36af..261f4b7 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -216,8 +216,7 @@ static void read_guest_mem(TestServer *s)
 
 static void *thread_function(void *data)
 {
-    GMainLoop *loop;
-    loop = g_main_loop_new(NULL, FALSE);
+    GMainLoop *loop = data;
     g_main_loop_run(loop);
     return NULL;
 }
@@ -389,7 +388,7 @@ static TestServer *test_server_new(const gchar *name)
     g_strdup_printf(QEMU_CMD extra, (mem), (mem), (root), (s)->chr_name,       \
                     (s)->socket_path, (s)->chr_name, ##__VA_ARGS__)
 
-static void test_server_free(TestServer *server)
+static gboolean _test_server_free(TestServer *server)
 {
     int i;
 
@@ -406,9 +405,15 @@ static void test_server_free(TestServer *server)
     unlink(server->socket_path);
     g_free(server->socket_path);
 
-
     g_free(server->chr_name);
     g_free(server);
+
+    return FALSE;
+}
+
+static void test_server_free(TestServer *server)
+{
+    g_idle_add((GSourceFunc)_test_server_free, server);
 }
 
 static void wait_for_log_fd(TestServer *s)
@@ -590,6 +595,8 @@ int main(int argc, char **argv)
     char *qemu_cmd = NULL;
     int ret;
     char template[] = "/tmp/vhost-test-XXXXXX";
+    GMainLoop *loop;
+    GThread *thread;
 
     g_test_init(&argc, &argv, NULL);
 
@@ -612,8 +619,9 @@ int main(int argc, char **argv)
 
     server = test_server_new("test");
 
+    loop = g_main_loop_new(NULL, FALSE);
     /* run the main loop thread so the chardev may operate */
-    g_thread_new(NULL, thread_function, NULL);
+    thread = g_thread_new(NULL, thread_function, loop);
 
     qemu_cmd = GET_QEMU_CMD(server);
 
@@ -632,6 +640,14 @@ int main(int argc, char **argv)
     /* cleanup */
     test_server_free(server);
 
+    /* finish the helper thread and dispatch pending sources */
+    g_main_loop_quit(loop);
+    g_thread_join(thread);
+    while (g_main_context_pending(NULL)) {
+        g_main_context_iteration (NULL, TRUE);
+    }
+    g_main_loop_unref(loop);
+
     ret = rmdir(tmpfs);
     if (ret != 0) {
         g_test_message("unable to rmdir: path (%s): %s\n",
-- 
2.5.0

[Qemu-devel] [PATCH v2 2/3] vhost-user-test: use unix port for migration

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

TCP port 1234 may be used by another process concurrently. Instead use a
temporary unix socket.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/vhost-user-test.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 261f4b7..29205ed 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -123,6 +123,7 @@ static VhostUserMsg m __attribute__ ((unused));
 
 typedef struct TestServer {
     gchar *socket_path;
+    gchar *mig_path;
     gchar *chr_name;
     CharDriverState *chr;
     int fds_num;
@@ -364,6 +365,7 @@ static TestServer *test_server_new(const gchar *name)
     gchar *chr_path;
 
     server->socket_path = g_strdup_printf("%s/%s.sock", tmpfs, name);
+    server->mig_path = g_strdup_printf("%s/%s.mig", tmpfs, name);
 
     chr_path = g_strdup_printf("unix:%s,server,nowait", server->socket_path);
     server->chr_name = g_strdup_printf("chr-%s", name);
@@ -405,6 +407,9 @@ static gboolean _test_server_free(TestServer *server)
     unlink(server->socket_path);
     g_free(server->socket_path);
 
+    unlink(server->mig_path);
+    g_free(server->mig_path);
+
     g_free(server->chr_name);
     g_free(server);
 
@@ -512,7 +517,7 @@ static void test_migrate(void)
 {
     TestServer *s = test_server_new("src");
     TestServer *dest = test_server_new("dest");
-    const char *uri = "tcp:127.0.0.1:1234";
+    char *uri = g_strdup_printf("%s%s", "unix:", dest->mig_path);
     QTestState *global = global_qtest, *from, *to;
     GSource *source;
     gchar *cmd;
@@ -583,6 +588,7 @@ static void test_migrate(void)
     test_server_free(dest);
     qtest_quit(from);
     test_server_free(s);
+    g_free(uri);
 
     global_qtest = global;
 }
-- 
2.5.0

[Qemu-devel] [PATCH v2 3/3] vhost-user-test: fix crash with glib < 2.36

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The prepare callback needs to be implemented with glib < 2.36.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/vhost-user-test.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 29205ed..27dedeb 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -506,11 +506,20 @@ test_migrate_source_check(GSource *source)
     return FALSE;
 }
 
+#if !GLIB_CHECK_VERSION(2,36,0)
+static gboolean
+test_migrate_source_prepare(GSource *source, gint *timeout)
+{
+    *timeout = -1;
+    return FALSE;
+}
+#endif
+
 GSourceFuncs test_migrate_source_funcs = {
-    NULL,
-    test_migrate_source_check,
-    NULL,
-    NULL
+#if !GLIB_CHECK_VERSION(2,36,0)
+    .prepare = test_migrate_source_prepare,
+#endif
+    .check = test_migrate_source_check,
 };
 
 static void test_migrate(void)
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 3/3] vhost-user-test: fix crash with glib < 2.36

[Michael S. Tsirkin]

On Mon, Nov 30, 2015 at 12:17:00PM +0100, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The prepare callback needs to be implemented with glib < 2.36.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/vhost-user-test.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 29205ed..27dedeb 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -506,11 +506,20 @@ test_migrate_source_check(GSource *source)
>      return FALSE;
>  }
>  
> +#if !GLIB_CHECK_VERSION(2,36,0)
> +static gboolean
> +test_migrate_source_prepare(GSource *source, gint *timeout)
> +{
> +    *timeout = -1;
> +    return FALSE;
> +}
> +#endif
> +
>  GSourceFuncs test_migrate_source_funcs = {
> -    NULL,
> -    test_migrate_source_check,
> -    NULL,
> -    NULL
> +#if !GLIB_CHECK_VERSION(2,36,0)
> +    .prepare = test_migrate_source_prepare,
> +#endif
> +    .check = test_migrate_source_check,
>  };
>  
>  static void test_migrate(void)

I don't see why do we need the ifdefs, we can use the
same code for all versions.
I queued a patch that does exactly that.

> -- 
> 2.5.0

Re: [Qemu-devel] [PATCH v2 3/3] vhost-user-test: fix crash with glib < 2.36

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Nov 30, 2015 at 12:17:00PM +0100, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> 
>> The prepare callback needs to be implemented with glib < 2.36.
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  tests/vhost-user-test.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
>> index 29205ed..27dedeb 100644
>> --- a/tests/vhost-user-test.c
>> +++ b/tests/vhost-user-test.c
>> @@ -506,11 +506,20 @@ test_migrate_source_check(GSource *source)
>>      return FALSE;
>>  }
>>  
>> +#if !GLIB_CHECK_VERSION(2,36,0)
>> +static gboolean
>> +test_migrate_source_prepare(GSource *source, gint *timeout)
>> +{
>> +    *timeout = -1;
>> +    return FALSE;
>> +}
>> +#endif
>> +
>>  GSourceFuncs test_migrate_source_funcs = {
>> -    NULL,
>> -    test_migrate_source_check,
>> -    NULL,
>> -    NULL
>> +#if !GLIB_CHECK_VERSION(2,36,0)
>> +    .prepare = test_migrate_source_prepare,
>> +#endif
>> +    .check = test_migrate_source_check,
>>  };
>>  
>>  static void test_migrate(void)
>
> I don't see why do we need the ifdefs, we can use the
> same code for all versions.
> I queued a patch that does exactly that.

The ifdefs serve as a marker that lets us drop unnecessary code when our
required version of GLib reaches 2.36.  A comment might do, too, but it
should probably contain GLIB_CHECK_VERSION() to be visible in grep.