Marcel Apfelbaum <marce...@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?
>
> Signed-off-by: Marcel Apfelbaum <marce...@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
