[PATCH v3 0/5] io/command: implement portable spawn

[PATCH v3 0/5] io/command: implement portable spawn

[marcandre.lureau]

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

Hi,

This enables support of command spawning in the io/ subsystem on Windows.

v3:
 - actually make it work :) (it seems I didn't test correctly earlier attempts)
 - add a patch for readv_writev() and partial IOs
 - add a patch to avoid some potential race in echo test
 - drop usage of PIPE_NOWAIT which was broken

Marc-André Lureau (5):
  osdep: make readv_writev() work with partial read/write
  tests/channel-helper: set blocking in main thread
  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         |   5 +-
 io/channel-command.c                 | 185 +++++++++++----------------
 tests/unit/io-channel-helpers.c      |   9 +-
 tests/unit/test-io-channel-command.c |  32 ++---
 util/osdep.c                         |  10 +-
 tests/unit/meson.build               |   2 +-
 6 files changed, 107 insertions(+), 136 deletions(-)

-- 
2.37.3

[PATCH v3 1/5] osdep: make readv_writev() work with partial read/write

[marcandre.lureau]

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

With a pipe or other reasons, read/write may return less than the
requested bytes. This happens with the test-io-channel-command test on
Windows. glib spawn code uses a binary pipe of 4096 bytes, and the first
read returns that much (although more are requested), for some unclear
reason...

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/osdep.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index 60fcbbaebe..b85715a743 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -544,12 +544,17 @@ readv_writev(int fd, const struct iovec *iov, int iov_cnt, bool do_write)
 {
     unsigned i = 0;
     ssize_t ret = 0;
+    ssize_t off = 0;
     while (i < iov_cnt) {
         ssize_t r = do_write
-            ? write(fd, iov[i].iov_base, iov[i].iov_len)
-            : read(fd, iov[i].iov_base, iov[i].iov_len);
+            ? write(fd, iov[i].iov_base + off, iov[i].iov_len - off)
+            : read(fd, iov[i].iov_base + off, iov[i].iov_len - off);
         if (r > 0) {
             ret += r;
+            off += r;
+            if (off < iov[i].iov_len) {
+                continue;
+            }
         } else if (!r) {
             break;
         } else if (errno == EINTR) {
@@ -562,6 +567,7 @@ readv_writev(int fd, const struct iovec *iov, int iov_cnt, bool do_write)
             }
             break;
         }
+        off = 0;
         i++;
     }
     return ret;
-- 
2.37.3

[PATCH v3 2/5] tests/channel-helper: set blocking in main thread

[marcandre.lureau]

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

The /io/channel/command/echo tests run the reader side and the writer
side with the same underlying command channel. Setting the blocking mode
of the fd/handles while the other end is already reading/writing may
create issues (deadlock in win32 when earlier attempt of this series
were using SetNamedPipeHandleState). Let's just do it before spawning
the threads to avoid further concurrency issues.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/io-channel-helpers.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/unit/io-channel-helpers.c b/tests/unit/io-channel-helpers.c
index ff156ed3c4..c0799c21c2 100644
--- a/tests/unit/io-channel-helpers.c
+++ b/tests/unit/io-channel-helpers.c
@@ -25,7 +25,6 @@
 struct QIOChannelTest {
     QIOChannel *src;
     QIOChannel *dst;
-    bool blocking;
     size_t len;
     size_t niov;
     char *input;
@@ -42,8 +41,6 @@ static gpointer test_io_thread_writer(gpointer opaque)
 {
     QIOChannelTest *data = opaque;
 
-    qio_channel_set_blocking(data->src, data->blocking, NULL);
-
     qio_channel_writev_all(data->src,
                            data->inputv,
                            data->niov,
@@ -58,8 +55,6 @@ static gpointer test_io_thread_reader(gpointer opaque)
 {
     QIOChannelTest *data = opaque;
 
-    qio_channel_set_blocking(data->dst, data->blocking, NULL);
-
     qio_channel_readv_all(data->dst,
                           data->outputv,
                           data->niov,
@@ -113,7 +108,9 @@ void qio_channel_test_run_threads(QIOChannelTest *test,
 
     test->src = src;
     test->dst = dst;
-    test->blocking = blocking;
+
+    qio_channel_set_blocking(test->dst, blocking, NULL);
+    qio_channel_set_blocking(test->src, blocking, NULL);
 
     reader = g_thread_new("reader",
                           test_io_thread_reader,
-- 
2.37.3

[PATCH v3 3/5] io/command: use glib GSpawn, instead of open-coding fork/exec

[marcandre.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.

As pointed out by Daniel Berrangé: there is 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.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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..f84d1f03a0 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 | G_SPAWN_DO_NOT_REAP_CHILD;
+    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.3

[PATCH v3 4/5] io/command: implement support for win32

[marcandre.lureau]

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

The initial implementation was changing the pipe state created by GLib
to PIPE_NOWAIT, but it turns out it doesn't work (read/write returns an
error). Since reading may return less than the requested amount, it
seems to be non-blocking already. However, the IO operation may block
until the FD is ready, I can't find good sources of information, to be
safe we can just poll for readiness before.

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>
---
 include/io/channel-command.h |  3 ++
 io/channel-command.c         | 80 ++++++++++++++++++++++++++----------
 2 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/include/io/channel-command.h b/include/io/channel-command.h
index 8dc58273c0..98934e6d9e 100644
--- a/include/io/channel-command.h
+++ b/include/io/channel-command.h
@@ -42,6 +42,9 @@ struct QIOChannelCommand {
     int writefd;
     int readfd;
     GPid pid;
+#ifdef WIN32
+    bool blocking;
+#endif
 };
 
 
diff --git a/io/channel-command.c b/io/channel-command.c
index f84d1f03a0..74516252ba 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,13 +176,27 @@ 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);
     }
 }
 
+#ifdef WIN32
+static bool win32_fd_poll(int fd, gushort events)
+{
+    GPollFD pfd = { .fd = _get_osfhandle(fd), .events = events };
+    int res;
+
+    do {
+        res = g_poll(&pfd, 1, 0);
+    } while (res < 0 && errno == EINTR);
+    if (res == 0) {
+        return false;
+    }
+
+    return true;
+}
+#endif
 
 static ssize_t qio_channel_command_readv(QIOChannel *ioc,
                                          const struct iovec *iov,
@@ -184,6 +208,12 @@ static ssize_t qio_channel_command_readv(QIOChannel *ioc,
     QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
     ssize_t ret;
 
+#ifdef WIN32
+    if (!cioc->blocking && !win32_fd_poll(cioc->readfd, G_IO_IN)) {
+        return QIO_CHANNEL_ERR_BLOCK;
+    }
+#endif
+
  retry:
     ret = readv(cioc->readfd, iov, niov);
     if (ret < 0) {
@@ -213,6 +243,12 @@ static ssize_t qio_channel_command_writev(QIOChannel *ioc,
     QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
     ssize_t ret;
 
+#ifdef WIN32
+    if (!cioc->blocking && !win32_fd_poll(cioc->writefd, G_IO_OUT)) {
+        return QIO_CHANNEL_ERR_BLOCK;
+    }
+#endif
+
  retry:
     ret = writev(cioc->writefd, iov, niov);
     if (ret <= 0) {
@@ -233,14 +269,14 @@ 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();
+    cioc->blocking = enabled;
 #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 +317,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.3

[PATCH v3 5/5] tests/unit: make test-io-channel-command work on win32

[marcandre.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 | 32 ++++++++++++----------------
 tests/unit/meson.build               |  2 +-
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
index aa09c559cd..be98c3452a 100644
--- a/tests/unit/test-io-channel-command.c
+++ b/tests/unit/test-io-channel-command.c
@@ -24,29 +24,27 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 
-#ifndef WIN32
+#define TEST_PATH "test-io-channel-command.fifo"
+
+#define SOCAT_SRC "PIPE:" TEST_PATH ",wronly"
+#define SOCAT_DST "PIPE:" TEST_PATH ",rdonly"
+
 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,
+        g_getenv("SOCAT"), "-", SOCAT_SRC, NULL,
     };
     const char *dstargv[] = {
-        "/bin/socat", dstfifo, "-", NULL,
+        g_getenv("SOCAT"), SOCAT_DST, "-", NULL,
     };
 
-    unlink(TEST_FIFO);
-    if (access("/bin/socat", X_OK) < 0) {
+    unlink(TEST_PATH);
+    if (!g_file_test(g_getenv("SOCAT"), G_FILE_TEST_IS_EXECUTABLE)) {
         g_test_skip("socat is missing");
         return;
     }
-    if (mkfifo(TEST_FIFO, 0600) < 0) {
-        abort();
-    }
     src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
                                                     O_WRONLY,
                                                     &error_abort));
@@ -61,7 +59,7 @@ static void test_io_channel_command_fifo(bool async)
     object_unref(OBJECT(src));
     object_unref(OBJECT(dst));
 
-    unlink(TEST_FIFO);
+    unlink(TEST_PATH);
 }
 
 
@@ -81,11 +79,12 @@ static void test_io_channel_command_echo(bool async)
     QIOChannel *ioc;
     QIOChannelTest *test;
     const char *socatargv[] = {
-        "/bin/socat", "-", "-", NULL,
+        g_getenv("SOCAT"), "-", "-", NULL,
     };
 
-    if (access("/bin/socat", X_OK) < 0) {
-        return; /* Pretend success if socat is not present */
+    if (!g_file_test(g_getenv("SOCAT"), G_FILE_TEST_IS_EXECUTABLE)) {
+        g_test_skip("socat is missing");
+        return;
     }
 
     ioc = QIO_CHANNEL(qio_channel_command_new_spawn(socatargv,
@@ -108,7 +107,6 @@ static void test_io_channel_command_echo_sync(void)
 {
     test_io_channel_command_echo(false);
 }
-#endif
 
 int main(int argc, char **argv)
 {
@@ -116,7 +114,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",
@@ -125,7 +122,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();
 }
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index b497a41378..42e8218ac2 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -1,4 +1,3 @@
-
 testblock = declare_dependency(dependencies: [block], sources: 'iothread.c')
 
 tests = {
@@ -164,6 +163,7 @@ endif
 test_env = environment()
 test_env.set('G_TEST_SRCDIR', meson.current_source_dir())
 test_env.set('G_TEST_BUILDDIR', meson.current_build_dir())
+test_env.set('SOCAT', find_program('socat').full_path())
 
 slow_tests = {
   'test-crypto-tlscredsx509': 45,
-- 
2.37.3

Re: [PATCH v3 2/5] tests/channel-helper: set blocking in main thread

[Daniel P. Berrangé]

On Thu, Oct 06, 2022 at 12:12:19PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The /io/channel/command/echo tests run the reader side and the writer
> side with the same underlying command channel. Setting the blocking mode
> of the fd/handles while the other end is already reading/writing may
> create issues (deadlock in win32 when earlier attempt of this series
> were using SetNamedPipeHandleState). Let's just do it before spawning
> the threads to avoid further concurrency issues.

Curious, makes me wonder why I did this in the threads in the first
place...

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/unit/io-channel-helpers.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

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


> 
> diff --git a/tests/unit/io-channel-helpers.c b/tests/unit/io-channel-helpers.c
> index ff156ed3c4..c0799c21c2 100644
> --- a/tests/unit/io-channel-helpers.c
> +++ b/tests/unit/io-channel-helpers.c
> @@ -25,7 +25,6 @@
>  struct QIOChannelTest {
>      QIOChannel *src;
>      QIOChannel *dst;
> -    bool blocking;
>      size_t len;
>      size_t niov;
>      char *input;
> @@ -42,8 +41,6 @@ static gpointer test_io_thread_writer(gpointer opaque)
>  {
>      QIOChannelTest *data = opaque;
>  
> -    qio_channel_set_blocking(data->src, data->blocking, NULL);
> -
>      qio_channel_writev_all(data->src,
>                             data->inputv,
>                             data->niov,
> @@ -58,8 +55,6 @@ static gpointer test_io_thread_reader(gpointer opaque)
>  {
>      QIOChannelTest *data = opaque;
>  
> -    qio_channel_set_blocking(data->dst, data->blocking, NULL);
> -
>      qio_channel_readv_all(data->dst,
>                            data->outputv,
>                            data->niov,
> @@ -113,7 +108,9 @@ void qio_channel_test_run_threads(QIOChannelTest *test,
>  
>      test->src = src;
>      test->dst = dst;
> -    test->blocking = blocking;
> +
> +    qio_channel_set_blocking(test->dst, blocking, NULL);
> +    qio_channel_set_blocking(test->src, blocking, NULL);
>  
>      reader = g_thread_new("reader",
>                            test_io_thread_reader,
> -- 
> 2.37.3
> 

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

Re: [PATCH v3 1/5] osdep: make readv_writev() work with partial read/write

[Daniel P. Berrangé]

On Thu, Oct 06, 2022 at 12:12:18PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> With a pipe or other reasons, read/write may return less than the
> requested bytes. This happens with the test-io-channel-command test on
> Windows. glib spawn code uses a binary pipe of 4096 bytes, and the first
> read returns that much (although more are requested), for some unclear
> reason...

Wow, lucky this function isn't too widely used historically!

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  util/osdep.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

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

> 
> diff --git a/util/osdep.c b/util/osdep.c
> index 60fcbbaebe..b85715a743 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c

There's a comment just above here that says

   helper function for iov_send_recv

which is not true, as iov_send_recv just calls send/recv instead,
again in a loop with the exact same logic flaw.  Care to fix the
comment and also fix iov_send_recv in the same way ?

> @@ -544,12 +544,17 @@ readv_writev(int fd, const struct iovec *iov, int iov_cnt, bool do_write)
>  {
>      unsigned i = 0;
>      ssize_t ret = 0;
> +    ssize_t off = 0;
>      while (i < iov_cnt) {
>          ssize_t r = do_write
> -            ? write(fd, iov[i].iov_base, iov[i].iov_len)
> -            : read(fd, iov[i].iov_base, iov[i].iov_len);
> +            ? write(fd, iov[i].iov_base + off, iov[i].iov_len - off)
> +            : read(fd, iov[i].iov_base + off, iov[i].iov_len - off);
>          if (r > 0) {
>              ret += r;
> +            off += r;
> +            if (off < iov[i].iov_len) {
> +                continue;
> +            }
>          } else if (!r) {
>              break;
>          } else if (errno == EINTR) {
> @@ -562,6 +567,7 @@ readv_writev(int fd, const struct iovec *iov, int iov_cnt, bool do_write)
>              }
>              break;
>          }
> +        off = 0;
>          i++;
>      }
>      return ret;
> -- 
> 2.37.3
> 

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

Re: [PATCH v3 4/5] io/command: implement support for win32

[Daniel P. Berrangé]

On Thu, Oct 06, 2022 at 12:12:21PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The initial implementation was changing the pipe state created by GLib
> to PIPE_NOWAIT, but it turns out it doesn't work (read/write returns an
> error). Since reading may return less than the requested amount, it
> seems to be non-blocking already. However, the IO operation may block
> until the FD is ready, I can't find good sources of information, to be
> safe we can just poll for readiness before.

The downside is that we double the number of syscalls for each blocking
I/O. Doesn't matter if we do actually block, but if there was data
already pending, this is extra overhead.

But then this code never worked on Windows, so users will likely not
have performance expectations at this time.

> 
> 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>
> ---
>  include/io/channel-command.h |  3 ++
>  io/channel-command.c         | 80 ++++++++++++++++++++++++++----------
>  2 files changed, 62 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 :|

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

[Daniel P. Berrangé]

On Thu, Oct 06, 2022 at 12:12:22PM +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 | 32 ++++++++++++----------------
>  tests/unit/meson.build               |  2 +-
>  2 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
> index aa09c559cd..be98c3452a 100644
> --- a/tests/unit/test-io-channel-command.c
> +++ b/tests/unit/test-io-channel-command.c
> @@ -24,29 +24,27 @@
>  #include "qapi/error.h"
>  #include "qemu/module.h"
>  
> -#ifndef WIN32
> +#define TEST_PATH "test-io-channel-command.fifo"
> +
> +#define SOCAT_SRC "PIPE:" TEST_PATH ",wronly"
> +#define SOCAT_DST "PIPE:" TEST_PATH ",rdonly"
> +
>  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,
> +        g_getenv("SOCAT"), "-", SOCAT_SRC, NULL,

Please don't rely on env variables, as it complicates the ability to
invoke the test directly, without the meson harness. Either pass the
path from meson at compile time in config-host.h, or make this code
use an unqualified path, so it honours $PATH at runtime.

>      };
>      const char *dstargv[] = {
> -        "/bin/socat", dstfifo, "-", NULL,
> +        g_getenv("SOCAT"), SOCAT_DST, "-", NULL,
>      };
>  
> -    unlink(TEST_FIFO);
> -    if (access("/bin/socat", X_OK) < 0) {
> +    unlink(TEST_PATH);
> +    if (!g_file_test(g_getenv("SOCAT"), G_FILE_TEST_IS_EXECUTABLE)) {
>          g_test_skip("socat is missing");
>          return;
>      }
> -    if (mkfifo(TEST_FIFO, 0600) < 0) {
> -        abort();
> -    }
>      src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
>                                                      O_WRONLY,
>                                                      &error_abort));
> @@ -61,7 +59,7 @@ static void test_io_channel_command_fifo(bool async)
>      object_unref(OBJECT(src));
>      object_unref(OBJECT(dst));
>  
> -    unlink(TEST_FIFO);
> +    unlink(TEST_PATH);
>  }
>  
>  
> @@ -81,11 +79,12 @@ static void test_io_channel_command_echo(bool async)
>      QIOChannel *ioc;
>      QIOChannelTest *test;
>      const char *socatargv[] = {
> -        "/bin/socat", "-", "-", NULL,
> +        g_getenv("SOCAT"), "-", "-", NULL,
>      };
>  
> -    if (access("/bin/socat", X_OK) < 0) {
> -        return; /* Pretend success if socat is not present */
> +    if (!g_file_test(g_getenv("SOCAT"), G_FILE_TEST_IS_EXECUTABLE)) {
> +        g_test_skip("socat is missing");
> +        return;
>      }
>  
>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(socatargv,
> @@ -108,7 +107,6 @@ static void test_io_channel_command_echo_sync(void)
>  {
>      test_io_channel_command_echo(false);
>  }
> -#endif
>  
>  int main(int argc, char **argv)
>  {
> @@ -116,7 +114,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",
> @@ -125,7 +122,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();
>  }
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index b497a41378..42e8218ac2 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -1,4 +1,3 @@
> -
>  testblock = declare_dependency(dependencies: [block], sources: 'iothread.c')
>  
>  tests = {

Spurious line deletion.


> @@ -164,6 +163,7 @@ endif
>  test_env = environment()
>  test_env.set('G_TEST_SRCDIR', meson.current_source_dir())
>  test_env.set('G_TEST_BUILDDIR', meson.current_build_dir())
> +test_env.set('SOCAT', find_program('socat').full_path())
>  
>  slow_tests = {
>    'test-crypto-tlscredsx509': 45,
> -- 
> 2.37.3
> 

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

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

[Marc-André Lureau]

Hi

On Thu, Oct 6, 2022 at 12:42 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Oct 06, 2022 at 12:12:22PM +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 | 32 ++++++++++++----------------
> >  tests/unit/meson.build               |  2 +-
> >  2 files changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
> > index aa09c559cd..be98c3452a 100644
> > --- a/tests/unit/test-io-channel-command.c
> > +++ b/tests/unit/test-io-channel-command.c
> > @@ -24,29 +24,27 @@
> >  #include "qapi/error.h"
> >  #include "qemu/module.h"
> >
> > -#ifndef WIN32
> > +#define TEST_PATH "test-io-channel-command.fifo"
> > +
> > +#define SOCAT_SRC "PIPE:" TEST_PATH ",wronly"
> > +#define SOCAT_DST "PIPE:" TEST_PATH ",rdonly"
> > +
> >  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,
> > +        g_getenv("SOCAT"), "-", SOCAT_SRC, NULL,
>
> Please don't rely on env variables, as it complicates the ability to
> invoke the test directly, without the meson harness. Either pass the
> path from meson at compile time in config-host.h, or make this code
> use an unqualified path, so it honours $PATH at runtime.

I tried to pass it through config-host.h, but I dont see a way to
escape the \ is the paths.

Unqualified path doesn't work under msys2.

>
> >      };
> >      const char *dstargv[] = {
> > -        "/bin/socat", dstfifo, "-", NULL,
> > +        g_getenv("SOCAT"), SOCAT_DST, "-", NULL,
> >      };
> >
> > -    unlink(TEST_FIFO);
> > -    if (access("/bin/socat", X_OK) < 0) {
> > +    unlink(TEST_PATH);
> > +    if (!g_file_test(g_getenv("SOCAT"), G_FILE_TEST_IS_EXECUTABLE)) {
> >          g_test_skip("socat is missing");
> >          return;
> >      }
> > -    if (mkfifo(TEST_FIFO, 0600) < 0) {
> > -        abort();
> > -    }
> >      src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
> >                                                      O_WRONLY,
> >                                                      &error_abort));
> > @@ -61,7 +59,7 @@ static void test_io_channel_command_fifo(bool async)
> >      object_unref(OBJECT(src));
> >      object_unref(OBJECT(dst));
> >
> > -    unlink(TEST_FIFO);
> > +    unlink(TEST_PATH);
> >  }
> >
> >
> > @@ -81,11 +79,12 @@ static void test_io_channel_command_echo(bool async)
> >      QIOChannel *ioc;
> >      QIOChannelTest *test;
> >      const char *socatargv[] = {
> > -        "/bin/socat", "-", "-", NULL,
> > +        g_getenv("SOCAT"), "-", "-", NULL,
> >      };
> >
> > -    if (access("/bin/socat", X_OK) < 0) {
> > -        return; /* Pretend success if socat is not present */
> > +    if (!g_file_test(g_getenv("SOCAT"), G_FILE_TEST_IS_EXECUTABLE)) {
> > +        g_test_skip("socat is missing");
> > +        return;
> >      }
> >
> >      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(socatargv,
> > @@ -108,7 +107,6 @@ static void test_io_channel_command_echo_sync(void)
> >  {
> >      test_io_channel_command_echo(false);
> >  }
> > -#endif
> >
> >  int main(int argc, char **argv)
> >  {
> > @@ -116,7 +114,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",
> > @@ -125,7 +122,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();
> >  }
> > diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> > index b497a41378..42e8218ac2 100644
> > --- a/tests/unit/meson.build
> > +++ b/tests/unit/meson.build
> > @@ -1,4 +1,3 @@
> > -
> >  testblock = declare_dependency(dependencies: [block], sources: 'iothread.c')
> >
> >  tests = {
>
> Spurious line deletion.
>
>
> > @@ -164,6 +163,7 @@ endif
> >  test_env = environment()
> >  test_env.set('G_TEST_SRCDIR', meson.current_source_dir())
> >  test_env.set('G_TEST_BUILDDIR', meson.current_build_dir())
> > +test_env.set('SOCAT', find_program('socat').full_path())
> >
> >  slow_tests = {
> >    'test-crypto-tlscredsx509': 45,
> > --
> > 2.37.3
> >
>
> 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 :|
>

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

[Daniel P. Berrangé]

On Thu, Oct 06, 2022 at 12:46:17PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Oct 6, 2022 at 12:42 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Oct 06, 2022 at 12:12:22PM +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 | 32 ++++++++++++----------------
> > >  tests/unit/meson.build               |  2 +-
> > >  2 files changed, 15 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
> > > index aa09c559cd..be98c3452a 100644
> > > --- a/tests/unit/test-io-channel-command.c
> > > +++ b/tests/unit/test-io-channel-command.c
> > > @@ -24,29 +24,27 @@
> > >  #include "qapi/error.h"
> > >  #include "qemu/module.h"
> > >
> > > -#ifndef WIN32
> > > +#define TEST_PATH "test-io-channel-command.fifo"
> > > +
> > > +#define SOCAT_SRC "PIPE:" TEST_PATH ",wronly"
> > > +#define SOCAT_DST "PIPE:" TEST_PATH ",rdonly"
> > > +
> > >  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,
> > > +        g_getenv("SOCAT"), "-", SOCAT_SRC, NULL,
> >
> > Please don't rely on env variables, as it complicates the ability to
> > invoke the test directly, without the meson harness. Either pass the
> > path from meson at compile time in config-host.h, or make this code
> > use an unqualified path, so it honours $PATH at runtime.
> 
> I tried to pass it through config-host.h, but I dont see a way to
> escape the \ is the paths.

This must be possible, as we have lots of strings in config-host.h
that are paths - eg many CONFIG_QEMU_xxxDIR variables


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

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]

Hi

On Thu, Oct 6, 2022 at 2:14 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Oct 06, 2022 at 12:46:17PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Oct 6, 2022 at 12:42 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> > >
> > > On Thu, Oct 06, 2022 at 12:12:22PM +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 | 32
> ++++++++++++----------------
> > > >  tests/unit/meson.build               |  2 +-
> > > >  2 files changed, 15 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/tests/unit/test-io-channel-command.c
> b/tests/unit/test-io-channel-command.c
> > > > index aa09c559cd..be98c3452a 100644
> > > > --- a/tests/unit/test-io-channel-command.c
> > > > +++ b/tests/unit/test-io-channel-command.c
> > > > @@ -24,29 +24,27 @@
> > > >  #include "qapi/error.h"
> > > >  #include "qemu/module.h"
> > > >
> > > > -#ifndef WIN32
> > > > +#define TEST_PATH "test-io-channel-command.fifo"
> > > > +
> > > > +#define SOCAT_SRC "PIPE:" TEST_PATH ",wronly"
> > > > +#define SOCAT_DST "PIPE:" TEST_PATH ",rdonly"
> > > > +
> > > >  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,
> > > > +        g_getenv("SOCAT"), "-", SOCAT_SRC, NULL,
> > >
> > > Please don't rely on env variables, as it complicates the ability to
> > > invoke the test directly, without the meson harness. Either pass the
> > > path from meson at compile time in config-host.h, or make this code
> > > use an unqualified path, so it honours $PATH at runtime.
> >
> > I tried to pass it through config-host.h, but I dont see a way to
> > escape the \ is the paths.
>
> This must be possible, as we have lots of strings in config-host.h
> that are paths - eg many CONFIG_QEMU_xxxDIR variables
>

Those paths, generated by meson I believe, use /-dir separators. But
find_program() returns \-seperated paths.. Maybe the solution is to replace
\ with / ? (hopefully the path is not quoted already...)

There is a proposal for to_quoted():
https://github.com/mesonbuild/meson/issues/10417

I found another solution, using g_find_program_in_path(). See v3.

thanks

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4570 bytes --]