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

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

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 | 45 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH for-2.5 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 for-2.5 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 for-2.5 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 29205ed..4ab48e4 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -506,8 +506,19 @@ 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,
+#if !GLIB_CHECK_VERSION(2,36,0)
+    test_migrate_source_prepare,
+#endif
     test_migrate_source_check,
     NULL,
     NULL
-- 
2.5.0

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

[Michael S. Tsirkin]

On Fri, Nov 27, 2015 at 03:41:20PM +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>

But nothing prevents us from implementing it for all versions.


> ---
>  tests/vhost-user-test.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 29205ed..4ab48e4 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -506,8 +506,19 @@ 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,
> +#if !GLIB_CHECK_VERSION(2,36,0)
> +    test_migrate_source_prepare,
> +#endif
>      test_migrate_source_check,

So now for version 2.36 check will be used instead of prepare.
That's clearly wrong.

I'll switch this to named initializers to avoid the mess.

>      NULL,
>      NULL
> -- 
> 2.5.0

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

[Marc-André Lureau]

----- Original Message -----
> On Fri, Nov 27, 2015 at 03:41:20PM +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>
> 
> But nothing prevents us from implementing it for all versions.

Sure, it's just a kind of dead code for me that should be removed in future version. So I prefer to keep it under #ifdef.

> > ---
> >  tests/vhost-user-test.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> > index 29205ed..4ab48e4 100644
> > --- a/tests/vhost-user-test.c
> > +++ b/tests/vhost-user-test.c
> > @@ -506,8 +506,19 @@ 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,
> > +#if !GLIB_CHECK_VERSION(2,36,0)
> > +    test_migrate_source_prepare,
> > +#endif
> >      test_migrate_source_check,
> 
> So now for version 2.36 check will be used instead of prepare.
> That's clearly wrong.

instead?
> 
> I'll switch this to named initializers to avoid the mess.

I don't follow what is the mess here.

> 
> >      NULL,
> >      NULL
> > --
> > 2.5.0
> 

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

[Marc-André Lureau]

On Mon, Nov 30, 2015 at 12:00 PM, Marc-André Lureau <mlureau@redhat.com> wrote:
> I don't follow what is the mess here.

Ah ok, I get it, I forgot the #else NULL.

-- 
Marc-André Lureau

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

[Michael S. Tsirkin]

On Mon, Nov 30, 2015 at 06:00:29AM -0500, Marc-André Lureau wrote:
> 
> 
> ----- Original Message -----
> > On Fri, Nov 27, 2015 at 03:41:20PM +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>
> > 
> > But nothing prevents us from implementing it for all versions.
> 
> Sure, it's just a kind of dead code for me that should be removed in future version. So I prefer to keep it under #ifdef.
> 
> > > ---
> > >  tests/vhost-user-test.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> > > index 29205ed..4ab48e4 100644
> > > --- a/tests/vhost-user-test.c
> > > +++ b/tests/vhost-user-test.c
> > > @@ -506,8 +506,19 @@ 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,
> > > +#if !GLIB_CHECK_VERSION(2,36,0)
> > > +    test_migrate_source_prepare,
> > > +#endif
> > >      test_migrate_source_check,
> > 
> > So now for version 2.36 check will be used instead of prepare.
> > That's clearly wrong.
> 
> instead?
> > 
> > I'll switch this to named initializers to avoid the mess.
> 
> I don't follow what is the mess here.


You sent v2 that fixes it, and my patch fixes it too, so we don't need to argue.

> 
> > 
> > >      NULL,
> > >      NULL
> > > --
> > > 2.5.0
> > 

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

[Marc-André Lureau]

On Mon, Nov 30, 2015 at 2:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Nov 30, 2015 at 06:00:29AM -0500, Marc-André Lureau wrote:
>>
>>
>> ----- Original Message -----
>> > On Fri, Nov 27, 2015 at 03:41:20PM +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>
>> >
>> > But nothing prevents us from implementing it for all versions.
>>
>> Sure, it's just a kind of dead code for me that should be removed in future version. So I prefer to keep it under #ifdef.
>>
>> > > ---
>> > >  tests/vhost-user-test.c | 13 ++++++++++++-
>> > >  1 file changed, 12 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
>> > > index 29205ed..4ab48e4 100644
>> > > --- a/tests/vhost-user-test.c
>> > > +++ b/tests/vhost-user-test.c
>> > > @@ -506,8 +506,19 @@ 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,
>> > > +#if !GLIB_CHECK_VERSION(2,36,0)
>> > > +    test_migrate_source_prepare,
>> > > +#endif
>> > >      test_migrate_source_check,
>> >
>> > So now for version 2.36 check will be used instead of prepare.
>> > That's clearly wrong.
>>
>> instead?
>> >
>> > I'll switch this to named initializers to avoid the mess.
>>
>> I don't follow what is the mess here.
>
>
> You sent v2 that fixes it, and my patch fixes it too, so we don't need to argue.


I fixed it, you changed it because you don't agree with my will to
keep the version check to remove the dumb callback in the future. I
disagree, I think this is a right way to cleanup the code in the
future. Btw, Author/ Reported-by is wrong: it is the other way around.
You reported it to me, and I fixed it. Please reconsider my patch.

-- 
Marc-André Lureau

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

[Michael S. Tsirkin]

On Mon, Nov 30, 2015 at 02:12:54PM +0100, Marc-André Lureau wrote:
> On Mon, Nov 30, 2015 at 2:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Nov 30, 2015 at 06:00:29AM -0500, Marc-André Lureau wrote:
> >>
> >>
> >> ----- Original Message -----
> >> > On Fri, Nov 27, 2015 at 03:41:20PM +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>
> >> >
> >> > But nothing prevents us from implementing it for all versions.
> >>
> >> Sure, it's just a kind of dead code for me that should be removed in future version. So I prefer to keep it under #ifdef.
> >>
> >> > > ---
> >> > >  tests/vhost-user-test.c | 13 ++++++++++++-
> >> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> >> > > index 29205ed..4ab48e4 100644
> >> > > --- a/tests/vhost-user-test.c
> >> > > +++ b/tests/vhost-user-test.c
> >> > > @@ -506,8 +506,19 @@ 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,
> >> > > +#if !GLIB_CHECK_VERSION(2,36,0)
> >> > > +    test_migrate_source_prepare,
> >> > > +#endif
> >> > >      test_migrate_source_check,
> >> >
> >> > So now for version 2.36 check will be used instead of prepare.
> >> > That's clearly wrong.
> >>
> >> instead?
> >> >
> >> > I'll switch this to named initializers to avoid the mess.
> >>
> >> I don't follow what is the mess here.
> >
> >
> > You sent v2 that fixes it, and my patch fixes it too, so we don't need to argue.
> 
> 
> I fixed it, you changed it because you don't agree with my will to
> keep the version check to remove the dumb callback in the future. I
> disagree, I think this is a right way to cleanup the code in the
> future. Btw, Author/ Reported-by is wrong: it is the other way around.
> You reported it to me, and I fixed it.

I don't remember reporting it, but my patch didn't use your fix:
check list archives. Since I did not like v1,
I posted a patch before you posted v2.
That's why I did not list you as the author.

> Please reconsider my patch.

If you feel so strongly about it, I'll apply your patch
if you add a comment quoting the glib documentation.

> -- 
> Marc-André Lureau