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

[Alex Bennée]

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.

>
>>       g_rmdir(tmpdir);
>>   }
>>   -
>>   static void test_io_channel_command_fifo_async(void)
>>   {
>>       if (!socat) {
>> @@ -80,6 +83,7 @@ static void test_io_channel_command_fifo_sync(void)
>>         test_io_channel_command_fifo(false);
>>   }
>> +#endif
>>       static void test_io_channel_command_echo(bool async)
>> @@ -124,10 +128,12 @@ int main(int argc, char **argv)
>>         socat = g_find_program_in_path("socat");
>>   +#ifndef _WIN32
>>       g_test_add_func("/io/channel/command/fifo/sync",
>>                       test_io_channel_command_fifo_sync);
>>       g_test_add_func("/io/channel/command/fifo/async",
>>                       test_io_channel_command_fifo_async);
>> +#endif
>>       g_test_add_func("/io/channel/command/echo/sync",
>>                       test_io_channel_command_echo_sync);
>>       g_test_add_func("/io/channel/command/echo/async",


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro