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

Alex Bennée posted 35 patches 3 years ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Thomas Huth <thuth@redhat.com>, Wainer dos Santos Moschetta <wainersm@redhat.com>, Beraldo Leal <bleal@redhat.com>, Ed Maste <emaste@freebsd.org>, Li-Wen Hsu <lwhsu@freebsd.org>, Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Peter Maydell <peter.maydell@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Mahmoud Mandour <ma.mandourr@gmail.com>, Markus Armbruster <armbru@redhat.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, John G Johnson <john.g.johnson@oracle.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Alexander Bulekov <alxndr@bu.edu>, Bandan Das <bsd@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Darren Kenny <darren.kenny@oracle.com>, Qiuhao Li <Qiuhao.Li@outlook.com>, John Snow <jsnow@redhat.com>, Cleber Rosa <crosa@redhat.com>, Michael Roth <michael.roth@amd.com>
There is a newer version of this series
[PULL 04/35] tests/unit: drop hacky race avoidance in test-io-channel-command
Posted by Alex Bennée 3 years ago
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)
     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",
-- 
2.34.1


Re: [PULL 04/35] tests/unit: drop hacky race avoidance in test-io-channel-command
Posted by Philippe Mathieu-Daudé 3 years ago
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);

>       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",


Re: [PULL 04/35] tests/unit: drop hacky race avoidance in test-io-channel-command
Posted by Alex Bennée 3 years ago
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
Re: [PULL 04/35] tests/unit: drop hacky race avoidance in test-io-channel-command
Posted by Philippe Mathieu-Daudé 3 years ago
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);
>>>    }