[Qemu-devel] [PATCH V2 0/2] tests: Fix possible deadlock in qtest initialization

[Qemu-devel] [PATCH V2 0/2] tests: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

V1 -> V2:
    Addressed Stefan Hajnoczi's review:
    - Dropped qtest_state_valid API
    - Moved socket options in socket_accept. 

'socket_accept' waits for Qemu to init its unix socket.
If Qemu encounters an error during command line parsing,
it can exit before initializing the communication channel.

Using a timeout for sockets fixes the issue, but is not
enough. We need to know which test from check-qtest-*
fails.

We tweak the Makefile to produce per-test output.
The effect is that the output will grow, but we will know
which qtest failed. This helps when an asert is made on
other code and not in the test itself.


Marcel Apfelbaum (1):
  tests/libqtest: Fix possible deadlock in qtest initialization

Stefan Hajnoczi (1):
  tests: Tweak the Makefile to produce per-test output.

 tests/Makefile   | 2 +-
 tests/libqtest.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V2 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

'socket_accept' waits for Qemu to init its unix socket.
If Qemu encounters an error during command line parsing,
it can exit before initializing the communication channel.

Using a timeout for sockets fixes the issue.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 tests/libqtest.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index f587d36..f1ba254 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -34,6 +34,7 @@
 #include "qapi/qmp/json-parser.h"
 
 #define MAX_IRQ 256
+#define SOCKET_TIMEOUT 5
 
 QTestState *global_qtest;
 
@@ -78,12 +79,16 @@ static int socket_accept(int sock)
     struct sockaddr_un addr;
     socklen_t addrlen;
     int ret;
+    struct timeval timeout = { .tv_sec = SOCKET_TIMEOUT,
+                               .tv_usec = 0 };
+
+    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&timeout,
+               sizeof(timeout));
 
     addrlen = sizeof(addr);
     do {
         ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
     } while (ret == -1 && errno == EINTR);
-    g_assert_no_errno(ret);
     close(sock);
 
     return ret;
@@ -91,7 +96,7 @@ static int socket_accept(int sock)
 
 static void kill_qemu(QTestState *s)
 {
-    if (s->qemu_pid != -1) {
+    if (s && s->qemu_pid != -1) {
         kill(s->qemu_pid, SIGTERM);
         waitpid(s->qemu_pid, NULL, 0);
     }
@@ -153,6 +158,8 @@ QTestState *qtest_init(const char *extra_args)
     g_free(socket_path);
     g_free(qmp_socket_path);
 
+    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
+
     s->rx = g_string_new("");
     for (i = 0; i < MAX_IRQ; i++) {
         s->irq_level[i] = false;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V2 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Stefan Hajnoczi]

On Tue, Mar 11, 2014 at 12:09:09PM +0200, Marcel Apfelbaum wrote:
> @@ -78,12 +79,16 @@ static int socket_accept(int sock)
>      struct sockaddr_un addr;
>      socklen_t addrlen;
>      int ret;
> +    struct timeval timeout = { .tv_sec = SOCKET_TIMEOUT,
> +                               .tv_usec = 0 };
> +
> +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&timeout,
> +               sizeof(timeout));
>  
>      addrlen = sizeof(addr);
>      do {
>          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
>      } while (ret == -1 && errno == EINTR);
> -    g_assert_no_errno(ret);
>      close(sock);

Did you mean to leave SO_RCVTIMEO set after this function completes?

> @@ -91,7 +96,7 @@ static int socket_accept(int sock)
>  
>  static void kill_qemu(QTestState *s)
>  {
> -    if (s->qemu_pid != -1) {
> +    if (s && s->qemu_pid != -1) {
>          kill(s->qemu_pid, SIGTERM);
>          waitpid(s->qemu_pid, NULL, 0);
>      }

This is a bug in libqtest.c, please don't silence the crash.

kill_qemu() gets called from the SIGABRT signal handler but I forgot
that global_qtest isn't initialized yet while qtest_init() executes.

In other words, the cleanup is broken if we fail inside qtest_init().
Can you drop this hunk and I'll send a patch to fix the underlying
issue?

> @@ -153,6 +158,8 @@ QTestState *qtest_init(const char *extra_args)
>      g_free(socket_path);
>      g_free(qmp_socket_path);
>  
> +    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
> +

We probably shouldn't socket_accept() s->qmp_fd if s->fd already failed.
Otherwise we'll wait another 5 seconds for the timeout to explire:

    s->fd = socket_accept(sock);
    if (s->fd >= 0) {
        s->qmp_fd = socket_accept(qmpsock);
    }

Re: [Qemu-devel] [PATCH V2 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

On Tue, 2014-03-11 at 13:40 +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 11, 2014 at 12:09:09PM +0200, Marcel Apfelbaum wrote:
> > @@ -78,12 +79,16 @@ static int socket_accept(int sock)
> >      struct sockaddr_un addr;
> >      socklen_t addrlen;
> >      int ret;
> > +    struct timeval timeout = { .tv_sec = SOCKET_TIMEOUT,
> > +                               .tv_usec = 0 };
> > +
> > +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&timeout,
> > +               sizeof(timeout));
> >  
> >      addrlen = sizeof(addr);
> >      do {
> >          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
> >      } while (ret == -1 && errno == EINTR);
> > -    g_assert_no_errno(ret);
> >      close(sock);
> 
> Did you mean to leave SO_RCVTIMEO set after this function completes?
Yes, I don't think it hurts. A 5 sec timeout should be like infinite,
Qemu running on the same machine. If you think 

> 
> > @@ -91,7 +96,7 @@ static int socket_accept(int sock)
> >  
> >  static void kill_qemu(QTestState *s)
> >  {
> > -    if (s->qemu_pid != -1) {
> > +    if (s && s->qemu_pid != -1) {
> >          kill(s->qemu_pid, SIGTERM);
> >          waitpid(s->qemu_pid, NULL, 0);
> >      }
> 
> This is a bug in libqtest.c, please don't silence the crash.
I didn't see it like hiding a crash, I thought that if there
is any problem during init it is because the Qemu failed to start,
meaning that you don't have a process to kill (Qemu exited already).
Al of the above happens -> you don't have a global state.

Anyway, if you have a better way to deal with it, I have nothing against it :)

Thanks,
Marcel

> 
> kill_qemu() gets called from the SIGABRT signal handler but I forgot
> that global_qtest isn't initialized yet while qtest_init() executes.
> 
> In other words, the cleanup is broken if we fail inside qtest_init().
> Can you drop this hunk and I'll send a patch to fix the underlying
> issue?
> 
> > @@ -153,6 +158,8 @@ QTestState *qtest_init(const char *extra_args)
> >      g_free(socket_path);
> >      g_free(qmp_socket_path);
> >  
> > +    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
> > +
> 
> We probably shouldn't socket_accept() s->qmp_fd if s->fd already failed.
> Otherwise we'll wait another 5 seconds for the timeout to explire:
Yes, I already had this chunk, I have no idea why I dropped it, I'll
return it, thanks.

Thanks,
Marcel
> 
>     s->fd = socket_accept(sock);
>     if (s->fd >= 0) {
>         s->qmp_fd = socket_accept(qmpsock);
>     }

Re: [Qemu-devel] [PATCH V2 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Marcel Apfelbaum]

On Tue, 2014-03-11 at 14:51 +0200, Marcel Apfelbaum wrote:
> On Tue, 2014-03-11 at 13:40 +0100, Stefan Hajnoczi wrote:
> > On Tue, Mar 11, 2014 at 12:09:09PM +0200, Marcel Apfelbaum wrote:
> > > @@ -78,12 +79,16 @@ static int socket_accept(int sock)
> > >      struct sockaddr_un addr;
> > >      socklen_t addrlen;
> > >      int ret;
> > > +    struct timeval timeout = { .tv_sec = SOCKET_TIMEOUT,
> > > +                               .tv_usec = 0 };
> > > +
> > > +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&timeout,
> > > +               sizeof(timeout));
> > >  
> > >      addrlen = sizeof(addr);
> > >      do {
> > >          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
> > >      } while (ret == -1 && errno == EINTR);
> > > -    g_assert_no_errno(ret);
> > >      close(sock);
> > 
> > Did you mean to leave SO_RCVTIMEO set after this function completes?
> Yes, I don't think it hurts. A 5 sec timeout should be like infinite,
> Qemu running on the same machine. If you think 
... otherwise, I can remove the timeout, but I think it is OK.

> 
> > 
> > > @@ -91,7 +96,7 @@ static int socket_accept(int sock)
> > >  
> > >  static void kill_qemu(QTestState *s)
> > >  {
> > > -    if (s->qemu_pid != -1) {
> > > +    if (s && s->qemu_pid != -1) {
> > >          kill(s->qemu_pid, SIGTERM);
> > >          waitpid(s->qemu_pid, NULL, 0);
> > >      }
> > 
> > This is a bug in libqtest.c, please don't silence the crash.
> I didn't see it like hiding a crash, I thought that if there
> is any problem during init it is because the Qemu failed to start,
> meaning that you don't have a process to kill (Qemu exited already).
> Al of the above happens -> you don't have a global state.
> 
> Anyway, if you have a better way to deal with it, I have nothing against it :)
> 
> Thanks,
> Marcel
> 
> > 
> > kill_qemu() gets called from the SIGABRT signal handler but I forgot
> > that global_qtest isn't initialized yet while qtest_init() executes.
> > 
> > In other words, the cleanup is broken if we fail inside qtest_init().
> > Can you drop this hunk and I'll send a patch to fix the underlying
> > issue?
I dropped it, please take care of it as it gets a segmentation fault
if we abort in qtest_init.

Thanks,
Marcel
> > 
> > > @@ -153,6 +158,8 @@ QTestState *qtest_init(const char *extra_args)
> > >      g_free(socket_path);
> > >      g_free(qmp_socket_path);
> > >  
> > > +    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
> > > +
> > 
> > We probably shouldn't socket_accept() s->qmp_fd if s->fd already failed.
> > Otherwise we'll wait another 5 seconds for the timeout to explire:
> Yes, I already had this chunk, I have no idea why I dropped it, I'll
> return it, thanks.
> 
> Thanks,
> Marcel
> > 
> >     s->fd = socket_accept(sock);
> >     if (s->fd >= 0) {
> >         s->qmp_fd = socket_accept(qmpsock);
> >     }
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH V2 1/2] tests/libqtest: Fix possible deadlock in qtest initialization

[Stefan Hajnoczi]

On Tue, Mar 11, 2014 at 03:04:22PM +0200, Marcel Apfelbaum wrote:
> On Tue, 2014-03-11 at 14:51 +0200, Marcel Apfelbaum wrote:
> > On Tue, 2014-03-11 at 13:40 +0100, Stefan Hajnoczi wrote:
> > > On Tue, Mar 11, 2014 at 12:09:09PM +0200, Marcel Apfelbaum wrote:
> > > > @@ -78,12 +79,16 @@ static int socket_accept(int sock)
> > > >      struct sockaddr_un addr;
> > > >      socklen_t addrlen;
> > > >      int ret;
> > > > +    struct timeval timeout = { .tv_sec = SOCKET_TIMEOUT,
> > > > +                               .tv_usec = 0 };
> > > > +
> > > > +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&timeout,
> > > > +               sizeof(timeout));
> > > >  
> > > >      addrlen = sizeof(addr);
> > > >      do {
> > > >          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
> > > >      } while (ret == -1 && errno == EINTR);
> > > > -    g_assert_no_errno(ret);
> > > >      close(sock);
> > > 
> > > Did you mean to leave SO_RCVTIMEO set after this function completes?
> > Yes, I don't think it hurts. A 5 sec timeout should be like infinite,
> > Qemu running on the same machine. If you think 
> ... otherwise, I can remove the timeout, but I think it is OK.

I think you are right.  I checked that the qtest protocol has no
long-running operations.  It doesn't seem realistic that any qtest
command would take 5 seconds or longer.

So let's leave in the timeout.

Stefan

[Qemu-devel] [PATCH V2 2/2] tests: Tweak the Makefile to produce per-test output.

[Marcel Apfelbaum]

From: Stefan Hajnoczi <stefanha@redhat.com>

The effect is that the output will grow, but we will know
which qtest failed. This helps when an asert is made on
other code and not in the test itself.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 tests/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile b/tests/Makefile
index b17d41e..a8405c8 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -273,7 +273,7 @@ check-help:
 	@echo "changed with variable GTESTER_OPTIONS."
 
 SPEED = quick
-GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
+GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)
 GCOV_OPTIONS = -n $(if $(V),-f,)
 
 # gtester tests, possibly with verbose output
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V2 2/2] tests: Tweak the Makefile to produce per-test output.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On 03/11/2014 04:09 AM, Marcel Apfelbaum wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The effect is that the output will grow, but we will know
> which qtest failed. This helps when an asert is made on

s/asert/assert/

> other code and not in the test itself.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  tests/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index b17d41e..a8405c8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -273,7 +273,7 @@ check-help:
>  	@echo "changed with variable GTESTER_OPTIONS."
>  
>  SPEED = quick
> -GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
> +GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)

Isn't this effectively commenting out the rest of the line?  If so, why
not delete everything after the #?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V2 2/2] tests: Tweak the Makefile to produce per-test output.

[Marcel Apfelbaum]

On Tue, 2014-03-11 at 06:07 -0600, Eric Blake wrote:
> On 03/11/2014 04:09 AM, Marcel Apfelbaum wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > The effect is that the output will grow, but we will know
> > which qtest failed. This helps when an asert is made on
> 
> s/asert/assert/
Sure,

> 
> > other code and not in the test itself.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  tests/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/Makefile b/tests/Makefile
> > index b17d41e..a8405c8 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -273,7 +273,7 @@ check-help:
> >  	@echo "changed with variable GTESTER_OPTIONS."
> >  
> >  SPEED = quick
> > -GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
> > +GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)
> 
> Isn't this effectively commenting out the rest of the line?  If so, why
> not delete everything after the #?
Hi Eric, thank you for catching this.
Indeed, what I actually want to do is to remove the -q flag.
I sent V3 that fixes this,

Thanks,
Marcel

> 

Re: [Qemu-devel] [PATCH V2 2/2] tests: Tweak the Makefile to produce per-test output.

[Stefan Hajnoczi]

On Tue, Mar 11, 2014 at 12:09:10PM +0200, Marcel Apfelbaum wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The effect is that the output will grow, but we will know
> which qtest failed. This helps when an asert is made on
> other code and not in the test itself.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  tests/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index b17d41e..a8405c8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -273,7 +273,7 @@ check-help:
>  	@echo "changed with variable GTESTER_OPTIONS."
>  
>  SPEED = quick
> -GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
> +GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)
>  GCOV_OPTIONS = -n $(if $(V),-f,)
>  
>  # gtester tests, possibly with verbose output

I didn't propose this as a QEMU patch so I didn't add my Signed-off-by:.
I just posted a diff to show you that gtester without -q prints the test
binary names.

Please drop this patch and I'll send a final patch after I've checked
the output is still clean and doesn't duplicate information.

Re: [Qemu-devel] [PATCH V2 2/2] tests: Tweak the Makefile to produce per-test output.

[Marcel Apfelbaum]

On Tue, 2014-03-11 at 13:45 +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 11, 2014 at 12:09:10PM +0200, Marcel Apfelbaum wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > The effect is that the output will grow, but we will know
> > which qtest failed. This helps when an asert is made on
> > other code and not in the test itself.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  tests/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/Makefile b/tests/Makefile
> > index b17d41e..a8405c8 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -273,7 +273,7 @@ check-help:
> >  	@echo "changed with variable GTESTER_OPTIONS."
> >  
> >  SPEED = quick
> > -GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
> > +GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)
> >  GCOV_OPTIONS = -n $(if $(V),-f,)
> >  
> >  # gtester tests, possibly with verbose output
> 
> I didn't propose this as a QEMU patch so I didn't add my Signed-off-by:.
> I just posted a diff to show you that gtester without -q prints the test
> binary names.
> 
> Please drop this patch and I'll send a final patch after I've checked
> the output is still clean and doesn't duplicate information.
Sure,
Marcel