Re: [Qemu-devel] [PULL 10/26] tests: fix small leak in test-io-channel-command

["Marc-André Lureau" <mlureau@redhat.com>]

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?

> 
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  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 :|
>