From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49449) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhFz6-0006CD-Cl for qemu-devel@nongnu.org; Tue, 06 Sep 2016 09:00:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhFz2-0007tH-0d for qemu-devel@nongnu.org; Tue, 06 Sep 2016 09:00:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33418) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhFz1-0007so-P3 for qemu-devel@nongnu.org; Tue, 06 Sep 2016 09:00:11 -0400 Date: Tue, 6 Sep 2016 14:00:07 +0100 From: "Daniel P. Berrange" Message-ID: <20160906130007.GH10095@redhat.com> Reply-To: "Daniel P. Berrange" 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-Disposition: inline In-Reply-To: <1486217545.12.1473166413883.JavaMail.zimbra@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: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , qemu-devel@nongnu.org, peter maydell , armbru@redhat.com, pbonzini@redhat.com On Tue, Sep 06, 2016 at 08:53:33AM -0400, Marc-Andr=C3=A9 Lureau wrote: > Hi >=20 > ----- Original Message ----- > > On Tue, Sep 06, 2016 at 04:26:23PM +0400, Marc-Andr=C3=A9 Lureau wrot= e: > > > srcfifo && dstfifo must be freed in error case, however unlink() ma= y > > > delete a file from a different context. Instead, use mkdtemp()/rmdi= r() > > > 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/ar= chive/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); } >=20 > 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=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 a= sync) > > > "/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 as= ync) > > > 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= data) > > > 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(); 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 :|