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

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Sep 06, 2016 at 08:53:33AM -0400, Marc-André Lureau wrote:
> 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

That could have been trivially fixed by reordering the cleanup code

eg

    unlink(TEST_FIFO);

  end:
    g_free(srcfifo);
    g_free(dstfifo);
 }

> 
> 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é 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();


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 :|