Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

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);
>  }