Re: [PATCH v2 3/3] tests/unit: make test-io-channel-command work on win32

[Daniel P . Berrangé]

On Fri, Sep 02, 2022 at 03:19:00PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This has been tested under msys2 & windows 11. I haven't tried to make
> it work with other environments yet, but that should be enough to
> validate the channel-command implementation anyway.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/unit/test-io-channel-command.c | 31 +++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/unit/test-io-channel-command.c 
> b/tests/unit/test-io-channel-command.c
> index 99056e07c0..56dcc0be19 100644
> --- a/tests/unit/test-io-channel-command.c
> +++ b/tests/unit/test-io-channel-command.c
> @@ -24,28 +24,38 @@
>  #include "qapi/error.h"
>  #include "qemu/module.h"
>  
> -#ifndef WIN32
> +#define TEST_FIFO "tests/test-io-channel-command.fifo"
> +
> +#ifdef WIN32
> +#define SOCAT "/bin/socat.exe"
> +#define SOCAT_SRC "PIPE:" TEST_FIFO ",wronly"
> +#define SOCAT_DST "PIPE:" TEST_FIFO ",rdonly"
> +#else
> +#define SOCAT "/bin/socat"
> +#define SOCAT_SRC "UNIX-LISTEN:" TEST_FIFO
> +#define SOCAT_DST "UNIX-CONNECT:" TEST_FIFO
> +#endif

This is changing the UNIX side to use UNIX sockets, while
the Windows side uses PIPEs...

> +
>  static void test_io_channel_command_fifo(bool async)
>  {
> -#define TEST_FIFO "tests/test-io-channel-command.fifo"
>      QIOChannel *src, *dst;
>      QIOChannelTest *test;
> -    const char *srcfifo = "PIPE:" TEST_FIFO ",wronly";
> -    const char *dstfifo = "PIPE:" TEST_FIFO ",rdonly";
>      const char *srcargv[] = {
> -        "/bin/socat", "-", srcfifo, NULL,
> +        SOCAT, "-", SOCAT_SRC, NULL,
>      };
>      const char *dstargv[] = {
> -        "/bin/socat", dstfifo, "-", NULL,
> +        SOCAT, SOCAT_DST, "-", NULL,
>      };

..but this original code uses FIFOs. Why flip the UNIX
side from PIPEs to UNIX sockets. Was this a mistake and
you intended to use UNIX sockets on Windows ?  If so,
how about we just use use UNIX sockets on every platform
and rename the test case.

>  
>      unlink(TEST_FIFO);
> -    if (access("/bin/socat", X_OK) < 0) {
> +    if (access(SOCAT, X_OK) < 0) {
>          return; /* Pretend success if socat is not present */
>      }
> +#ifndef WIN32
>      if (mkfifo(TEST_FIFO, 0600) < 0) {
>          abort();
>      }
> +#endif
>      src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
>                                                      O_WRONLY,
>                                                      &error_abort));
> @@ -80,10 +90,10 @@ static void test_io_channel_command_echo(bool async)
>      QIOChannel *ioc;
>      QIOChannelTest *test;
>      const char *socatargv[] = {
> -        "/bin/socat", "-", "-", NULL,
> +        SOCAT, "-", "-", NULL,
>      };
>  
> -    if (access("/bin/socat", X_OK) < 0) {
> +    if (access(SOCAT, X_OK) < 0) {
>          return; /* Pretend success if socat is not present */
>      }
>  
> @@ -107,7 +117,6 @@ static void test_io_channel_command_echo_sync(void)
>  {
>      test_io_channel_command_echo(false);
>  }
> -#endif
>  
>  int main(int argc, char **argv)
>  {
> @@ -115,7 +124,6 @@ int main(int argc, char **argv)
>  
>      g_test_init(&argc, &argv, NULL);
>  
> -#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",
> @@ -124,7 +132,6 @@ int main(int argc, char **argv)
>                      test_io_channel_command_echo_sync);
>      g_test_add_func("/io/channel/command/echo/async",
>                      test_io_channel_command_echo_async);
> -#endif
>  
>      return g_test_run();
>  }
> -- 
> 2.37.2
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|