[Qemu-devel] [PATCH for-2.5 v4 0/4] vhost-user-test fixes

[Qemu-devel] [PATCH for-2.5 v4 0/4] 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).

v3->v4:
- add patch to include vhost-user-test when target list lacks 386.
- add a code comment about <2.36 prepare callback.

v2->v3:
- quote glib documentation in commit message of last patch.

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

Marc-André Lureau (4):
  vhost-user-test: fix chardriver race
  vhost-user-test: use unix port for migration
  vhost-user-test: fix crash with glib < 2.36
  tests: add vhost-user-test when target is x64 only

 tests/Makefile          |  6 +++---
 tests/vhost-user-test.c | 51 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 45 insertions(+), 12 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH for-2.5 v4 1/4] 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

Re: [Qemu-devel] [PATCH for-2.5 v4 1/4] vhost-user-test: fix chardriver race

[Alex Bennée]

marcandre.lureau@redhat.com writes:

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

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  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",


--
Alex Bennée

[Qemu-devel] [PATCH for-2.5 v4 2/4] 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

Re: [Qemu-devel] [PATCH for-2.5 v4 2/4] vhost-user-test: use unix port for migration

[Alex Bennée]

marcandre.lureau@redhat.com writes:

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

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

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


--
Alex Bennée

[Qemu-devel] [PATCH for-2.5 v4 3/4] 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,
quoting glib documentation:
"Since 2.36 this may be NULL, in which case the effect is as if the
function always returns FALSE with a timeout of -1."

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

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 29205ed..29de739 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -506,11 +506,22 @@ test_migrate_source_check(GSource *source)
     return FALSE;
 }
 
+#if !GLIB_CHECK_VERSION(2,36,0)
+/* this callback is unnecessary with glib >2.36, the default
+ * prepare for the source does the same */
+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 for-2.5 v4 3/4] vhost-user-test: fix crash with glib < 2.36

[Alex Bennée]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The prepare callback needs to be implemented with glib < 2.36,
> quoting glib documentation:
> "Since 2.36 this may be NULL, in which case the effect is as if the
> function always returns FALSE with a timeout of -1."
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  tests/vhost-user-test.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 29205ed..29de739 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -506,11 +506,22 @@ test_migrate_source_check(GSource *source)
>      return FALSE;
>  }
>
> +#if !GLIB_CHECK_VERSION(2,36,0)
> +/* this callback is unnecessary with glib >2.36, the default
> + * prepare for the source does the same */
> +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)


--
Alex Bennée

[Qemu-devel] [PATCH for-2.5 v4 4/4] tests: add vhost-user-test when target is x64 only

[marcandre.lureau]

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

check-qtest-x86_64-y is overwritten by the last assignment. Move
vhost-user-test addition after.

Reported-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 0ef00a1..cf1228b 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -198,13 +198,13 @@ check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
 check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
-ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
-check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
-endif
 check-qtest-i386-y += tests/test-netfilter$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
+ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
+check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
+endif
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
-- 
2.5.0

Re: [Qemu-devel] [PATCH for-2.5 v4 4/4] tests: add vhost-user-test when target is x64 only

[Alex Bennée]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> check-qtest-x86_64-y is overwritten by the last assignment. Move
> vhost-user-test addition after.
>
> Reported-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/Makefile | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 0ef00a1..cf1228b 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -198,13 +198,13 @@ check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
>  check-qtest-i386-y += tests/q35-test$(EXESUF)
>  gcov-files-i386-y += hw/pci-host/q35.c
>  check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
> -ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
> -check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
> -endif
>  check-qtest-i386-y += tests/test-netfilter$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> +ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
> +check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
> +endif

So is this limitation due to the guest binary we need to test this?
Surely any KVM enabled build with vhost support should be able to run
the test?

>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)


--
Alex Bennée

Re: [Qemu-devel] [PATCH for-2.5 v4 4/4] tests: add vhost-user-test when target is x64 only

[Marc-André Lureau]

Hi

----- Original Message -----
> > diff --git a/tests/Makefile b/tests/Makefile
> > index 0ef00a1..cf1228b 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -198,13 +198,13 @@ check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
> >  check-qtest-i386-y += tests/q35-test$(EXESUF)
> >  gcov-files-i386-y += hw/pci-host/q35.c
> >  check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) +=
> >  tests/vhost-user-test$(EXESUF)
> > -ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
> > -check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) +=
> > tests/vhost-user-test$(EXESUF)
> > -endif
> >  check-qtest-i386-y += tests/test-netfilter$(EXESUF)
> >  check-qtest-x86_64-y = $(check-qtest-i386-y)
> >  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> >  gcov-files-x86_64-y = $(subst
> >  i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> > +ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
> > +check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) +=
> > tests/vhost-user-test$(EXESUF)
> > +endif
> 
> So is this limitation due to the guest binary we need to test this?
> Surely any KVM enabled build with vhost support should be able to run
> the test?

Any that has CONFIG_VHOST_NET_TEST_$target_name set in configure. It looks like we are
missing other than x86 hosts support in the Makefile, but I don't have other than
x86 kvm-enabled hosts to test it. I suppose this is better to be addresses after 2.5.