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

[Qemu-devel] [PATCH V4] 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.

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

diff --git a/tests/libqtest.c b/tests/libqtest.c
index f587d36..c9e78aa 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;
@@ -147,12 +152,16 @@ QTestState *qtest_init(const char *extra_args)
     }
 
     s->fd = socket_accept(sock);
-    s->qmp_fd = socket_accept(qmpsock);
+    if (s->fd >= 0) {
+        s->qmp_fd = socket_accept(qmpsock);
+    }
     unlink(socket_path);
     unlink(qmp_socket_path);
     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 V4] tests/libqtest: Fix possible deadlock in qtest initialization

[Stefan Hajnoczi]

On Tue, Mar 11, 2014 at 03:00:34PM +0200, Marcel Apfelbaum wrote:
> '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.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  tests/libqtest.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Marcel Apfelbaum]

On Tue, 2014-03-11 at 19:51 +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 11, 2014 at 03:00:34PM +0200, Marcel Apfelbaum wrote:
> > '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.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  tests/libqtest.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks for the help, Stefan!
Please be aware that until you take care of the
"abort during qtest_init" issue, qtest will crash
on such occasions.

Last thing,  which maintainer should take this?
Thanks,
Marcel

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

[Stefan Hajnoczi]

On Tue, Mar 11, 2014 at 09:20:55PM +0200, Marcel Apfelbaum wrote:
> On Tue, 2014-03-11 at 19:51 +0100, Stefan Hajnoczi wrote:
> > On Tue, Mar 11, 2014 at 03:00:34PM +0200, Marcel Apfelbaum wrote:
> > > '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.
> > > 
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > >  tests/libqtest.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Thanks for the help, Stefan!
> Please be aware that until you take care of the
> "abort during qtest_init" issue, qtest will crash
> on such occasions.
> 
> Last thing,  which maintainer should take this?

Andreas Färber is the most likely victim^H^H^H^H^H^Hmaintainer:

"I don't feel like the official qtest maintainer, but I care and am
actively working on it. I would consider it best to have individual
tests maintained by subsystem maintainers and only define a maintainer
for the core libqtest.c/qtest.c code."

Although I'm interested in libqtest myself I don't have the bandwidth to
review/merge all future patches in that area.

Stefan