[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() fail
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure |
Date: |
Mon, 17 Feb 2014 17:49:31 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Stefan Hajnoczi <address@hidden> writes:
> The QEMU process stays running if the test case fails. This patch fixes
> the leak by installing a SIGABRT signal handler which invokes
> qtest_end().
>
> In order to make that work for assertion failures during qtest_init(),
> we need to initialize QTestState fields including file descriptors and
> pids carefully. qtest_quit() is then safe to call even during
> qtest_init().
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> tests/libqtest.c | 29 ++++++++++++++++++++++++++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 8b2b2d7..09a0481 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -44,6 +44,7 @@ struct QTestState
> bool irq_level[MAX_IRQ];
> GString *rx;
> pid_t qemu_pid; /* our child QEMU process */
> + struct sigaction sigact_old; /* restored on exit */
> };
>
> #define g_assert_no_errno(ret) do { \
> @@ -88,6 +89,11 @@ static int socket_accept(int sock)
> return ret;
> }
>
> +static void sigabrt_handler(int signo)
> +{
> + qtest_end();
> +}
> +
> QTestState *qtest_init(const char *extra_args)
> {
> QTestState *s;
> @@ -96,11 +102,22 @@ QTestState *qtest_init(const char *extra_args)
> gchar *qmp_socket_path;
> gchar *command;
> const char *qemu_binary;
> + struct sigaction sigact;
>
> qemu_binary = getenv("QTEST_QEMU_BINARY");
> g_assert(qemu_binary != NULL);
>
> - s = g_malloc(sizeof(*s));
> + s = g_malloc0(sizeof(*s));
> + s->fd = -1;
> + s->qmp_fd = -1;
> + s->qemu_pid = -1;
> +
> + /* Catch SIGABRT to clean up on g_assert() failure */
> + sigact = (struct sigaction){
> + .sa_handler = sigabrt_handler,
> + .sa_flags = SA_RESETHAND,
> + };
Assumes zero-initialization has the same effect as
sigemptyset(&sigact.sa_mask). Quoting POSIX:
The implementation of the sigemptyset() (or sigfillset()) function
could quite trivially clear (or set) all the bits in the signal set.
Alternatively, it would be reasonable to initialize part of the
structure, such as a version field, to permit binary-compatibility
between releases where the size of the set varies. For such
reasons, either sigemptyset() or sigfillset() must be called prior
to any other use of the signal set, even if such use is read-only
(for example, as an argument to sigpending()).
Looks like you better sigemptyset() here, for maximum portability.
> + sigaction(SIGABRT, &sigact, &s->sigact_old);
>
> socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> @@ -150,13 +167,19 @@ void qtest_quit(QTestState *s)
> {
> int status;
>
> + sigaction(SIGABRT, &s->sigact_old, NULL);
> +
Can this ever restore the action to anything but the default action?
> if (s->qemu_pid != -1) {
> kill(s->qemu_pid, SIGTERM);
> waitpid(s->qemu_pid, &status, 0);
> }
>
> - close(s->fd);
> - close(s->qmp_fd);
> + if (s->fd != -1) {
> + close(s->fd);
> + }
> + if (s->qmp_fd != -1) {
> + close(s->qmp_fd);
> + }
I generally don't bother to avoid close(-1).
> g_string_free(s->rx, true);
> g_free(s);
> }
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, (continued)
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Paolo Bonzini, 2014/02/17
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Markus Armbruster, 2014/02/17
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Stefan Hajnoczi, 2014/02/18
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Markus Armbruster, 2014/02/18
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Paolo Bonzini, 2014/02/18
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Markus Armbruster, 2014/02/18
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Stefan Hajnoczi, 2014/02/18
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Paolo Bonzini, 2014/02/18
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Daniel P. Berrange, 2014/02/18
- Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Paolo Bonzini, 2014/02/18
Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure,
Markus Armbruster <=
Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Peter Maydell, 2014/02/18
Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Markus Armbruster, 2014/02/18
Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Stefan Hajnoczi, 2014/02/18
Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure, Markus Armbruster, 2014/02/18