| Message ID | 79b736d734e91b6c5b675b061fe9847e8bd24065.1356022464.git.jbaron@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 > >
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 >> >
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. [...]
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)