[Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Stefan Hajnoczi]

GLib uses abort(3) to exit failed test cases.  As a result, the pid file and
UNIX domain sockets for a running test are leaked upon failure.

Since abort(3) does not call atexit(3) handler functions, we could set up a
SIGABRT handler that performs cleanup.  But there are other conditions where
processes die, like SIGSEGV or SIGBUS.

Let's unlink pid files and UNIX domain sockets as soon as the QEMU process has
initialized and connections have been made.  This eliminates the possibility of
leaking these files.

Note that the actual QEMU process is orphaned when a test case fails.  This
series does not fix that problem.

Stefan Hajnoczi (2):
  qtest: unlink QEMU pid file after startup
  qtest: unlink UNIX domain sockets after connecting

 tests/libqtest.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

-- 
1.8.4.2

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

[Stefan Hajnoczi]

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);
+    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);
-- 
1.8.4.2

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

[Markus Armbruster]

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

[Qemu-devel] [PATCH 2/2] qtest: unlink UNIX domain sockets after connecting

[Stefan Hajnoczi]

UNIX domain sockets are leaked when tests call abort(3) (indirectly via
glib assert functions).

Unlink the files immediately after the connection has been established
to avoid leaks.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index dd93be8..c9a4f89 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -45,7 +45,6 @@ struct QTestState
     GString *rx;
     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;
 };
 
 #define g_assert_no_errno(ret) do { \
@@ -110,6 +109,8 @@ QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, i;
+    gchar *socket_path;
+    gchar *qmp_socket_path;
     gchar *pid_file;
     gchar *command;
     const char *qemu_binary;
@@ -120,12 +121,12 @@ QTestState *qtest_init(const char *extra_args)
 
     s = g_malloc(sizeof(*s));
 
-    s->socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
-    s->qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
+    socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
+    qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
     pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
 
-    sock = init_socket(s->socket_path);
-    qmpsock = init_socket(s->qmp_socket_path);
+    sock = init_socket(socket_path);
+    qmpsock = init_socket(qmp_socket_path);
 
     pid = fork();
     if (pid == 0) {
@@ -136,8 +137,8 @@ QTestState *qtest_init(const char *extra_args)
                                   "-pidfile %s "
                                   "-machine accel=qtest "
                                   "-display none "
-                                  "%s", qemu_binary, s->socket_path,
-                                  s->qmp_socket_path, pid_file,
+                                  "%s", qemu_binary, socket_path,
+                                  qmp_socket_path, pid_file,
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);
@@ -145,6 +146,10 @@ QTestState *qtest_init(const char *extra_args)
 
     s->fd = socket_accept(sock);
     s->qmp_fd = socket_accept(qmpsock);
+    unlink(socket_path);
+    unlink(qmp_socket_path);
+    g_free(socket_path);
+    g_free(qmp_socket_path);
 
     s->rx = g_string_new("");
     s->child_pid = pid;
@@ -179,10 +184,6 @@ void qtest_quit(QTestState *s)
     close(s->fd);
     close(s->qmp_fd);
     g_string_free(s->rx, true);
-    unlink(s->socket_path);
-    unlink(s->qmp_socket_path);
-    g_free(s->socket_path);
-    g_free(s->qmp_socket_path);
     g_free(s);
 }
 
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Stefan Hajnoczi]

On Thu, Nov 21, 2013 at 12:03:53PM +0100, Stefan Hajnoczi wrote:
> GLib uses abort(3) to exit failed test cases.  As a result, the pid file and
> UNIX domain sockets for a running test are leaked upon failure.
> 
> Since abort(3) does not call atexit(3) handler functions, we could set up a
> SIGABRT handler that performs cleanup.  But there are other conditions where
> processes die, like SIGSEGV or SIGBUS.
> 
> Let's unlink pid files and UNIX domain sockets as soon as the QEMU process has
> initialized and connections have been made.  This eliminates the possibility of
> leaking these files.
> 
> Note that the actual QEMU process is orphaned when a test case fails.  This
> series does not fix that problem.
> 
> Stefan Hajnoczi (2):
>   qtest: unlink QEMU pid file after startup
>   qtest: unlink UNIX domain sockets after connecting
> 
>  tests/libqtest.c | 45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)

Ping

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> GLib uses abort(3) to exit failed test cases.  As a result, the pid file and
> UNIX domain sockets for a running test are leaked upon failure.
>
> Since abort(3) does not call atexit(3) handler functions, we could set up a
> SIGABRT handler that performs cleanup.  But there are other conditions where
> processes die, like SIGSEGV or SIGBUS.
>
> Let's unlink pid files and UNIX domain sockets as soon as the QEMU process has
> initialized and connections have been made.  This eliminates the possibility of
> leaking these files.
>
> Note that the actual QEMU process is orphaned when a test case fails.  This
> series does not fix that problem.

I guess for more reliable and complete cleanup, we'd need a monitor
process that adopts and handles orphans, and removes left-behind files.
To make the latter work, tests must create files only in a temporary
directory the monitor process can then remove wholesale.

Anyway, this series is a welcome improvement.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Peter Maydell]

On 21 November 2013 11:03, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> GLib uses abort(3) to exit failed test cases.  As a result, the pid file and
> UNIX domain sockets for a running test are leaked upon failure.
>
> Since abort(3) does not call atexit(3) handler functions, we could set up a
> SIGABRT handler that performs cleanup.  But there are other conditions where
> processes die, like SIGSEGV or SIGBUS.
>
> Let's unlink pid files and UNIX domain sockets as soon as the QEMU process has
> initialized and connections have been made.  This eliminates the possibility of
> leaking these files.

So looking back through mailing list history suggests that these patches
are supposed to avoid intermittent make check failures like:

TEST: tests/qom-test... (pid=5078)
  /i386/qom/none:                                                      **
ERROR:/home/petmay01/linaro/qemu-for-merges/tests/libqtest.c:71:init_socket:
assertion failed (ret != -1): (-1 != -1)
FAIL
GTester: last random seed: R02S79ea313790bc9a8b21d9af5ed55c2fff
(pid=5080)
  /i386/qom/pc:                                                        OK
  /i386/qom/isapc:                                                     OK
  /i386/qom/q35:                                                       OK
FAIL: tests/qom-test

but this patch series doesn't actually say that's what it's for,
so does it fix that kind of error?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Stefan Hajnoczi]

On Fri, Jan 31, 2014 at 1:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 21 November 2013 11:03, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> GLib uses abort(3) to exit failed test cases.  As a result, the pid file and
>> UNIX domain sockets for a running test are leaked upon failure.
>>
>> Since abort(3) does not call atexit(3) handler functions, we could set up a
>> SIGABRT handler that performs cleanup.  But there are other conditions where
>> processes die, like SIGSEGV or SIGBUS.
>>
>> Let's unlink pid files and UNIX domain sockets as soon as the QEMU process has
>> initialized and connections have been made.  This eliminates the possibility of
>> leaking these files.
>
> So looking back through mailing list history suggests that these patches
> are supposed to avoid intermittent make check failures like:
>
> TEST: tests/qom-test... (pid=5078)
>   /i386/qom/none:                                                      **
> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/libqtest.c:71:init_socket:
> assertion failed (ret != -1): (-1 != -1)
> FAIL
> GTester: last random seed: R02S79ea313790bc9a8b21d9af5ed55c2fff
> (pid=5080)
>   /i386/qom/pc:                                                        OK
>   /i386/qom/isapc:                                                     OK
>   /i386/qom/q35:                                                       OK
> FAIL: tests/qom-test
>
> but this patch series doesn't actually say that's what it's for,
> so does it fix that kind of error?

I might help but I'm not sure.  The assertion failure you posted
happened on Gerd's buildslaves, I haven't reproduced it myself and
don't really understand what's wrong there.

However, I noticed that my /tmp is getting cluttered with pid files
and UNIX domain sockets from qtest.  I also noticed that QEMU
processes are left running (this is not fixed by my series) when tests
fail.

So this series at least stops us from cluttering /tmp.

I haven't reproduced the init_socket() assertion failure so I'm not
sure what the root cause is.

Stefan

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Gerd Hoffmann]

  Hi,

> > ERROR:/home/petmay01/linaro/qemu-for-merges/tests/libqtest.c:71:init_socket:
> > assertion failed (ret != -1): (-1 != -1)
> > FAIL
> > GTester: last random seed: R02S79ea313790bc9a8b21d9af5ed55c2fff
> > (pid=5080)
> >   /i386/qom/pc:                                                        OK
> >   /i386/qom/isapc:                                                     OK
> >   /i386/qom/q35:                                                       OK
> > FAIL: tests/qom-test
> >
> > but this patch series doesn't actually say that's what it's for,
> > so does it fix that kind of error?
> 
> I might help but I'm not sure.  The assertion failure you posted
> happened on Gerd's buildslaves, I haven't reproduced it myself and
> don't really understand what's wrong there.

It seems to have stopped happening on the buildslaves,
not sure whenever this is just luck or whenever it was
fixed by some patch.

> However, I noticed that my /tmp is getting cluttered with pid files
> and UNIX domain sockets from qtest.  I also noticed that QEMU
> processes are left running (this is not fixed by my series) when tests
> fail.
> 
> So this series at least stops us from cluttering /tmp.

Removing the clutter from /tmp unbroke the buildslaves, so not
cluttering tmp certainly is a good thing.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Stefan Hajnoczi]

On Fri, Jan 31, 2014 at 12:07:34AM +0000, Peter Maydell wrote:
> On 21 November 2013 11:03, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > GLib uses abort(3) to exit failed test cases.  As a result, the pid file and
> > UNIX domain sockets for a running test are leaked upon failure.
> >
> > Since abort(3) does not call atexit(3) handler functions, we could set up a
> > SIGABRT handler that performs cleanup.  But there are other conditions where
> > processes die, like SIGSEGV or SIGBUS.
> >
> > Let's unlink pid files and UNIX domain sockets as soon as the QEMU process has
> > initialized and connections have been made.  This eliminates the possibility of
> > leaking these files.
> 
> So looking back through mailing list history suggests that these patches
> are supposed to avoid intermittent make check failures like:
> 
> TEST: tests/qom-test... (pid=5078)
>   /i386/qom/none:                                                      **
> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/libqtest.c:71:init_socket:
> assertion failed (ret != -1): (-1 != -1)
> FAIL
> GTester: last random seed: R02S79ea313790bc9a8b21d9af5ed55c2fff
> (pid=5080)
>   /i386/qom/pc:                                                        OK
>   /i386/qom/isapc:                                                     OK
>   /i386/qom/q35:                                                       OK
> FAIL: tests/qom-test
> 
> but this patch series doesn't actually say that's what it's for,
> so does it fix that kind of error?

I still think we should merge these patches :).  Are you happy to merge
them?

Stefan

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Peter Maydell]

On 3 February 2014 09:54, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> I still think we should merge these patches :).  Are you happy to merge
> them?

They look like they're fixing a bug to me, so yes. You could
make my life easier by arranging for them to appear in a pull
request...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Stefan Hajnoczi]

On Mon, Feb 03, 2014 at 10:30:08AM +0000, Peter Maydell wrote:
> On 3 February 2014 09:54, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > I still think we should merge these patches :).  Are you happy to merge
> > them?
> 
> They look like they're fixing a bug to me, so yes. You could
> make my life easier by arranging for them to appear in a pull
> request...

Your wish is my command.  I have sent a pull request (after rebasing and
retesting).

Stefan

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Andreas Färber]

Am 03.02.2014 10:54, schrieb Stefan Hajnoczi:
> On Fri, Jan 31, 2014 at 12:07:34AM +0000, Peter Maydell wrote:
>> On 21 November 2013 11:03, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> GLib uses abort(3) to exit failed test cases.  As a result, the pid file and
>>> UNIX domain sockets for a running test are leaked upon failure.
>>>
>>> Since abort(3) does not call atexit(3) handler functions, we could set up a
>>> SIGABRT handler that performs cleanup.  But there are other conditions where
>>> processes die, like SIGSEGV or SIGBUS.
>>>
>>> Let's unlink pid files and UNIX domain sockets as soon as the QEMU process has
>>> initialized and connections have been made.  This eliminates the possibility of
>>> leaking these files.
>>
>> So looking back through mailing list history suggests that these patches
>> are supposed to avoid intermittent make check failures like:
>>
>> TEST: tests/qom-test... (pid=5078)
>>   /i386/qom/none:                                                      **
>> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/libqtest.c:71:init_socket:
>> assertion failed (ret != -1): (-1 != -1)
>> FAIL
>> GTester: last random seed: R02S79ea313790bc9a8b21d9af5ed55c2fff
>> (pid=5080)
>>   /i386/qom/pc:                                                        OK
>>   /i386/qom/isapc:                                                     OK
>>   /i386/qom/q35:                                                       OK
>> FAIL: tests/qom-test
>>
>> but this patch series doesn't actually say that's what it's for,
>> so does it fix that kind of error?
> 
> I still think we should merge these patches :).

+1

As an explanation, the temporary files contain the PID. When they remain
behind due to test failure *and* the PID wraps around and file names
thus happen to match, the error was triggered, and thereby not on each
run but seemingly "sometimes".

I am not 100% familiar with the unlinking and code ordering here, but it
had looked sane to me back when I looked at it, I just didn't feel
confident enough for a Reviewed-by. I could give it a spin and add a
Tested-by if that reassures PMM.

Andreas

>  Are you happy to merge
> them?
> 
> Stefan
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/2] qtest: don't leak pid files and UNIX domain sockets

[Peter Maydell]

On 3 February 2014 11:10, Andreas Färber <afaerber@suse.de> wrote:
> As an explanation, the temporary files contain the PID. When they remain
> behind due to test failure *and* the PID wraps around and file names
> thus happen to match, the error was triggered, and thereby not on each
> run but seemingly "sometimes".

Yeah, that's what it looks like to me too.

> I am not 100% familiar with the unlinking and code ordering here, but it
> had looked sane to me back when I looked at it, I just didn't feel
> confident enough for a Reviewed-by. I could give it a spin and add a
> Tested-by if that reassures PMM.

I'm happy the code is good, I'm just being lazy about directly
applying patches to master, especially since Anthony's patches
db is currently stalled and not updating :-) (I guess I need to
deal with that some time, though, since we don't have a mechanism
for handling patches that fall through the cracks between
subsystems beyond "somebody with commit access applies them"...)

thanks
-- PMM