[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-c
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command |
Date: |
Tue, 6 Sep 2016 09:01:06 -0400 (EDT) |
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 <address@hidden>
> > > Reviewed-by: Eric Blake <address@hidden>
> > > ---
> > > 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
> > |:|
> >
>
- [Qemu-devel] [PULL 05/26] tests: fix test-cutils leaks, (continued)
- [Qemu-devel] [PULL 05/26] tests: fix test-cutils leaks, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 04/26] qga: free remaining leaking state, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 06/26] tests: fix test-vmstate leaks, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 07/26] tests: fix test-iov leaks, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 08/26] tests: fix check-qom-interface leaks, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 09/26] tests: fix check-qom-proplist leaks, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 11/26] tests: fix leak in test-string-input-visitor, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 12/26] portio: keep references on portio, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 13/26] pc: simplify passing qemu_irq, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 14/26] pc: don't leak a20_line, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 16/26] acpi-build: fix array leak, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 15/26] machine: use class base init generated name, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 17/26] tests: fix qom-test leaks, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 18/26] pc: free i8259, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 19/26] pc: keep gsi reference, Marc-André Lureau, 2016/09/06
- [Qemu-devel] [PULL 20/26] sd: free timer, Marc-André Lureau, 2016/09/06