Re: [Qemu-devel] [PATCH 1/2] qtest: unlink QEMU pid file after startup

[Markus Armbruster <armbru@redhat.com>]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> After starting the QEMU process and initializing the QMP connection, we
> can read the pid file and unlink it.
>
> Just stash away the pid instead of the pid filename.  This way we can
> avoid pid file leaks since running tests may abort(3) without cleanup.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqtest.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 359d571..dd93be8 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -43,8 +43,8 @@ struct QTestState
>      int qmp_fd;
>      bool irq_level[MAX_IRQ];
>      GString *rx;
> -    gchar *pid_file; /* QEMU PID file */
>      int child_pid;   /* Child process created to execute QEMU */
> +    pid_t qemu_pid;  /* QEMU process spawned by our child */
>      char *socket_path, *qmp_socket_path;
>  };
>  
> @@ -90,13 +90,13 @@ static int socket_accept(int sock)
>      return ret;
>  }
>  
> -static pid_t qtest_qemu_pid(QTestState *s)
> +static pid_t read_pid_file(const char *pid_file)
>  {
>      FILE *f;
>      char buffer[1024];
>      pid_t pid = -1;
>  
> -    f = fopen(s->pid_file, "r");
> +    f = fopen(pid_file, "r");
>      if (f) {
>          if (fgets(buffer, sizeof(buffer), f)) {
>              pid = atoi(buffer);
> @@ -147,7 +147,6 @@ QTestState *qtest_init(const char *extra_args)
>      s->qmp_fd = socket_accept(qmpsock);
>  
>      s->rx = g_string_new("");
> -    s->pid_file = pid_file;
>      s->child_pid = pid;
>      for (i = 0; i < MAX_IRQ; i++) {
>          s->irq_level[i] = false;
> @@ -157,8 +156,12 @@ QTestState *qtest_init(const char *extra_args)
>      qtest_qmp_discard_response(s, "");
>      qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
>  
> +    s->qemu_pid = read_pid_file(pid_file);
> +    unlink(pid_file);

unlink() can fail, but I guess there's not much useful you can do when
it fails.  Also, no worse than before your patch (see last hunk).

> +    g_free(pid_file);
> +
>      if (getenv("QTEST_STOP")) {
> -        kill(qtest_qemu_pid(s), SIGSTOP);
> +        kill(s->qemu_pid, SIGSTOP);
>      }
>  
>      return s;
> @@ -168,19 +171,16 @@ void qtest_quit(QTestState *s)
>  {
>      int status;
>  
> -    pid_t pid = qtest_qemu_pid(s);
> -    if (pid != -1) {
> -        kill(pid, SIGTERM);
> -        waitpid(pid, &status, 0);
> +    if (s->qemu_pid != -1) {
> +        kill(s->qemu_pid, SIGTERM);
> +        waitpid(s->qemu_pid, &status, 0);
>      }
>  
>      close(s->fd);
>      close(s->qmp_fd);
>      g_string_free(s->rx, true);
> -    unlink(s->pid_file);
>      unlink(s->socket_path);
>      unlink(s->qmp_socket_path);
> -    g_free(s->pid_file);
>      g_free(s->socket_path);
>      g_free(s->qmp_socket_path);
>      g_free(s);