| Message ID | 1394453534-24334-2-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 10, 2014 at 02:12:13PM +0200, Marcel Apfelbaum wrote: > @@ -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)); > + Please move this into socket_accept() so qtest_init() doesn't need to know about setsockopt() details. It should just get an fd or -errno return value from socket_accept()... > 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); ...then you can ensure that we unlink the sockets even on failure. And then something like g_assert(s->fd >= 0 && s->qmp_fd >= 0) would be fine. The process can abort() at this point.
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? > > 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
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
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); @@ -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 + */ +bool qtest_state_valid(QTestState *s); + +/** * qtest_qmp_discard_response: * @s: #QTestState instance to operate on. * @fmt...: QMP message to send to qemu
'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. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- tests/libqtest.c | 26 +++++++++++++++++++++----- tests/libqtest.h | 8 ++++++++ 2 files changed, 29 insertions(+), 5 deletions(-)