Re: [Qemu-devel] [PATCH v2 1/3] qtest: Enable creation of multiple qemu instances

[Jason Baron]

On Thu, Dec 20, 2012 at 08:07:02PM +0000, Blue Swirl wrote:
> On Thu, Dec 20, 2012 at 5:14 PM, Jason Baron <address@hidden> wrote:
> > From: Jason Baron <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  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
> >