| Message ID | 20160906122639.11163-11-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote: > srcfifo && dstfifo must be freed in error case, however unlink() may > delete a file from a different context. Instead, use mkdtemp()/rmdir() > for the temporary files. This isn't making much sense to me. What different context are you referring to ? The fifo filename is unique to this particular unit test and AFAIK, we don't support running the same unit test multiple times concurrently, so the existing unlink looks safe to me. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > tests/test-io-channel-command.c | 20 +++++++++++++------- > tests/test-qga.c | 3 ++- > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c > index 1d1f461..f99118e 100644 > --- a/tests/test-io-channel-command.c > +++ b/tests/test-io-channel-command.c > @@ -18,6 +18,7 @@ > * > */ > > +#include <glib/gstdio.h> > > #include "qemu/osdep.h" > #include "io/channel-command.h" > #include "io-channel-helpers.h" > @@ -26,11 +27,14 @@ > #ifndef WIN32 > static void test_io_channel_command_fifo(bool async) > { > -#define TEST_FIFO "tests/test-io-channel-command.fifo" > QIOChannel *src, *dst; > QIOChannelTest *test; > - char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO); > - char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO); > + char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX"); > + g_assert_nonnull(mkdtemp(tmpdir)); > + > + char *fifo = g_strdup_printf("%s/command.fifo", tmpdir); > + char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo); > + char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo); > const char *srcargv[] = { > "/bin/socat", "-", srcfifo, NULL, > }; > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async) > "/bin/socat", dstfifo, "-", NULL, > }; > > - unlink(TEST_FIFO); > if (access("/bin/socat", X_OK) < 0) { > - return; /* Pretend success if socat is not present */ > + goto end; /* Pretend success if socat is not present */ > } > - if (mkfifo(TEST_FIFO, 0600) < 0) { > + if (mkfifo(fifo, 0600) < 0) { > abort(); > } > src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv, > @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async) > object_unref(OBJECT(src)); > object_unref(OBJECT(dst)); > > +end: > + g_free(fifo); > g_free(srcfifo); > g_free(dstfifo); > - unlink(TEST_FIFO); > + g_rmdir(tmpdir); > + g_free(tmpdir); > } > > > diff --git a/tests/test-qga.c b/tests/test-qga.c > index 21f44f8..0d1acef 100644 > --- a/tests/test-qga.c > +++ b/tests/test-qga.c > @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data) > fixture->loop = g_main_loop_new(NULL, FALSE); > > fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX"); > - g_assert_nonnull(mkdtemp(fixture->test_dir)); > + path = mkdtemp(fixture->test_dir); > + g_assert_nonnull(path); > > path = g_build_filename(fixture->test_dir, "sock", NULL); > cwd = g_get_current_dir(); > -- > 2.10.0 > > Regards, Daniel
Hi ----- Original Message ----- > On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote: > > srcfifo && dstfifo must be freed in error case, however unlink() may > > delete a file from a different context. Instead, use mkdtemp()/rmdir() > > for the temporary files. > > This isn't making much sense to me. What different context are > you referring to ? The fifo filename is unique to this particular > unit test and AFAIK, we don't support running the same unit test > multiple times concurrently, so the existing unlink looks safe to > me. This was changed to address Eric's comment: https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg04554.html Is it a problem to use temporary dir instead so tests could actually run concurrently? Anything wrong with this patch? > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > --- > > tests/test-io-channel-command.c | 20 +++++++++++++------- > > tests/test-qga.c | 3 ++- > > 2 files changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/tests/test-io-channel-command.c > > b/tests/test-io-channel-command.c > > index 1d1f461..f99118e 100644 > > --- a/tests/test-io-channel-command.c > > +++ b/tests/test-io-channel-command.c > > @@ -18,6 +18,7 @@ > > * > > */ > > > > +#include <glib/gstdio.h> > > > > #include "qemu/osdep.h" > > #include "io/channel-command.h" > > #include "io-channel-helpers.h" > > @@ -26,11 +27,14 @@ > > #ifndef WIN32 > > static void test_io_channel_command_fifo(bool async) > > { > > -#define TEST_FIFO "tests/test-io-channel-command.fifo" > > QIOChannel *src, *dst; > > QIOChannelTest *test; > > - char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO); > > - char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO); > > + char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX"); > > + g_assert_nonnull(mkdtemp(tmpdir)); > > + > > + char *fifo = g_strdup_printf("%s/command.fifo", tmpdir); > > + char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo); > > + char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo); > > const char *srcargv[] = { > > "/bin/socat", "-", srcfifo, NULL, > > }; > > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async) > > "/bin/socat", dstfifo, "-", NULL, > > }; > > > > - unlink(TEST_FIFO); > > if (access("/bin/socat", X_OK) < 0) { > > - return; /* Pretend success if socat is not present */ > > + goto end; /* Pretend success if socat is not present */ > > } > > - if (mkfifo(TEST_FIFO, 0600) < 0) { > > + if (mkfifo(fifo, 0600) < 0) { > > abort(); > > } > > src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv, > > @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async) > > object_unref(OBJECT(src)); > > object_unref(OBJECT(dst)); > > > > +end: > > + g_free(fifo); > > g_free(srcfifo); > > g_free(dstfifo); > > - unlink(TEST_FIFO); > > + g_rmdir(tmpdir); > > + g_free(tmpdir); > > } > > > > > > diff --git a/tests/test-qga.c b/tests/test-qga.c > > index 21f44f8..0d1acef 100644 > > --- a/tests/test-qga.c > > +++ b/tests/test-qga.c > > @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data) > > fixture->loop = g_main_loop_new(NULL, FALSE); > > > > fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX"); > > - g_assert_nonnull(mkdtemp(fixture->test_dir)); > > + path = mkdtemp(fixture->test_dir); > > + g_assert_nonnull(path); > > > > path = g_build_filename(fixture->test_dir, "sock", NULL); > > cwd = g_get_current_dir(); > > -- > > 2.10.0 > > > > > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| >
On Tue, Sep 06, 2016 at 08:53:33AM -0400, Marc-André Lureau wrote: > Hi > > ----- Original Message ----- > > On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote: > > > srcfifo && dstfifo must be freed in error case, however unlink() may > > > delete a file from a different context. Instead, use mkdtemp()/rmdir() > > > for the temporary files. > > > > This isn't making much sense to me. What different context are > > you referring to ? The fifo filename is unique to this particular > > unit test and AFAIK, we don't support running the same unit test > > multiple times concurrently, so the existing unlink looks safe to > > me. > > This was changed to address Eric's comment: https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg04554.html That could have been trivially fixed by reordering the cleanup code eg unlink(TEST_FIFO); end: g_free(srcfifo); g_free(dstfifo); } > > Is it a problem to use temporary dir instead so tests could actually > run concurrently? I've always written tests on the assumption that distinct unit tests can run concurrently, but you'll never have the same unit test run concurrently. IMHO running the same unit test concurrently is a non-goal. > Anything wrong with this patch? It needlessly adds extra code complexity to solve a non-goal IMHO. > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > --- > > > tests/test-io-channel-command.c | 20 +++++++++++++------- > > > tests/test-qga.c | 3 ++- > > > 2 files changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/tests/test-io-channel-command.c > > > b/tests/test-io-channel-command.c > > > index 1d1f461..f99118e 100644 > > > --- a/tests/test-io-channel-command.c > > > +++ b/tests/test-io-channel-command.c > > > @@ -18,6 +18,7 @@ > > > * > > > */ > > > > > > +#include <glib/gstdio.h> > > > > > > #include "qemu/osdep.h" > > > #include "io/channel-command.h" > > > #include "io-channel-helpers.h" > > > @@ -26,11 +27,14 @@ > > > #ifndef WIN32 > > > static void test_io_channel_command_fifo(bool async) > > > { > > > -#define TEST_FIFO "tests/test-io-channel-command.fifo" > > > QIOChannel *src, *dst; > > > QIOChannelTest *test; > > > - char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO); > > > - char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO); > > > + char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX"); > > > + g_assert_nonnull(mkdtemp(tmpdir)); > > > + > > > + char *fifo = g_strdup_printf("%s/command.fifo", tmpdir); > > > + char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo); > > > + char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo); > > > const char *srcargv[] = { > > > "/bin/socat", "-", srcfifo, NULL, > > > }; > > > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async) > > > "/bin/socat", dstfifo, "-", NULL, > > > }; > > > > > > - unlink(TEST_FIFO); > > > if (access("/bin/socat", X_OK) < 0) { > > > - return; /* Pretend success if socat is not present */ > > > + goto end; /* Pretend success if socat is not present */ > > > } > > > - if (mkfifo(TEST_FIFO, 0600) < 0) { > > > + if (mkfifo(fifo, 0600) < 0) { > > > abort(); > > > } > > > src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv, > > > @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async) > > > object_unref(OBJECT(src)); > > > object_unref(OBJECT(dst)); > > > > > > +end: > > > + g_free(fifo); > > > g_free(srcfifo); > > > g_free(dstfifo); > > > - unlink(TEST_FIFO); > > > + g_rmdir(tmpdir); > > > + g_free(tmpdir); > > > } > > > > > > > > > diff --git a/tests/test-qga.c b/tests/test-qga.c > > > index 21f44f8..0d1acef 100644 > > > --- a/tests/test-qga.c > > > +++ b/tests/test-qga.c > > > @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data) > > > fixture->loop = g_main_loop_new(NULL, FALSE); > > > > > > fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX"); > > > - g_assert_nonnull(mkdtemp(fixture->test_dir)); > > > + path = mkdtemp(fixture->test_dir); > > > + g_assert_nonnull(path); > > > > > > path = g_build_filename(fixture->test_dir, "sock", NULL); > > > cwd = g_get_current_dir(); Regards, Daniel
Hi ----- Original Message ----- > Hi > > ----- Original Message ----- > > On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-André Lureau wrote: > > > srcfifo && dstfifo must be freed in error case, however unlink() may > > > delete a file from a different context. Instead, use mkdtemp()/rmdir() > > > for the temporary files. > > > > This isn't making much sense to me. What different context are > > you referring to ? The fifo filename is unique to this particular > > unit test and AFAIK, we don't support running the same unit test > > multiple times concurrently, so the existing unlink looks safe to > > me. > > This was changed to address Eric's comment: > https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg04554.html > > Is it a problem to use temporary dir instead so tests could actually run > concurrently? > > Anything wrong with this patch? Actually, the patch leaks /tmp/test-io-channel.XXXXXX dirs now, oops. At least, there is a missing unlink for the fifo. Anything else? > > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > --- > > > tests/test-io-channel-command.c | 20 +++++++++++++------- > > > tests/test-qga.c | 3 ++- > > > 2 files changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/tests/test-io-channel-command.c > > > b/tests/test-io-channel-command.c > > > index 1d1f461..f99118e 100644 > > > --- a/tests/test-io-channel-command.c > > > +++ b/tests/test-io-channel-command.c > > > @@ -18,6 +18,7 @@ > > > * > > > */ > > > > > > +#include <glib/gstdio.h> > > > > > > #include "qemu/osdep.h" > > > #include "io/channel-command.h" > > > #include "io-channel-helpers.h" > > > @@ -26,11 +27,14 @@ > > > #ifndef WIN32 > > > static void test_io_channel_command_fifo(bool async) > > > { > > > -#define TEST_FIFO "tests/test-io-channel-command.fifo" > > > QIOChannel *src, *dst; > > > QIOChannelTest *test; > > > - char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO); > > > - char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO); > > > + char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX"); > > > + g_assert_nonnull(mkdtemp(tmpdir)); > > > + > > > + char *fifo = g_strdup_printf("%s/command.fifo", tmpdir); > > > + char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo); > > > + char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo); > > > const char *srcargv[] = { > > > "/bin/socat", "-", srcfifo, NULL, > > > }; > > > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async) > > > "/bin/socat", dstfifo, "-", NULL, > > > }; > > > > > > - unlink(TEST_FIFO); > > > if (access("/bin/socat", X_OK) < 0) { > > > - return; /* Pretend success if socat is not present */ > > > + goto end; /* Pretend success if socat is not present */ > > > } > > > - if (mkfifo(TEST_FIFO, 0600) < 0) { > > > + if (mkfifo(fifo, 0600) < 0) { > > > abort(); > > > } > > > src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv, > > > @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async) > > > object_unref(OBJECT(src)); > > > object_unref(OBJECT(dst)); > > > > > > +end: > > > + g_free(fifo); > > > g_free(srcfifo); > > > g_free(dstfifo); > > > - unlink(TEST_FIFO); > > > + g_rmdir(tmpdir); > > > + g_free(tmpdir); > > > } > > > > > > > > > diff --git a/tests/test-qga.c b/tests/test-qga.c > > > index 21f44f8..0d1acef 100644 > > > --- a/tests/test-qga.c > > > +++ b/tests/test-qga.c > > > @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data) > > > fixture->loop = g_main_loop_new(NULL, FALSE); > > > > > > fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX"); > > > - g_assert_nonnull(mkdtemp(fixture->test_dir)); > > > + path = mkdtemp(fixture->test_dir); > > > + g_assert_nonnull(path); > > > > > > path = g_build_filename(fixture->test_dir, "sock", NULL); > > > cwd = g_get_current_dir(); > > > -- > > > 2.10.0 > > > > > > > > > > Regards, > > Daniel > > -- > > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ > > |:| > > |: http://libvirt.org -o- http://virt-manager.org > > |:| > > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ > > |:| > > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc > > |:| > > >
diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c index 1d1f461..f99118e 100644 --- a/tests/test-io-channel-command.c +++ b/tests/test-io-channel-command.c @@ -18,6 +18,7 @@ * */ +#include <glib/gstdio.h> #include "qemu/osdep.h" #include "io/channel-command.h" #include "io-channel-helpers.h" @@ -26,11 +27,14 @@ #ifndef WIN32 static void test_io_channel_command_fifo(bool async) { -#define TEST_FIFO "tests/test-io-channel-command.fifo" QIOChannel *src, *dst; QIOChannelTest *test; - char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO); - char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO); + char *tmpdir = g_strdup("/tmp/test-io-channel.XXXXXX"); + g_assert_nonnull(mkdtemp(tmpdir)); + + char *fifo = g_strdup_printf("%s/command.fifo", tmpdir); + char *srcfifo = g_strdup_printf("PIPE:%s,wronly", fifo); + char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", fifo); const char *srcargv[] = { "/bin/socat", "-", srcfifo, NULL, }; @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async) "/bin/socat", dstfifo, "-", NULL, }; - unlink(TEST_FIFO); if (access("/bin/socat", X_OK) < 0) { - return; /* Pretend success if socat is not present */ + goto end; /* Pretend success if socat is not present */ } - if (mkfifo(TEST_FIFO, 0600) < 0) { + if (mkfifo(fifo, 0600) < 0) { abort(); } src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv, @@ -59,9 +62,12 @@ static void test_io_channel_command_fifo(bool async) object_unref(OBJECT(src)); object_unref(OBJECT(dst)); +end: + g_free(fifo); g_free(srcfifo); g_free(dstfifo); - unlink(TEST_FIFO); + g_rmdir(tmpdir); + g_free(tmpdir); } diff --git a/tests/test-qga.c b/tests/test-qga.c index 21f44f8..0d1acef 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -55,7 +55,8 @@ fixture_setup(TestFixture *fixture, gconstpointer data) fixture->loop = g_main_loop_new(NULL, FALSE); fixture->test_dir = g_strdup("/tmp/qgatest.XXXXXX"); - g_assert_nonnull(mkdtemp(fixture->test_dir)); + path = mkdtemp(fixture->test_dir); + g_assert_nonnull(path); path = g_build_filename(fixture->test_dir, "sock", NULL); cwd = g_get_current_dir();