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

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

[Stefan Hajnoczi]

This pull request has been rebased onto qemu.git/master and retested.

The series fixes init_socket() failures when reused PIDs cause temporary file
name collisions.

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.5.3

[Qemu-devel] [PULL 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.5.3

[Qemu-devel] [PULL 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.5.3

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

[Peter Maydell]

On 3 February 2014 15:20, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> This pull request has been rebased onto qemu.git/master and retested.
>
> The series fixes init_socket() failures when reused PIDs cause temporary file
> name collisions.

Your pull request appears to be missing the critically
important part, ie the git repo/branch/tag information
to pull from... ("git request-pull" is your friend here.)

thanks
-- PMM

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

[Stefan Hajnoczi]

On Mon, Feb 03, 2014 at 04:01:00PM +0000, Peter Maydell wrote:
> On 3 February 2014 15:20, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > This pull request has been rebased onto qemu.git/master and retested.
> >
> > The series fixes init_socket() failures when reused PIDs cause temporary file
> > name collisions.
> 
> Your pull request appears to be missing the critically
> important part, ie the git repo/branch/tag information
> to pull from... ("git request-pull" is your friend here.)

Oops, I ran the wrong script.  Here are the full details and signed tag:

The following changes since commit 2f61120c10da9128357510debc8e66880cd2bfdc:

  Merge remote-tracking branch 'qmp-unstable/queue/qmp' into staging (2014-02-01 23:32:31 +0000)

are available in the git repository at:


  git://github.com/stefanha/qemu.git tags/qtest-for-peter

for you to fetch changes up to 56db2e5843256c857addb17deb743109330649be:

  qtest: unlink UNIX domain sockets after connecting (2014-02-03 16:06:24 +0100)

----------------------------------------------------------------
qtest resource cleanup patches

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

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

[Peter Maydell]

On 4 February 2014 08:31, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Mon, Feb 03, 2014 at 04:01:00PM +0000, Peter Maydell wrote:
>> On 3 February 2014 15:20, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > This pull request has been rebased onto qemu.git/master and retested.
>> >
>> > The series fixes init_socket() failures when reused PIDs cause temporary file
>> > name collisions.
>>
>> Your pull request appears to be missing the critically
>> important part, ie the git repo/branch/tag information
>> to pull from... ("git request-pull" is your friend here.)
>
> Oops, I ran the wrong script.  Here are the full details and signed tag:

Applied, thanks.

-- PMM

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

[Andreas Färber]

Am 07.02.2014 17:41, schrieb Peter Maydell:
> On 4 February 2014 08:31, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> On Mon, Feb 03, 2014 at 04:01:00PM +0000, Peter Maydell wrote:
>>> On 3 February 2014 15:20, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>>> This pull request has been rebased onto qemu.git/master and retested.
>>>>
>>>> The series fixes init_socket() failures when reused PIDs cause temporary file
>>>> name collisions.
>>>
>>> Your pull request appears to be missing the critically
>>> important part, ie the git repo/branch/tag information
>>> to pull from... ("git request-pull" is your friend here.)
>>
>> Oops, I ran the wrong script.  Here are the full details and signed tag:
> 
> Applied, thanks.

Thanks. I've stumbled over a remaining pid file leak:

http://patchwork.ozlabs.org/patch/318576/

Regards,
Andreas

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