Re: [PULL 04/35] tests/unit: drop hacky race avoidance in test-io-channel-command

[Philippe Mathieu-Daudé]

On 6/2/23 14:11, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > Hi Alex, Thomas.
> >
> > On 26/1/23 12:22, Alex Bennée wrote:
> > > We don't need to play timing games to ensure one socat wins over the
> > > other, just create the fifo they both can use before spawning the
> > > processes. However in the process we need to disable two tests for
> > > Windows platforms as we don't have an abstraction for mkfifo().
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1403
> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > > Message-Id: <20230124180127.1881110-5-alex.bennee@linaro.org>
> > > diff --git a/tests/unit/test-io-channel-command.c
> > > b/tests/unit/test-io-channel-command.c
> > > index 19f72eab96..425e2f5594 100644
> > > --- a/tests/unit/test-io-channel-command.c
> > > +++ b/tests/unit/test-io-channel-command.c
> > > @@ -20,6 +20,8 @@
> > >      #include "qemu/osdep.h"
> > >    #include <glib/gstdio.h>
> > > +#include <sys/types.h>
> > > +#include <sys/stat.h>
> > >    #include "io/channel-command.h"
> > >    #include "io-channel-helpers.h"
> > >    #include "qapi/error.h"
> > > @@ -29,6 +31,7 @@
> > >      static char *socat = NULL;
> > >    +#ifndef _WIN32
> > >    static void test_io_channel_command_fifo(bool async)
> > >    {
> > >        g_autofree gchar *tmpdir = g_dir_make_tmp("qemu-test-io-channel.XXXXXX", 
> > > NULL);
> > > @@ -40,12 +43,13 @@ static void test_io_channel_command_fifo(bool async)
> > >        QIOChannel *src, *dst;
> > >        QIOChannelTest *test;
> > >    +    if (mkfifo(fifo, 0600)) {
> > > +        g_error("mkfifo: %s", strerror(errno));
> > > +    }
> > > +
> > >        src = QIO_CHANNEL(qio_channel_command_new_spawn((const char **) srcargv,
> > >                                                        O_WRONLY,
> > >                                                        &error_abort));
> > > -    /* try to avoid a race to create the socket */
> > > -    g_usleep(1000);
> > > -
> > >        dst = QIO_CHANNEL(qio_channel_command_new_spawn((const char **) dstargv,
> > >                                                        O_RDONLY,
> > >                                                        &error_abort));
> > > @@ -60,7 +64,6 @@ static void test_io_channel_command_fifo(bool async)
> >
> > Testing on Darwin/Aarch64 I'm getting (reproducible):
> >
> > 78/93 qemu:unit / test-io-channel-command             ERROR 2.38s
> > killed by signal 13 SIGPIPE
> > > > > MALLOC_PERTURB_=10 G_TEST_BUILDDIR=./tests/unit
> >      G_TEST_SRCDIR=tests/unit ./tests/unit/test-io-channel-command
> >     --tap -k
> > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> > stderr:
> > 2023/02/03 08:26:49 socat[32507] E
> > mkfifo(/var/folders/yj/r7khncsj4d77k04ybz9lw4tm0000gn/T/qemu-test-io-channel.GMARZ1/test-io-channel-command.fifo,
> > 438): File exists
> >
> > (test program exited with status code -13)
> >
> > TAP parsing error: Too few tests run (expected 4, got 0)
> > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >
> > We call g_rmdir(), but I see various qtests calling unlink()
> > before rmdir(). Do we need it here?
> >
> > +       g_unlink(fifo);
>
> Probably - the man page implies rmdir is expecting an empty directory.

Ah indeed; also it returns 0 on success. Then maybe:

  err = g_unlink(fifo);
  assert(err == 0);
  err = g_rmdir(tmpdir);
  assert(err == 0);

> > >        g_rmdir(tmpdir);
> > >    }