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

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> 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?

>
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
>  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