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

[Daniel P. Berrange]

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.

> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  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 :|