qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] io/command: implement portable spawn
@ 2022-09-02 11:18 marcandre.lureau
  2022-09-02 11:18 ` [PATCH v2 1/3] io/command: use glib GSpawn, instead of open-coding fork/exec marcandre.lureau
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: marcandre.lureau @ 2022-09-02 11:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé, Marc-André Lureau

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

A small series, based on earlier "[PATCH] io/command: implement portable spawn"
to enable Windows support of command spawning in the io/ subsystem.

Marc-André Lureau (3):
  io/command: use glib GSpawn, instead of open-coding fork/exec
  io/command: implement support for win32
  tests/unit: make test-io-channel-command work on win32

 include/io/channel-command.h         |   2 +-
 io/channel-command.c                 | 163 +++++++++------------------
 tests/unit/test-io-channel-command.c |  31 +++--
 3 files changed, 75 insertions(+), 121 deletions(-)

-- 
2.37.2



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] io/command: use glib GSpawn, instead of open-coding fork/exec
  2022-09-02 11:18 [PATCH v2 0/3] io/command: implement portable spawn marcandre.lureau
@ 2022-09-02 11:18 ` marcandre.lureau
  2022-10-03 10:38   ` Daniel P. Berrangé
  2022-09-02 11:18 ` [PATCH v2 2/3] io/command: implement support for win32 marcandre.lureau
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: marcandre.lureau @ 2022-09-02 11:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé, Marc-André Lureau

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Simplify qio_channel_command_new_spawn() with GSpawn API. This will
allow to build for WIN32 in the following patches.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/io/channel-command.h |   2 +-
 io/channel-command.c         | 105 ++++++-----------------------------
 2 files changed, 19 insertions(+), 88 deletions(-)

diff --git a/include/io/channel-command.h b/include/io/channel-command.h
index 305ac1d280..8dc58273c0 100644
--- a/include/io/channel-command.h
+++ b/include/io/channel-command.h
@@ -41,7 +41,7 @@ struct QIOChannelCommand {
     QIOChannel parent;
     int writefd;
     int readfd;
-    pid_t pid;
+    GPid pid;
 };
 
 
diff --git a/io/channel-command.c b/io/channel-command.c
index 9f2f4a1793..35ba14c6a2 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -31,7 +31,7 @@
  * qio_channel_command_new_pid:
  * @writefd: the FD connected to the command's stdin
  * @readfd: the FD connected to the command's stdout
- * @pid: the PID of the running child command
+ * @pid: the PID/HANDLE of the running child command
  * @errp: pointer to a NULL-initialized error object
  *
  * Create a channel for performing I/O with the
@@ -50,7 +50,7 @@
 static QIOChannelCommand *
 qio_channel_command_new_pid(int writefd,
                             int readfd,
-                            pid_t pid)
+                            GPid pid)
 {
     QIOChannelCommand *ioc;
 
@@ -69,94 +69,24 @@ qio_channel_command_new_spawn(const char *const argv[],
                               int flags,
                               Error **errp)
 {
-    pid_t pid = -1;
-    int stdinfd[2] = { -1, -1 };
-    int stdoutfd[2] = { -1, -1 };
-    int devnull = -1;
-    bool stdinnull = false, stdoutnull = false;
-    QIOChannelCommand *ioc;
+    g_autoptr(GError) err = NULL;
+    GPid pid = 0;
+    GSpawnFlags gflags = G_SPAWN_CLOEXEC_PIPES;
+    int stdinfd = -1, stdoutfd = -1;
 
     flags = flags & O_ACCMODE;
-
-    if (flags == O_RDONLY) {
-        stdinnull = true;
-    }
-    if (flags == O_WRONLY) {
-        stdoutnull = true;
-    }
-
-    if (stdinnull || stdoutnull) {
-        devnull = open("/dev/null", O_RDWR);
-        if (devnull < 0) {
-            error_setg_errno(errp, errno,
-                             "Unable to open /dev/null");
-            goto error;
-        }
-    }
-
-    if ((!stdinnull && !g_unix_open_pipe(stdinfd, FD_CLOEXEC, NULL)) ||
-        (!stdoutnull && !g_unix_open_pipe(stdoutfd, FD_CLOEXEC, NULL))) {
-        error_setg_errno(errp, errno,
-                         "Unable to open pipe");
-        goto error;
-    }
-
-    pid = qemu_fork(errp);
-    if (pid < 0) {
-        goto error;
-    }
-
-    if (pid == 0) { /* child */
-        dup2(stdinnull ? devnull : stdinfd[0], STDIN_FILENO);
-        dup2(stdoutnull ? devnull : stdoutfd[1], STDOUT_FILENO);
-        /* Leave stderr connected to qemu's stderr */
-
-        if (!stdinnull) {
-            close(stdinfd[0]);
-            close(stdinfd[1]);
-        }
-        if (!stdoutnull) {
-            close(stdoutfd[0]);
-            close(stdoutfd[1]);
-        }
-        if (devnull != -1) {
-            close(devnull);
-        }
-
-        execv(argv[0], (char * const *)argv);
-        _exit(1);
+    gflags |= flags == O_WRONLY ? G_SPAWN_STDOUT_TO_DEV_NULL : 0;
+
+    if (!g_spawn_async_with_pipes(NULL, (char **)argv, NULL, gflags, NULL, NULL,
+                                  &pid,
+                                  flags == O_RDONLY ? NULL : &stdinfd,
+                                  flags == O_WRONLY ? NULL : &stdoutfd,
+                                  NULL, &err)) {
+        error_setg(errp, "%s", err->message);
+        return NULL;
     }
 
-    if (!stdinnull) {
-        close(stdinfd[0]);
-    }
-    if (!stdoutnull) {
-        close(stdoutfd[1]);
-    }
-
-    ioc = qio_channel_command_new_pid(stdinnull ? devnull : stdinfd[1],
-                                      stdoutnull ? devnull : stdoutfd[0],
-                                      pid);
-    trace_qio_channel_command_new_spawn(ioc, argv[0], flags);
-    return ioc;
-
- error:
-    if (devnull != -1) {
-        close(devnull);
-    }
-    if (stdinfd[0] != -1) {
-        close(stdinfd[0]);
-    }
-    if (stdinfd[1] != -1) {
-        close(stdinfd[1]);
-    }
-    if (stdoutfd[0] != -1) {
-        close(stdoutfd[0]);
-    }
-    if (stdoutfd[1] != -1) {
-        close(stdoutfd[1]);
-    }
-    return NULL;
+    return qio_channel_command_new_pid(stdinfd, stdoutfd, pid);
 }
 
 #else /* WIN32 */
@@ -221,7 +151,7 @@ static void qio_channel_command_init(Object *obj)
     QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
     ioc->readfd = -1;
     ioc->writefd = -1;
-    ioc->pid = -1;
+    ioc->pid = 0;
 }
 
 static void qio_channel_command_finalize(Object *obj)
@@ -239,6 +169,7 @@ static void qio_channel_command_finalize(Object *obj)
 #ifndef WIN32
         qio_channel_command_abort(ioc, NULL);
 #endif
+        g_spawn_close_pid(ioc->pid);
     }
 }
 
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] io/command: implement support for win32
  2022-09-02 11:18 [PATCH v2 0/3] io/command: implement portable spawn marcandre.lureau
  2022-09-02 11:18 ` [PATCH v2 1/3] io/command: use glib GSpawn, instead of open-coding fork/exec marcandre.lureau
@ 2022-09-02 11:18 ` marcandre.lureau
  2022-10-03 10:42   ` Daniel P. Berrangé
  2022-09-02 11:19 ` [PATCH v2 3/3] tests/unit: make test-io-channel-command work on win32 marcandre.lureau
  2022-10-03 10:02 ` [PATCH v2 0/3] io/command: implement portable spawn Marc-André Lureau
  3 siblings, 1 reply; 8+ messages in thread
From: marcandre.lureau @ 2022-09-02 11:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé, Marc-André Lureau

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This is a fairly straightforward implementation of the equivalent UNIX
version.

GLib uses _mkpipe() to setup the FDs. We take that for granted, and set
the underlying named-pipes to nonblocking. This is done by other
projects as well (found on github), but I am not confident this works
reliably (msdn SetNamedPipeHandleState documentation discourage this
usage).

Alternatively, we could setup the FDs ourself, and use UNIX sockets on
Windows, which can be used in blocking/non-blocking mode. I haven't
tried it, as I am not sure it is necessary.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 io/channel-command.c | 58 ++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/io/channel-command.c b/io/channel-command.c
index 35ba14c6a2..9bc292f3fd 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -26,7 +26,6 @@
 #include "qemu/sockets.h"
 #include "trace.h"
 
-#ifndef WIN32
 /**
  * qio_channel_command_new_pid:
  * @writefd: the FD connected to the command's stdin
@@ -60,7 +59,13 @@ qio_channel_command_new_pid(int writefd,
     ioc->writefd = writefd;
     ioc->pid = pid;
 
-    trace_qio_channel_command_new_pid(ioc, writefd, readfd, pid);
+    trace_qio_channel_command_new_pid(ioc, writefd, readfd,
+#ifdef WIN32
+                                      GetProcessId(pid)
+#else
+                                      pid
+#endif
+        );
     return ioc;
 }
 
@@ -89,18 +94,6 @@ qio_channel_command_new_spawn(const char *const argv[],
     return qio_channel_command_new_pid(stdinfd, stdoutfd, pid);
 }
 
-#else /* WIN32 */
-QIOChannelCommand *
-qio_channel_command_new_spawn(const char *const argv[],
-                              int flags,
-                              Error **errp)
-{
-    error_setg_errno(errp, ENOSYS,
-                     "Command spawn not supported on this platform");
-    return NULL;
-}
-#endif /* WIN32 */
-
 #ifndef WIN32
 static int qio_channel_command_abort(QIOChannelCommand *ioc,
                                      Error **errp)
@@ -143,6 +136,23 @@ static int qio_channel_command_abort(QIOChannelCommand *ioc,
 
     return 0;
 }
+#else
+static int qio_channel_command_abort(QIOChannelCommand *ioc,
+                                     Error **errp)
+{
+    DWORD ret;
+
+    TerminateProcess(ioc->pid, 0);
+    ret = WaitForSingleObject(ioc->pid, 1000);
+    if (ret != WAIT_OBJECT_0) {
+        error_setg(errp,
+                   "Process %llu refused to die",
+                   (unsigned long long)GetProcessId(ioc->pid));
+        return -1;
+    }
+
+    return 0;
+}
 #endif /* ! WIN32 */
 
 
@@ -166,9 +176,7 @@ static void qio_channel_command_finalize(Object *obj)
     }
     ioc->writefd = ioc->readfd = -1;
     if (ioc->pid > 0) {
-#ifndef WIN32
         qio_channel_command_abort(ioc, NULL);
-#endif
         g_spawn_close_pid(ioc->pid);
     }
 }
@@ -233,14 +241,20 @@ static int qio_channel_command_set_blocking(QIOChannel *ioc,
                                             bool enabled,
                                             Error **errp)
 {
+    QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
+
 #ifdef WIN32
-    /* command spawn is not supported on win32 */
-    g_assert_not_reached();
+    DWORD dwMode = PIPE_READMODE_BYTE | enabled ? PIPE_WAIT : PIPE_NOWAIT;
+
+    if ((cioc->writefd >= 0 && !SetNamedPipeHandleState((HANDLE)_get_osfhandle(cioc->writefd), &dwMode, NULL, NULL)) ||
+        (cioc->readfd >= 0 &&!SetNamedPipeHandleState((HANDLE)_get_osfhandle(cioc->readfd), &dwMode, NULL, NULL))) {
+        error_setg_win32(errp, GetLastError(), "Failed to set nonblocking");
+        return -1;
+    }
 #else
-    QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
 
-    if (!g_unix_set_fd_nonblocking(cioc->writefd, !enabled, NULL) ||
-        !g_unix_set_fd_nonblocking(cioc->readfd, !enabled, NULL)) {
+    if ((cioc->writefd >= 0 && !g_unix_set_fd_nonblocking(cioc->writefd, !enabled, NULL)) ||
+        (cioc->readfd >= 0 && !g_unix_set_fd_nonblocking(cioc->readfd, !enabled, NULL))) {
         error_setg_errno(errp, errno, "Failed to set FD nonblocking");
         return -1;
     }
@@ -281,6 +295,8 @@ static int qio_channel_command_close(QIOChannel *ioc,
                          (unsigned long long)cioc->pid);
         return -1;
     }
+#else
+    WaitForSingleObject(cioc->pid, INFINITE);
 #endif
 
     if (rv < 0) {
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] tests/unit: make test-io-channel-command work on win32
  2022-09-02 11:18 [PATCH v2 0/3] io/command: implement portable spawn marcandre.lureau
  2022-09-02 11:18 ` [PATCH v2 1/3] io/command: use glib GSpawn, instead of open-coding fork/exec marcandre.lureau
  2022-09-02 11:18 ` [PATCH v2 2/3] io/command: implement support for win32 marcandre.lureau
@ 2022-09-02 11:19 ` marcandre.lureau
  2022-10-03 10:46   ` Daniel P. Berrangé
  2022-10-03 10:02 ` [PATCH v2 0/3] io/command: implement portable spawn Marc-André Lureau
  3 siblings, 1 reply; 8+ messages in thread
From: marcandre.lureau @ 2022-09-02 11:19 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé, Marc-André Lureau

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
+
 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,
     };
 
     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



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/3] io/command: implement portable spawn
  2022-09-02 11:18 [PATCH v2 0/3] io/command: implement portable spawn marcandre.lureau
                   ` (2 preceding siblings ...)
  2022-09-02 11:19 ` [PATCH v2 3/3] tests/unit: make test-io-channel-command work on win32 marcandre.lureau
@ 2022-10-03 10:02 ` Marc-André Lureau
  3 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2022-10-03 10:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé

Hi

On Fri, Sep 2, 2022 at 3:19 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> A small series, based on earlier "[PATCH] io/command: implement portable spawn"
> to enable Windows support of command spawning in the io/ subsystem.
>

Daniel, please review.
thanks

> Marc-André Lureau (3):
>   io/command: use glib GSpawn, instead of open-coding fork/exec
>   io/command: implement support for win32
>   tests/unit: make test-io-channel-command work on win32
>
>  include/io/channel-command.h         |   2 +-
>  io/channel-command.c                 | 163 +++++++++------------------
>  tests/unit/test-io-channel-command.c |  31 +++--
>  3 files changed, 75 insertions(+), 121 deletions(-)
>
> --
> 2.37.2
>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/3] io/command: use glib GSpawn, instead of open-coding fork/exec
  2022-09-02 11:18 ` [PATCH v2 1/3] io/command: use glib GSpawn, instead of open-coding fork/exec marcandre.lureau
@ 2022-10-03 10:38   ` Daniel P. Berrangé
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2022-10-03 10:38 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel

On Fri, Sep 02, 2022 at 03:18:58PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Simplify qio_channel_command_new_spawn() with GSpawn API. This will
> allow to build for WIN32 in the following patches.

There a change in semantics here too. The current code only touches
stdin/stdout/stderr. Any other  FDs which do NOT have O_CLOEXEC set
will be inherited.  With the new code, all FDs except stdin/out/err
will be explicitly closed, because we don't set the flag
G_SPAWN_LEAVE_DESCRIPTORS_OPEN.

The only place we use QIOChannelCommand today is the migration
exec: protocol, and that is only declared to use stdin/stdout.

IOW, this is a good improvement, but we should call this out in
the commit message as a behaviour change.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/io/channel-command.h |   2 +-
>  io/channel-command.c         | 105 ++++++-----------------------------
>  2 files changed, 19 insertions(+), 88 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/3] io/command: implement support for win32
  2022-09-02 11:18 ` [PATCH v2 2/3] io/command: implement support for win32 marcandre.lureau
@ 2022-10-03 10:42   ` Daniel P. Berrangé
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2022-10-03 10:42 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel

On Fri, Sep 02, 2022 at 03:18:59PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This is a fairly straightforward implementation of the equivalent UNIX
> version.
> 
> GLib uses _mkpipe() to setup the FDs. We take that for granted, and set
> the underlying named-pipes to nonblocking. This is done by other
> projects as well (found on github), but I am not confident this works
> reliably (msdn SetNamedPipeHandleState documentation discourage this
> usage).

Well if other projects do this, and our unit tests pass, then that
is good enough for me, until someone complains with an example of
where it fails. It is less broken that today where we disallow
spawn entirely, so wont be a regression regardless :-)

> Alternatively, we could setup the FDs ourself, and use UNIX sockets on
> Windows, which can be used in blocking/non-blocking mode. I haven't
> tried it, as I am not sure it is necessary.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  io/channel-command.c | 58 ++++++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 21 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] tests/unit: make test-io-channel-command work on win32
  2022-09-02 11:19 ` [PATCH v2 3/3] tests/unit: make test-io-channel-command work on win32 marcandre.lureau
@ 2022-10-03 10:46   ` Daniel P. Berrangé
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2022-10-03 10:46 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: qemu-devel

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 :|



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-10-03 10:53 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-02 11:18 [PATCH v2 0/3] io/command: implement portable spawn marcandre.lureau
2022-09-02 11:18 ` [PATCH v2 1/3] io/command: use glib GSpawn, instead of open-coding fork/exec marcandre.lureau
2022-10-03 10:38   ` Daniel P. Berrangé
2022-09-02 11:18 ` [PATCH v2 2/3] io/command: implement support for win32 marcandre.lureau
2022-10-03 10:42   ` Daniel P. Berrangé
2022-09-02 11:19 ` [PATCH v2 3/3] tests/unit: make test-io-channel-command work on win32 marcandre.lureau
2022-10-03 10:46   ` Daniel P. Berrangé
2022-10-03 10:02 ` [PATCH v2 0/3] io/command: implement portable spawn Marc-André Lureau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).