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

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Wed, 2014-03-12 at 10:42 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.a@redhat.com> writes:
> 
> > '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.
> > It gets worse as the make check-qtest-* gets stuck without
> > notifying which test exactly has problems, so debugging can
> > be a challenge.
> >
> > The solution has two parts:
> >  - Use a timeout for the socket.
> >  - Expose a qtest_state_valid that checks that the connections
> >    with Qemu are OK.
> > Asserting qtest_state_valid in each test after qtest_init
> > is a must, as we need to trace which test failed.
> 
> Is that assert in the next patch?
Yes, for every qtest test, but Stefan NACKED it :(.
The reason would be that he didn't want to have a manual assertion
on each test just to see the test name. The alternative is to
output each test using gtest options.

I have my doubts because even that we see the test name,
we still receive the assert in qtestlib, but it is not
the qtestlib's fault, it is the test's supplied command line fault,
this why I think the test should assert.

But this is too philosophical for me :), the other pacth
that prevents the gtest getting stuck was accepted, I am now
looking for a maintainer to put it into his tree.

Thanks,
Marcel
 
> 
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  tests/libqtest.c | 26 +++++++++++++++++++++-----
> >  tests/libqtest.h |  8 ++++++++
> >  2 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/libqtest.c b/tests/libqtest.c
> > index f587d36..93dfa81 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;
> >  
> > @@ -83,7 +84,6 @@ static int socket_accept(int sock)
> >      do {
> >          ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
> >      } while (ret == -1 && errno == EINTR);
> > -    g_assert_no_errno(ret);
> >      close(sock);
> >  
> >      return ret;
> > @@ -111,6 +111,8 @@ QTestState *qtest_init(const char *extra_args)
> >      gchar *command;
> >      const char *qemu_binary;
> >      struct sigaction sigact;
> > +    struct timeval socket_timeout = { .tv_sec = SOCKET_TIMEOUT,
> > +                                      .tv_usec = 0 };
> >  
> >      qemu_binary = getenv("QTEST_QEMU_BINARY");
> >      g_assert(qemu_binary != NULL);
> > @@ -123,6 +125,11 @@ QTestState *qtest_init(const char *extra_args)
> >      sock = init_socket(socket_path);
> >      qmpsock = init_socket(qmp_socket_path);
> >  
> > +    setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> > +               sizeof(socket_timeout));
> > +    setsockopt(qmpsock, SOL_SOCKET, SO_RCVTIMEO, (void *)&socket_timeout,
> > +               sizeof(socket_timeout));
> > +
> >      /* Catch SIGABRT to clean up on g_assert() failure */
> >      sigact = (struct sigaction){
> >          .sa_handler = sigabrt_handler,
> > @@ -147,7 +154,9 @@ 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);
> 
> The conditional looks odd.  But without it, we could wait for timeout
> two times.
> 
> If s->fd < 0, then s->qmp_fd remains 0, and should not be used.  Are you
> sure that's the case?  qtest_quit() and qtest_qmpv() use it.  Reachable?
> 
> Perhaps s->qmp_fd = -1 would be safer.
> 
> Could you explain to me again why we want to continue after
> socket_accept() fails, regardless of whether it fails due to timeout or
> something else?
> 
> > @@ -158,9 +167,11 @@ QTestState *qtest_init(const char *extra_args)
> >          s->irq_level[i] = false;
> >      }
> >  
> > -    /* Read the QMP greeting and then do the handshake */
> > -    qtest_qmp_discard_response(s, "");
> > -    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> > +    if (qtest_state_valid(s)) {
> > +        /* Read the QMP greeting and then do the handshake */
> > +        qtest_qmp_discard_response(s, "");
> > +        qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
> > +    }
> >  
> >      if (getenv("QTEST_STOP")) {
> >          kill(s->qemu_pid, SIGSTOP);
> > @@ -169,6 +180,11 @@ QTestState *qtest_init(const char *extra_args)
> >      return s;
> >  }
> >  
> > +bool qtest_state_valid(QTestState *s)
> > +{
> > +    return (s->fd >= 0) && (s->qmp_fd >= 0);
> > +}
> > +
> >  void qtest_quit(QTestState *s)
> >  {
> >      sigaction(SIGABRT, &s->sigact_old, NULL);
> > diff --git a/tests/libqtest.h b/tests/libqtest.h
> > index 9deebdc..39a37b1 100644
> > --- a/tests/libqtest.h
> > +++ b/tests/libqtest.h
> > @@ -45,6 +45,14 @@ QTestState *qtest_init(const char *extra_args);
> >  void qtest_quit(QTestState *s);
> >  
> >  /**
> > + * qtest_state_valid:
> > + * @state: #QTestState instance to check
> > + *
> > + * Returns: True if qtest was initialized successfully
> 
> If you mean the macro defined by stdbool.h, that one's spelled with a
> lower case 't'.
> 
> > + */
> > +bool qtest_state_valid(QTestState *s);
> > +
> > +/**
> >   * qtest_qmp_discard_response:
> >   * @s: #QTestState instance to operate on.
> >   * @fmt...: QMP message to send to qemu