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

[Blue Swirl]

On Thu, Dec 20, 2012 at 8:26 PM, Jason Baron <address@hidden> 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 <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?

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

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