From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47302) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhFsn-0000Tl-Iz for qemu-devel@nongnu.org; Tue, 06 Sep 2016 08:53:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhFsi-0006FY-Tb for qemu-devel@nongnu.org; Tue, 06 Sep 2016 08:53:44 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:47400) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhFsi-0006Eu-LM for qemu-devel@nongnu.org; Tue, 06 Sep 2016 08:53:40 -0400 Date: Tue, 6 Sep 2016 08:53:33 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1486217545.12.1473166413883.JavaMail.zimbra@redhat.com> In-Reply-To: <20160906124655.GE10095@redhat.com> References: <20160906122639.11163-1-marcandre.lureau@redhat.com> <20160906122639.11163-11-marcandre.lureau@redhat.com> <20160906124655.GE10095@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 ----- > 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. This was changed to address Eric's comment: https://lists.nongnu.org/archiv= e/html/qemu-devel/2016-07/msg04554.html Is it a problem to use temporary dir instead so tests could actually run co= ncurrently? Anything wrong with this patch? >=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 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(); > > -- > > 2.10.0 > >=20 > >=20 >=20 > Regards, > Daniel > -- > |: 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 :| >=20