From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49650) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhG01-0006uM-SQ for qemu-devel@nongnu.org; Tue, 06 Sep 2016 09:01:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhFzw-00084a-28 for qemu-devel@nongnu.org; Tue, 06 Sep 2016 09:01:12 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:48126) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhFzv-00084W-QA for qemu-devel@nongnu.org; Tue, 06 Sep 2016 09:01:07 -0400 Date: Tue, 6 Sep 2016 09:01:06 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1446849559.3048.1473166866111.JavaMail.zimbra@redhat.com> In-Reply-To: <1486217545.12.1473166413883.JavaMail.zimbra@redhat.com> References: <20160906122639.11163-1-marcandre.lureau@redhat.com> <20160906122639.11163-11-marcandre.lureau@redhat.com> <20160906124655.GE10095@redhat.com> <1486217545.12.1473166413883.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 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: "Daniel P. Berrange" Cc: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , qemu-devel@nongnu.org, peter maydell , armbru@redhat.com, pbonzini@redhat.com Hi ----- Original Message ----- > Hi >=20 > ----- Original Message ----- > > 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. > >=20 > > 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 > This was changed to address Eric's comment: > https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg04554.html >=20 > Is it a problem to use temporary dir instead so tests could actually run > concurrently? >=20 > 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? >=20 > >=20 > > >=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-command.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 asy= nc) > > > "/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 asyn= c) > > > 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 d= ata) > > > 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(); > > > -- > > > 2.10.0 > > >=20 > > >=20 > >=20 > > Regards, > > Daniel > > -- > > |: http://berrange.com -o- http://www.flickr.com/photos/dberran= ge/ > > |:| > > |: http://libvirt.org -o- http://virt-manager.= org > > |:| > > |: http://autobuild.org -o- http://search.cpan.org/~danbe= rr/ > > |:| > > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-= vnc > > |:| > >=20 >=20