[PULL 0/7] Win32 patches

[PULL 0/7] Win32 patches

[marcandre.lureau]

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

The following changes since commit ab44ea1059242ff2dbbde44e94468f6c6e5f87be:

  Merge tag 'pull-testing-gdbstub-plugins-gitdm-111022-1' of https://github.com/stsquad/qemu into staging (2022-10-11 15:31:27 -0400)

are available in the Git repository at:

  https://gitlab.com/marcandre.lureau/qemu.git tags/win32-pull-request

for you to fetch changes up to 76f5148c21b4543e62a6ad605ac4b44133421401:

  tests/unit: make test-io-channel-command work on win32 (2022-10-12 19:22:01 +0400)

----------------------------------------------------------------
win32-related misc patches

----------------------------------------------------------------

Marc-André Lureau (7):
  win32: set threads name
  osdep: make readv_writev() work with partial read/write
  util: make do_send_recv work with partial send/recv
  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 |  37 +++---
 util/iov.c                           |  10 +-
 util/osdep.c                         |  11 +-
 util/qemu-thread-win32.c             |  54 +++++++-
 7 files changed, 171 insertions(+), 140 deletions(-)

-- 
2.37.3

[PULL 1/7] win32: set threads name

[marcandre.lureau]

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

As described in:
https://learn.microsoft.com/en-us/visualstudio/debugger/how-to-set-a-thread-name-in-native-code?view=vs-2022

SetThreadDescription() is available since Windows 10, version 1607 and
in some versions only by "Run Time Dynamic Linking". Its declaration is
not yet in mingw, so we lookup the function the same way glib does.

Tested with Visual Studio Community 2022 debugger.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
---
 util/qemu-thread-win32.c | 54 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index a2d5a6e825..b9a467d7db 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -19,12 +19,39 @@
 
 static bool name_threads;
 
+typedef HRESULT (WINAPI *pSetThreadDescription) (HANDLE hThread,
+                                                 PCWSTR lpThreadDescription);
+static pSetThreadDescription SetThreadDescriptionFunc;
+static HMODULE kernel32_module;
+
+static bool load_set_thread_description(void)
+{
+    static gsize _init_once = 0;
+
+    if (g_once_init_enter(&_init_once)) {
+        kernel32_module = LoadLibrary("kernel32.dll");
+        if (kernel32_module) {
+            SetThreadDescriptionFunc =
+                (pSetThreadDescription)GetProcAddress(kernel32_module,
+                                                      "SetThreadDescription");
+            if (!SetThreadDescriptionFunc) {
+                FreeLibrary(kernel32_module);
+            }
+        }
+        g_once_init_leave(&_init_once, 1);
+    }
+
+    return !!SetThreadDescriptionFunc;
+}
+
 void qemu_thread_naming(bool enable)
 {
-    /* But note we don't actually name them on Windows yet */
     name_threads = enable;
 
-    fprintf(stderr, "qemu: thread naming not supported on this host\n");
+    if (enable && !load_set_thread_description()) {
+        fprintf(stderr, "qemu: thread naming not supported on this host\n");
+        name_threads = false;
+    }
 }
 
 static void error_exit(int err, const char *msg)
@@ -400,6 +427,25 @@ void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
+static bool set_thread_description(HANDLE h, const char *name)
+{
+    HRESULT hr;
+    g_autofree wchar_t *namew = NULL;
+
+    if (!load_set_thread_description()) {
+        return false;
+    }
+
+    namew = g_utf8_to_utf16(name, -1, NULL, NULL, NULL);
+    if (!namew) {
+        return false;
+    }
+
+    hr = SetThreadDescriptionFunc(h, namew);
+
+    return SUCCEEDED(hr);
+}
+
 void qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void *),
                        void *arg, int mode)
@@ -423,7 +469,11 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     if (!hThread) {
         error_exit(GetLastError(), __func__);
     }
+    if (name_threads && name && !set_thread_description(hThread, name)) {
+        fprintf(stderr, "qemu: failed to set thread description: %s\n", name);
+    }
     CloseHandle(hThread);
+
     thread->data = data;
 }
 
-- 
2.37.3

[PULL 2/7] 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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20221006113657.2656108-2-marcandre.lureau@redhat.com>
---
 util/osdep.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index 60fcbbaebe..746d5f7d71 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -538,18 +538,22 @@ int socket_init(void)
 
 
 #ifndef CONFIG_IOVEC
-/* helper function for iov_send_recv() */
 static ssize_t
 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 +566,7 @@ readv_writev(int fd, const struct iovec *iov, int iov_cnt, bool do_write)
             }
             break;
         }
+        off = 0;
         i++;
     }
     return ret;
-- 
2.37.3

[PULL 3/7] util: make do_send_recv work with partial send/recv

[marcandre.lureau]

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

According to msdn documentation and Linux man pages, send() should try
to send as much as possible in blocking mode, while recv() may return
earlier with a smaller available amount, we should try to continue
send/recv from there.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20221006113657.2656108-3-marcandre.lureau@redhat.com>
---
 util/iov.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/util/iov.c b/util/iov.c
index 22d6996cce..b4be580022 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -111,12 +111,17 @@ do_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, bool do_send)
     /*XXX Note: windows has WSASend() and WSARecv() */
     unsigned i = 0;
     ssize_t ret = 0;
+    ssize_t off = 0;
     while (i < iov_cnt) {
         ssize_t r = do_send
-            ? send(sockfd, iov[i].iov_base, iov[i].iov_len, 0)
-            : recv(sockfd, iov[i].iov_base, iov[i].iov_len, 0);
+            ? send(sockfd, iov[i].iov_base + off, iov[i].iov_len - off, 0)
+            : recv(sockfd, iov[i].iov_base + off, iov[i].iov_len - off, 0);
         if (r > 0) {
             ret += r;
+            off += r;
+            if (off < iov[i].iov_len) {
+                continue;
+            }
         } else if (!r) {
             break;
         } else if (errno == EINTR) {
@@ -129,6 +134,7 @@ do_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, bool do_send)
             }
             break;
         }
+        off = 0;
         i++;
     }
     return ret;
-- 
2.37.3

[PULL 4/7] 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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20221006113657.2656108-4-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

[PULL 5/7] 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>
Message-Id: <20221006113657.2656108-5-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

[PULL 6/7] 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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20221006113657.2656108-6-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

[PULL 7/7] 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.

Here are the changes:
- drop tests/ from fifo/pipe path, to avoid directory issues
- use g_find_program() to lookup the socat executable (otherwise we
would need to change ChanneCommand to use G_SPAWN_SEARCH_PATH, and deal
with missing socat differently)
- skip the "echo" test when socat is missing as well

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20221006113657.2656108-7-marcandre.lureau@redhat.com>
---
 tests/unit/test-io-channel-command.c | 37 ++++++++++++++--------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
index aa09c559cd..7eee939c07 100644
--- a/tests/unit/test-io-channel-command.c
+++ b/tests/unit/test-io-channel-command.c
@@ -24,29 +24,30 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 
-#ifndef WIN32
+#define TEST_FIFO "test-io-channel-command.fifo"
+
+#define SOCAT_SRC "PIPE:" TEST_FIFO ",wronly"
+#define SOCAT_DST "PIPE:" TEST_FIFO ",rdonly"
+
+static char *socat = NULL;
+
 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) {
-        g_test_skip("socat is missing");
+    if (!socat) {
+        g_test_skip("socat is not found in PATH");
         return;
     }
-    if (mkfifo(TEST_FIFO, 0600) < 0) {
-        abort();
-    }
+
+    unlink(TEST_FIFO);
     src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
                                                     O_WRONLY,
                                                     &error_abort));
@@ -81,11 +82,12 @@ 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) {
-        return; /* Pretend success if socat is not present */
+    if (!socat) {
+        g_test_skip("socat is not found in PATH");
+        return;
     }
 
     ioc = QIO_CHANNEL(qio_channel_command_new_spawn(socatargv,
@@ -108,7 +110,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 +117,8 @@ int main(int argc, char **argv)
 
     g_test_init(&argc, &argv, NULL);
 
-#ifndef WIN32
+    socat = g_find_program_in_path("socat");
+
     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 +127,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.3

Re: [PULL 0/7] Win32 patches

[Stefan Hajnoczi]

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

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL 5/7] io/command: use glib GSpawn, instead of open-coding fork/exec

[Stefan Weil via]

[-- Attachment #1.1.1: Type: text/plain, Size: 7024 bytes --]

Am 12.10.22 um 18:04 schrieb marcandre.lureau@redhat.com:

> 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>
> Message-Id: <20221006113657.2656108-5-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;


GPid is a pointer for Windows now. Therefore code like pid > 0 is no 
longer valid. As the new initial value is no longer -1, but 0 now, this 
patch might fix the code for Windows:

diff --git a/io/channel-command.c b/io/channel-command.c
index 74516252ba..8850a374f1 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -175,7 +175,7 @@ static void qio_channel_command_finalize(Object *obj)
          close(ioc->writefd);
      }
      ioc->writefd = ioc->readfd = -1;
-    if (ioc->pid > 0) {
+    if (ioc->pid != 0) {
          qio_channel_command_abort(ioc, NULL);
          g_spawn_close_pid(ioc->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);
>       }
>   }
>   

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6511 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PULL 5/7] io/command: use glib GSpawn, instead of open-coding fork/exec

[Marc-André Lureau]

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

Hi Stefan

On Sat, Oct 29, 2022 at 11:12 PM Stefan Weil <sw@weilnetz.de> wrote:

> Am 12.10.22 um 18:04 schrieb marcandre.lureau@redhat.com:
>
> > 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>
> > Message-Id: <20221006113657.2656108-5-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;
>
>
> GPid is a pointer for Windows now. Therefore code like pid > 0 is no
> longer valid. As the new initial value is no longer -1, but 0 now, this
> patch might fix the code for Windows:
>
> diff --git a/io/channel-command.c b/io/channel-command.c
> index 74516252ba..8850a374f1 100644
> --- a/io/channel-command.c
> +++ b/io/channel-command.c
> @@ -175,7 +175,7 @@ static void qio_channel_command_finalize(Object *obj)
>           close(ioc->writefd);
>       }
>       ioc->writefd = ioc->readfd = -1;
> -    if (ioc->pid > 0) {
> +    if (ioc->pid != 0) {
>           qio_channel_command_abort(ioc, NULL);
>           g_spawn_close_pid(ioc->pid);
>       }
>

Hmm, GPid is an "int" on unix, "void*" on windows. I think the current code
should work fine. Do you see really an issue? Your change looks correct to
me too, can you send a proper patch with details?

thanks


>
>
> >   };
> >
> >
> > 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);
> >       }
> >   }
> >
>

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