From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45707) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhFmL-000394-L1 for qemu-devel@nongnu.org; Tue, 06 Sep 2016 08:47:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhFmG-00054t-TX for qemu-devel@nongnu.org; Tue, 06 Sep 2016 08:47:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37390) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhFmG-00054j-LB for qemu-devel@nongnu.org; Tue, 06 Sep 2016 08:47:00 -0400 Date: Tue, 6 Sep 2016 13:46:56 +0100 From: "Daniel P. Berrange" Message-ID: <20160906124655.GE10095@redhat.com> Reply-To: "Daniel P. Berrange" References: <20160906122639.11163-1-marcandre.lureau@redhat.com> <20160906122639.11163-11-marcandre.lureau@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160906122639.11163-11-marcandre.lureau@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: qemu-devel@nongnu.org, peter.maydell@linaro.org, armbru@redhat.com, pbonzini@redhat.com On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-Andr=C3=A9 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. >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > Reviewed-by: Eric Blake > --- > tests/test-io-channel-command.c | 20 +++++++++++++------- > tests/test-qga.c | 3 ++- > 2 files changed, 15 insertions(+), 8 deletions(-) >=20 > diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-co= mmand.c > index 1d1f461..f99118e 100644 > --- a/tests/test-io-channel-command.c > +++ b/tests/test-io-channel-command.c > @@ -18,6 +18,7 @@ > * > */ > =20 > +#include > > #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 =3D g_strdup_printf("PIPE:%s,wronly", TEST_FIFO); > - char *dstfifo =3D g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO); > + char *tmpdir =3D g_strdup("/tmp/test-io-channel.XXXXXX"); > + g_assert_nonnull(mkdtemp(tmpdir)); > + > + char *fifo =3D g_strdup_printf("%s/command.fifo", tmpdir); > + char *srcfifo =3D g_strdup_printf("PIPE:%s,wronly", fifo); > + char *dstfifo =3D g_strdup_printf("PIPE:%s,rdonly", fifo); > const char *srcargv[] =3D { > "/bin/socat", "-", srcfifo, NULL, > }; > @@ -38,11 +42,10 @@ static void test_io_channel_command_fifo(bool async= ) > "/bin/socat", dstfifo, "-", NULL, > }; > =20 > - 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 =3D 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)); > =20 > +end: > + g_free(fifo); > g_free(srcfifo); > g_free(dstfifo); > - unlink(TEST_FIFO); > + g_rmdir(tmpdir); > + g_free(tmpdir); > } > =20 > =20 > 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 dat= a) > fixture->loop =3D g_main_loop_new(NULL, FALSE); > =20 > fixture->test_dir =3D g_strdup("/tmp/qgatest.XXXXXX"); > - g_assert_nonnull(mkdtemp(fixture->test_dir)); > + path =3D mkdtemp(fixture->test_dir); > + g_assert_nonnull(path); > =20 > path =3D g_build_filename(fixture->test_dir, "sock", NULL); > cwd =3D g_get_current_dir(); > --=20 > 2.10.0 >=20 >=20 Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|