Patchwork [v2,1/3] qtest: Enable creation of multiple qemu instances

login
register
mail settings
Submitter Jason Baron
Date Dec. 20, 2012, 5:14 p.m.
Message ID <79b736d734e91b6c5b675b061fe9847e8bd24065.1356022464.git.jbaron@redhat.com>
Download mbox | patch
Permalink /patch/207687/
State New
Headers show

Comments

Jason Baron - Dec. 20, 2012, 5:14 p.m.
From: Jason Baron <jbaron@redhat.com>

Currently, the qtest harness can only spawn 1 qemu instance at a time because
the parent pid is used to create the socket files. Use 'mkdtemp()' in
combination with the parent pid to avoid conflicts.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 tests/libqtest.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)
Blue Swirl - Dec. 20, 2012, 8:07 p.m.
On Thu, Dec 20, 2012 at 5:14 PM, Jason Baron <jbaron@redhat.com> wrote:
> From: Jason Baron <jbaron@redhat.com>
>
> Currently, the qtest harness can only spawn 1 qemu instance at a time because
> the parent pid is used to create the socket files. Use 'mkdtemp()' in

But mkdtemp() is not available on Win32.

> combination with the parent pid to avoid conflicts.
>
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  tests/libqtest.c |   15 +++++++++------
>  1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 71b84c1..57665c9 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -41,6 +41,7 @@ struct QTestState
>      GString *rx;
>      gchar *pid_file;
>      char *socket_path, *qmp_socket_path;
> +    char *tmp_dir;
>  };
>
>  #define g_assert_no_errno(ret) do { \
> @@ -105,7 +106,6 @@ QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s;
>      int sock, qmpsock, ret, i;
> -    gchar *pid_file;
>      gchar *command;
>      const char *qemu_binary;
>      pid_t pid;
> @@ -115,9 +115,11 @@ 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());
> -    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
> +    s->tmp_dir = g_strdup_printf("/tmp/qtest-%d-XXXXXX", getpid());
> +    g_assert(mkdtemp(s->tmp_dir) != NULL);
> +    s->socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "sock");
> +    s->qmp_socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "qmp");
> +    s->pid_file = g_strdup_printf("%s/%s", s->tmp_dir, "pid");
>
>      sock = init_socket(s->socket_path);
>      qmpsock = init_socket(s->qmp_socket_path);
> @@ -131,7 +133,7 @@ QTestState *qtest_init(const char *extra_args)
>                                    "-pidfile %s "
>                                    "-machine accel=qtest "
>                                    "%s", qemu_binary, s->socket_path,
> -                                  s->qmp_socket_path, pid_file,
> +                                  s->qmp_socket_path, s->pid_file,
>                                    extra_args ?: "");
>
>          ret = system(command);
> @@ -143,7 +145,6 @@ QTestState *qtest_init(const char *extra_args)
>      s->qmp_fd = socket_accept(qmpsock);
>
>      s->rx = g_string_new("");
> -    s->pid_file = pid_file;
>      for (i = 0; i < MAX_IRQ; i++) {
>          s->irq_level[i] = false;
>      }
> @@ -172,9 +173,11 @@ void qtest_quit(QTestState *s)
>      unlink(s->pid_file);
>      unlink(s->socket_path);
>      unlink(s->qmp_socket_path);
> +    unlink(s->tmp_dir);

-EISDIR, rmdir() would be needed instead.

>      g_free(s->pid_file);
>      g_free(s->socket_path);
>      g_free(s->qmp_socket_path);
> +    g_free(s->tmp_dir);
>  }
>
>  static void socket_sendf(int fd, const char *fmt, va_list ap)
> --
> 1.7.1
>
Jason Baron - Dec. 20, 2012, 8:26 p.m.
On Thu, Dec 20, 2012 at 08:07:02PM +0000, Blue Swirl wrote:
> On Thu, Dec 20, 2012 at 5:14 PM, Jason Baron <jbaron@redhat.com> wrote:
> > From: Jason Baron <jbaron@redhat.com>
> >
> > Currently, the qtest harness can only spawn 1 qemu instance at a time because
> > the parent pid is used to create the socket files. Use 'mkdtemp()' in
> 
> But mkdtemp() is not available on Win32.

So this case important even for qtest?

> 
> > combination with the parent pid to avoid conflicts.
> >
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  tests/libqtest.c |   15 +++++++++------
> >  1 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/libqtest.c b/tests/libqtest.c
> > index 71b84c1..57665c9 100644
> > --- a/tests/libqtest.c
> > +++ b/tests/libqtest.c
> > @@ -41,6 +41,7 @@ struct QTestState
> >      GString *rx;
> >      gchar *pid_file;
> >      char *socket_path, *qmp_socket_path;
> > +    char *tmp_dir;
> >  };
> >
> >  #define g_assert_no_errno(ret) do { \
> > @@ -105,7 +106,6 @@ QTestState *qtest_init(const char *extra_args)
> >  {
> >      QTestState *s;
> >      int sock, qmpsock, ret, i;
> > -    gchar *pid_file;
> >      gchar *command;
> >      const char *qemu_binary;
> >      pid_t pid;
> > @@ -115,9 +115,11 @@ 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());
> > -    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
> > +    s->tmp_dir = g_strdup_printf("/tmp/qtest-%d-XXXXXX", getpid());
> > +    g_assert(mkdtemp(s->tmp_dir) != NULL);
> > +    s->socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "sock");
> > +    s->qmp_socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "qmp");
> > +    s->pid_file = g_strdup_printf("%s/%s", s->tmp_dir, "pid");
> >
> >      sock = init_socket(s->socket_path);
> >      qmpsock = init_socket(s->qmp_socket_path);
> > @@ -131,7 +133,7 @@ QTestState *qtest_init(const char *extra_args)
> >                                    "-pidfile %s "
> >                                    "-machine accel=qtest "
> >                                    "%s", qemu_binary, s->socket_path,
> > -                                  s->qmp_socket_path, pid_file,
> > +                                  s->qmp_socket_path, s->pid_file,
> >                                    extra_args ?: "");
> >
> >          ret = system(command);
> > @@ -143,7 +145,6 @@ QTestState *qtest_init(const char *extra_args)
> >      s->qmp_fd = socket_accept(qmpsock);
> >
> >      s->rx = g_string_new("");
> > -    s->pid_file = pid_file;
> >      for (i = 0; i < MAX_IRQ; i++) {
> >          s->irq_level[i] = false;
> >      }
> > @@ -172,9 +173,11 @@ void qtest_quit(QTestState *s)
> >      unlink(s->pid_file);
> >      unlink(s->socket_path);
> >      unlink(s->qmp_socket_path);
> > +    unlink(s->tmp_dir);
> 
> -EISDIR, rmdir() would be needed instead.
> 

'unlink()' tested fine on Linux. But yes, it might not be as portable.

I looked at tempnam() and mktemp(), but they both generate linker warnings.
'mkstemp()' could be used but its awkward to delete the file right after
its created so that bind() can create it. Plus, it could be a greater
security risk in that the filename is easier to determine.

We could write our own random file string generator then, if mkdtemp(),
isn't ok.

> >      g_free(s->pid_file);
> >      g_free(s->socket_path);
> >      g_free(s->qmp_socket_path);
> > +    g_free(s->tmp_dir);
> >  }
> >
> >  static void socket_sendf(int fd, const char *fmt, va_list ap)
> > --
> > 1.7.1
> >
Blue Swirl - Dec. 20, 2012, 8:38 p.m.
On Thu, Dec 20, 2012 at 8:26 PM, Jason Baron <jbaron@redhat.com> wrote:
> On Thu, Dec 20, 2012 at 08:07:02PM +0000, Blue Swirl wrote:
>> On Thu, Dec 20, 2012 at 5:14 PM, Jason Baron <jbaron@redhat.com> wrote:
>> > From: Jason Baron <jbaron@redhat.com>
>> >
>> > Currently, the qtest harness can only spawn 1 qemu instance at a time because
>> > the parent pid is used to create the socket files. Use 'mkdtemp()' in
>>
>> But mkdtemp() is not available on Win32.
>
> So this case important even for qtest?

Well, there's $(EXESUF) handling for qtest also. But it does not seem
to build now:
  LINK  tests/test-thread-pool.exe
tests/test-thread-pool.o: In function `test_cancel':
/src/qemu/tests/test-thread-pool.c:177: undefined reference to
`___sync_val_compare_and_swap_4'
tests/test-thread-pool.o: In function `worker_cb':
/src/qemu/tests/test-thread-pool.c:18: undefined reference to
`___sync_fetch_and_add_4'

>
>>
>> > combination with the parent pid to avoid conflicts.
>> >
>> > Signed-off-by: Jason Baron <jbaron@redhat.com>
>> > ---
>> >  tests/libqtest.c |   15 +++++++++------
>> >  1 files changed, 9 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/tests/libqtest.c b/tests/libqtest.c
>> > index 71b84c1..57665c9 100644
>> > --- a/tests/libqtest.c
>> > +++ b/tests/libqtest.c
>> > @@ -41,6 +41,7 @@ struct QTestState
>> >      GString *rx;
>> >      gchar *pid_file;
>> >      char *socket_path, *qmp_socket_path;
>> > +    char *tmp_dir;
>> >  };
>> >
>> >  #define g_assert_no_errno(ret) do { \
>> > @@ -105,7 +106,6 @@ QTestState *qtest_init(const char *extra_args)
>> >  {
>> >      QTestState *s;
>> >      int sock, qmpsock, ret, i;
>> > -    gchar *pid_file;
>> >      gchar *command;
>> >      const char *qemu_binary;
>> >      pid_t pid;
>> > @@ -115,9 +115,11 @@ 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());
>> > -    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
>> > +    s->tmp_dir = g_strdup_printf("/tmp/qtest-%d-XXXXXX", getpid());
>> > +    g_assert(mkdtemp(s->tmp_dir) != NULL);
>> > +    s->socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "sock");
>> > +    s->qmp_socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "qmp");
>> > +    s->pid_file = g_strdup_printf("%s/%s", s->tmp_dir, "pid");
>> >
>> >      sock = init_socket(s->socket_path);
>> >      qmpsock = init_socket(s->qmp_socket_path);
>> > @@ -131,7 +133,7 @@ QTestState *qtest_init(const char *extra_args)
>> >                                    "-pidfile %s "
>> >                                    "-machine accel=qtest "
>> >                                    "%s", qemu_binary, s->socket_path,
>> > -                                  s->qmp_socket_path, pid_file,
>> > +                                  s->qmp_socket_path, s->pid_file,
>> >                                    extra_args ?: "");
>> >
>> >          ret = system(command);
>> > @@ -143,7 +145,6 @@ QTestState *qtest_init(const char *extra_args)
>> >      s->qmp_fd = socket_accept(qmpsock);
>> >
>> >      s->rx = g_string_new("");
>> > -    s->pid_file = pid_file;
>> >      for (i = 0; i < MAX_IRQ; i++) {
>> >          s->irq_level[i] = false;
>> >      }
>> > @@ -172,9 +173,11 @@ void qtest_quit(QTestState *s)
>> >      unlink(s->pid_file);
>> >      unlink(s->socket_path);
>> >      unlink(s->qmp_socket_path);
>> > +    unlink(s->tmp_dir);
>>
>> -EISDIR, rmdir() would be needed instead.
>>
>
> 'unlink()' tested fine on Linux. But yes, it might not be as portable.
>
> I looked at tempnam() and mktemp(), but they both generate linker warnings.
> 'mkstemp()' could be used but its awkward to delete the file right after
> its created so that bind() can create it. Plus, it could be a greater
> security risk in that the filename is easier to determine.
>
> We could write our own random file string generator then, if mkdtemp(),
> isn't ok.

Or introduce qemu_mkdtemp() which resolves either to mkdtemp() or to a
custom implementation:

http://stackoverflow.com/questions/278439/creating-a-temporary-directory-in-windows

>
>> >      g_free(s->pid_file);
>> >      g_free(s->socket_path);
>> >      g_free(s->qmp_socket_path);
>> > +    g_free(s->tmp_dir);
>> >  }
>> >
>> >  static void socket_sendf(int fd, const char *fmt, va_list ap)
>> > --
>> > 1.7.1
>> >
Markus Armbruster - Dec. 21, 2012, 7:47 a.m.
Jason Baron <jbaron@redhat.com> writes:

> On Thu, Dec 20, 2012 at 08:07:02PM +0000, Blue Swirl wrote:
>> On Thu, Dec 20, 2012 at 5:14 PM, Jason Baron <jbaron@redhat.com> wrote:
>> > From: Jason Baron <jbaron@redhat.com>
[...]
>> > @@ -172,9 +173,11 @@ void qtest_quit(QTestState *s)
>> >      unlink(s->pid_file);
>> >      unlink(s->socket_path);
>> >      unlink(s->qmp_socket_path);
>> > +    unlink(s->tmp_dir);
>> 
>> -EISDIR, rmdir() would be needed instead.
>> 
>
> 'unlink()' tested fine on Linux. But yes, it might not be as portable.

s/might not be as/isn't/

SUSv7:

[EPERM]
    The file named by path is a directory, and either the calling
    process does not have appropriate privileges, or the implementation
    prohibits using unlink() on directories.

[...]

Patch

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 71b84c1..57665c9 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -41,6 +41,7 @@  struct QTestState
     GString *rx;
     gchar *pid_file;
     char *socket_path, *qmp_socket_path;
+    char *tmp_dir;
 };
 
 #define g_assert_no_errno(ret) do { \
@@ -105,7 +106,6 @@  QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, ret, i;
-    gchar *pid_file;
     gchar *command;
     const char *qemu_binary;
     pid_t pid;
@@ -115,9 +115,11 @@  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());
-    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
+    s->tmp_dir = g_strdup_printf("/tmp/qtest-%d-XXXXXX", getpid());
+    g_assert(mkdtemp(s->tmp_dir) != NULL);
+    s->socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "sock");
+    s->qmp_socket_path = g_strdup_printf("%s/%s", s->tmp_dir, "qmp");
+    s->pid_file = g_strdup_printf("%s/%s", s->tmp_dir, "pid");
 
     sock = init_socket(s->socket_path);
     qmpsock = init_socket(s->qmp_socket_path);
@@ -131,7 +133,7 @@  QTestState *qtest_init(const char *extra_args)
                                   "-pidfile %s "
                                   "-machine accel=qtest "
                                   "%s", qemu_binary, s->socket_path,
-                                  s->qmp_socket_path, pid_file,
+                                  s->qmp_socket_path, s->pid_file,
                                   extra_args ?: "");
 
         ret = system(command);
@@ -143,7 +145,6 @@  QTestState *qtest_init(const char *extra_args)
     s->qmp_fd = socket_accept(qmpsock);
 
     s->rx = g_string_new("");
-    s->pid_file = pid_file;
     for (i = 0; i < MAX_IRQ; i++) {
         s->irq_level[i] = false;
     }
@@ -172,9 +173,11 @@  void qtest_quit(QTestState *s)
     unlink(s->pid_file);
     unlink(s->socket_path);
     unlink(s->qmp_socket_path);
+    unlink(s->tmp_dir);
     g_free(s->pid_file);
     g_free(s->socket_path);
     g_free(s->qmp_socket_path);
+    g_free(s->tmp_dir);
 }
 
 static void socket_sendf(int fd, const char *fmt, va_list ap)