[PATCH 00/10] Various win32 fixes & new 'get-win32-socket' QMP command

[PATCH 00/10] Various win32 fixes & new 'get-win32-socket' QMP command

[marcandre.lureau]

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

Hi,

The following series first fixes a few tests on win32. The second part focuses
on 'add_client' support, by limiting its scope to sockets and adding win32
support. Finally, it enables vnc-display test on win32, to exercise the new code
paths and demonstrate its usage.

'get-win32-socket' can be used to write more robusts code & tests using sockets
on Windows, and will be used by a follow up series testing dbus display support.

Marc-André Lureau (10):
  ccid-card-emulated: fix cast warning/error
  tests: fix path separator, use g_build_filename()
  tests: fix test-io-channel-command on win32
  tests/docker: fix a win32 error due to portability
  tests/readconfig: spice doesn't support unix socket on windows yet
  osdep: implement qemu_socketpair() for win32
  qmp: 'add_client' actually expects sockets
  qmp: add 'get-win32-socket'
  libqtest: make qtest_qmp_add_client work on win32
  qtest: enable vnc-display test on win32

 qapi/misc.json                       |  32 ++++++++
 include/qemu/sockets.h               |   2 -
 tests/qtest/libqtest.h               |   5 +-
 hw/usb/ccid-card-emulated.c          |   2 +-
 monitor/misc.c                       |  74 ++++++++++++++----
 monitor/qmp-cmds.c                   |  19 +++--
 tests/qtest/libqtest.c               |  16 +++-
 tests/qtest/readconfig-test.c        |   6 +-
 tests/qtest/vnc-display-test.c       |   5 --
 tests/unit/test-io-channel-command.c |   8 +-
 util/oslib-win32.c                   | 110 +++++++++++++++++++++++++++
 tests/docker/docker.py               |   6 +-
 12 files changed, 245 insertions(+), 40 deletions(-)

-- 
2.39.0

[PATCH 01/10] ccid-card-emulated: fix cast warning/error

[marcandre.lureau]

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

../hw/usb/ccid-card-emulated.c: In function 'handle_apdu_thread':
../hw/usb/ccid-card-emulated.c:251:24: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  251 |                 assert((unsigned long)event > 1000);

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/usb/ccid-card-emulated.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index ee41a81801..c328660075 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -248,7 +248,7 @@ static void *handle_apdu_thread(void* arg)
         WITH_QEMU_LOCK_GUARD(&card->vreader_mutex) {
             while (!QSIMPLEQ_EMPTY(&card->guest_apdu_list)) {
                 event = QSIMPLEQ_FIRST(&card->guest_apdu_list);
-                assert((unsigned long)event > 1000);
+                assert(event != NULL);
                 QSIMPLEQ_REMOVE_HEAD(&card->guest_apdu_list, entry);
                 if (event->p.data.type != EMUL_GUEST_APDU) {
                     DPRINTF(card, 1, "unexpected message in handle_apdu_thread\n");
-- 
2.39.0

Re: [PATCH 01/10] ccid-card-emulated: fix cast warning/error

[Thomas Huth]

On 03/01/2023 12.08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../hw/usb/ccid-card-emulated.c: In function 'handle_apdu_thread':
> ../hw/usb/ccid-card-emulated.c:251:24: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>    251 |                 assert((unsigned long)event > 1000);
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/usb/ccid-card-emulated.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index ee41a81801..c328660075 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -248,7 +248,7 @@ static void *handle_apdu_thread(void* arg)
>           WITH_QEMU_LOCK_GUARD(&card->vreader_mutex) {
>               while (!QSIMPLEQ_EMPTY(&card->guest_apdu_list)) {
>                   event = QSIMPLEQ_FIRST(&card->guest_apdu_list);
> -                assert((unsigned long)event > 1000);
> +                assert(event != NULL);
>                   QSIMPLEQ_REMOVE_HEAD(&card->guest_apdu_list, entry);
>                   if (event->p.data.type != EMUL_GUEST_APDU) {
>                       DPRINTF(card, 1, "unexpected message in handle_apdu_thread\n");

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 01/10] ccid-card-emulated: fix cast warning/error

[Laurent Vivier]

Le 03/01/2023 à 12:08, marcandre.lureau@redhat.com a écrit :
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> ../hw/usb/ccid-card-emulated.c: In function 'handle_apdu_thread':
> ../hw/usb/ccid-card-emulated.c:251:24: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>    251 |                 assert((unsigned long)event > 1000);
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/usb/ccid-card-emulated.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
> index ee41a81801..c328660075 100644
> --- a/hw/usb/ccid-card-emulated.c
> +++ b/hw/usb/ccid-card-emulated.c
> @@ -248,7 +248,7 @@ static void *handle_apdu_thread(void* arg)
>           WITH_QEMU_LOCK_GUARD(&card->vreader_mutex) {
>               while (!QSIMPLEQ_EMPTY(&card->guest_apdu_list)) {
>                   event = QSIMPLEQ_FIRST(&card->guest_apdu_list);
> -                assert((unsigned long)event > 1000);
> +                assert(event != NULL);
>                   QSIMPLEQ_REMOVE_HEAD(&card->guest_apdu_list, entry);
>                   if (event->p.data.type != EMUL_GUEST_APDU) {
>                       DPRINTF(card, 1, "unexpected message in handle_apdu_thread\n");

Applied to my trivial-patches branch.

Thanks,
Laurent

[PATCH 02/10] tests: fix path separator, use g_build_filename()

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/test-io-channel-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
index 19f72eab96..096224962c 100644
--- a/tests/unit/test-io-channel-command.c
+++ b/tests/unit/test-io-channel-command.c
@@ -32,7 +32,7 @@ static char *socat = NULL;
 static void test_io_channel_command_fifo(bool async)
 {
     g_autofree gchar *tmpdir = g_dir_make_tmp("qemu-test-io-channel.XXXXXX", NULL);
-    g_autofree gchar *fifo = g_strdup_printf("%s/%s", tmpdir, TEST_FIFO);
+    g_autofree gchar *fifo = g_build_filename(tmpdir, TEST_FIFO, NULL);
     g_autofree gchar *srcargs = g_strdup_printf("%s - PIPE:%s,wronly", socat, fifo);
     g_autofree gchar *dstargs = g_strdup_printf("%s PIPE:%s,rdonly -", socat, fifo);
     g_auto(GStrv) srcargv = g_strsplit(srcargs, " ", -1);
-- 
2.39.0

Re: [PATCH 02/10] tests: fix path separator, use g_build_filename()

[Thomas Huth]

On 03/01/2023 12.08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/unit/test-io-channel-command.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
> index 19f72eab96..096224962c 100644
> --- a/tests/unit/test-io-channel-command.c
> +++ b/tests/unit/test-io-channel-command.c
> @@ -32,7 +32,7 @@ static char *socat = NULL;
>   static void test_io_channel_command_fifo(bool async)
>   {
>       g_autofree gchar *tmpdir = g_dir_make_tmp("qemu-test-io-channel.XXXXXX", NULL);
> -    g_autofree gchar *fifo = g_strdup_printf("%s/%s", tmpdir, TEST_FIFO);
> +    g_autofree gchar *fifo = g_build_filename(tmpdir, TEST_FIFO, NULL);
>       g_autofree gchar *srcargs = g_strdup_printf("%s - PIPE:%s,wronly", socat, fifo);
>       g_autofree gchar *dstargs = g_strdup_printf("%s PIPE:%s,rdonly -", socat, fifo);
>       g_auto(GStrv) srcargv = g_strsplit(srcargs, " ", -1);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 02/10] tests: fix path separator, use g_build_filename()

[Philippe Mathieu-Daudé]

On 3/1/23 12:08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/unit/test-io-channel-command.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 03/10] tests: fix test-io-channel-command on win32

[marcandre.lureau]

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

socat "PIPE:"" on Windows are named pipes, not fifo path names.

Fixes: commit 68406d10859 ("tests/unit: cleanups for test-io-channel-command")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/test-io-channel-command.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
index 096224962c..e76ef2daaa 100644
--- a/tests/unit/test-io-channel-command.c
+++ b/tests/unit/test-io-channel-command.c
@@ -31,8 +31,12 @@ static char *socat = NULL;
 
 static void test_io_channel_command_fifo(bool async)
 {
+#ifdef WIN32
+    const gchar *fifo = TEST_FIFO;
+#else
     g_autofree gchar *tmpdir = g_dir_make_tmp("qemu-test-io-channel.XXXXXX", NULL);
     g_autofree gchar *fifo = g_build_filename(tmpdir, TEST_FIFO, NULL);
+#endif
     g_autofree gchar *srcargs = g_strdup_printf("%s - PIPE:%s,wronly", socat, fifo);
     g_autofree gchar *dstargs = g_strdup_printf("%s PIPE:%s,rdonly -", socat, fifo);
     g_auto(GStrv) srcargv = g_strsplit(srcargs, " ", -1);
@@ -57,7 +61,9 @@ static void test_io_channel_command_fifo(bool async)
     object_unref(OBJECT(src));
     object_unref(OBJECT(dst));
 
+#ifndef WIN32
     g_rmdir(tmpdir);
+#endif
 }
 
 
-- 
2.39.0

[PATCH 04/10] tests/docker: fix a win32 error due to portability

[marcandre.lureau]

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

docker.py is run during configure, and produces an error: No module
named 'pwd'.

Use a more portable and recommended alternative to lookup the user
"login name".

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/docker/docker.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 3a1ed7cb18..688ef62989 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -23,10 +23,10 @@
 import tempfile
 import re
 import signal
+import getpass
 from tarfile import TarFile, TarInfo
 from io import StringIO, BytesIO
 from shutil import copy, rmtree
-from pwd import getpwuid
 from datetime import datetime, timedelta
 
 
@@ -316,7 +316,7 @@ def build_image(self, tag, docker_dir, dockerfile,
 
         if user:
             uid = os.getuid()
-            uname = getpwuid(uid).pw_name
+            uname = getpass.getuser()
             tmp_df.write("\n")
             tmp_df.write("RUN id %s 2>/dev/null || useradd -u %d -U %s" %
                          (uname, uid, uname))
@@ -570,7 +570,7 @@ def run(self, args, argv):
 
         if args.user:
             uid = os.getuid()
-            uname = getpwuid(uid).pw_name
+            uname = getpass.getuser()
             df.write("\n")
             df.write("RUN id %s 2>/dev/null || useradd -u %d -U %s" %
                      (uname, uid, uname))
-- 
2.39.0

Re: [PATCH 04/10] tests/docker: fix a win32 error due to portability

[Philippe Mathieu-Daudé]

On 3/1/23 12:08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> docker.py is run during configure, and produces an error: No module
> named 'pwd'.
> 
> Use a more portable and recommended alternative to lookup the user
> "login name".
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/docker/docker.py | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 05/10] tests/readconfig: spice doesn't support unix socket on windows yet

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qtest/readconfig-test.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c
index c7a9b0c7dd..9ef870643d 100644
--- a/tests/qtest/readconfig-test.c
+++ b/tests/qtest/readconfig-test.c
@@ -109,8 +109,10 @@ static void test_spice(void)
     QTestState *qts;
     const char *cfgdata =
         "[spice]\n"
-        "disable-ticketing = \"on\"\n"
-        "unix = \"on\"\n";
+#ifndef WIN32
+        "unix = \"on\"\n"
+#endif
+        "disable-ticketing = \"on\"\n";
 
     qts = qtest_init_with_config(cfgdata);
     /* Test valid command */
-- 
2.39.0

Re: [PATCH 05/10] tests/readconfig: spice doesn't support unix socket on windows yet

[Thomas Huth]

On 03/01/2023 12.08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/qtest/readconfig-test.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c
> index c7a9b0c7dd..9ef870643d 100644
> --- a/tests/qtest/readconfig-test.c
> +++ b/tests/qtest/readconfig-test.c
> @@ -109,8 +109,10 @@ static void test_spice(void)
>       QTestState *qts;
>       const char *cfgdata =
>           "[spice]\n"
> -        "disable-ticketing = \"on\"\n"
> -        "unix = \"on\"\n";
> +#ifndef WIN32
> +        "unix = \"on\"\n"
> +#endif
> +        "disable-ticketing = \"on\"\n";
>   
>       qts = qtest_init_with_config(cfgdata);
>       /* Test valid command */

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 06/10] osdep: implement qemu_socketpair() for win32

[marcandre.lureau]

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

Manually implement a socketpair() function, using UNIX sockets and
simple peer credential checking.

QEMU doesn't make much use of socketpair, beside vhost-user which is not
available for win32 at this point. However, I intend to use it for
writing some new portable tests.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/sockets.h |   2 -
 util/oslib-win32.c     | 110 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 2b0698a7c9..d935fd80da 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -15,7 +15,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
 bool fd_is_socket(int fd);
 int qemu_socket(int domain, int type, int protocol);
 
-#ifndef WIN32
 /**
  * qemu_socketpair:
  * @domain: specifies a communication domain, such as PF_UNIX
@@ -30,7 +29,6 @@ int qemu_socket(int domain, int type, int protocol);
  * Return 0 on success.
  */
 int qemu_socketpair(int domain, int type, int protocol, int sv[2]);
-#endif
 
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
 /*
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 07ade41800..a7b0d8491e 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -496,6 +496,116 @@ ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t len, int flags,
     return ret;
 }
 
+int qemu_socketpair(int domain, int type, int protocol, int sv[2])
+{
+    struct sockaddr_un addr = {
+        0,
+    };
+    socklen_t socklen;
+    SOCKET listener = INVALID_SOCKET;
+    SOCKET client = INVALID_SOCKET;
+    SOCKET server = INVALID_SOCKET;
+    g_autofree char *path = NULL;
+    int tmpfd;
+    u_long arg, br;
+    int ret = -1;
+
+    g_return_val_if_fail(sv != NULL, -1);
+
+    addr.sun_family = AF_UNIX;
+    socklen = sizeof(addr);
+
+    tmpfd = g_file_open_tmp(NULL, &path, NULL);
+    if (tmpfd == -1) {
+        WSASetLastError(WSAEACCES);
+        goto out;
+    }
+
+    close(tmpfd);
+
+    if (strlen(path) >= sizeof(addr.sun_path)) {
+        WSASetLastError(WSAEACCES);
+        goto out;
+    }
+
+    strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
+
+    listener = socket(domain, type, protocol);
+    if (listener == INVALID_SOCKET) {
+        goto out;
+    }
+
+    if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
+        WSASetLastError(WSAEACCES);
+        goto out;
+    }
+    g_clear_pointer(&path, g_free);
+
+    if (bind(listener, (struct sockaddr *)&addr, socklen) == SOCKET_ERROR) {
+        goto out;
+    }
+
+    if (listen(listener, 1) == SOCKET_ERROR) {
+        goto out;
+    }
+
+    client = socket(domain, type, protocol);
+    if (client == INVALID_SOCKET) {
+        goto out;
+    }
+
+    arg = 1;
+    if (ioctlsocket(client, FIONBIO, &arg) == SOCKET_ERROR) {
+        goto out;
+    }
+
+    if (connect(client, (struct sockaddr *)&addr, socklen) == SOCKET_ERROR &&
+        WSAGetLastError() != WSAEWOULDBLOCK) {
+        goto out;
+    }
+
+    server = accept(listener, NULL, NULL);
+    if (server == INVALID_SOCKET) {
+        goto out;
+    }
+
+    arg = 0;
+    if (ioctlsocket(client, FIONBIO, &arg) == SOCKET_ERROR) {
+        goto out;
+    }
+
+    if (WSAIoctl(server, SIO_AF_UNIX_GETPEERPID, NULL, 0U, &arg, sizeof(arg),
+                 &br, NULL, NULL) == SOCKET_ERROR ||
+        arg != GetCurrentProcessId()) {
+        WSASetLastError(WSAEACCES);
+        goto out;
+    }
+
+    sv[0] = server;
+    server = INVALID_SOCKET;
+    sv[1] = client;
+    client = INVALID_SOCKET;
+    ret = 0;
+
+out:
+    if (ret == -1) {
+        errno = socket_error();
+    }
+    if (listener != INVALID_SOCKET) {
+        closesocket(listener);
+    }
+    if (client != INVALID_SOCKET) {
+        closesocket(client);
+    }
+    if (server != INVALID_SOCKET) {
+        closesocket(server);
+    }
+    if (path) {
+        DeleteFile(path);
+    }
+    return ret;
+}
+
 bool qemu_write_pidfile(const char *filename, Error **errp)
 {
     char buffer[128];
-- 
2.39.0

[PATCH 07/10] qmp: 'add_client' actually expects sockets

[marcandre.lureau]

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

Whether it is SPICE, VNC, D-Bus, or the socket chardev, they all
actually expect a socket kind or will fail in different ways at runtime.

Throw an error early if the given 'add_client' fd is not a socket, and
close it to avoid leaks.

This allows to replace the close() call with a more correct & portable
closesocket() version.

(this will allow importing sockets on Windows with a specialized command
in the following patch, while keeping the remaining monitor associated
sockets/add_client code & usage untouched)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor/qmp-cmds.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 2932b3f3a5..c491eb262b 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -20,6 +20,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/config-file.h"
 #include "qemu/uuid.h"
+#include "qemu/sockets.h"
 #include "chardev/char.h"
 #include "ui/qemu-spice.h"
 #include "ui/console.h"
@@ -248,16 +249,22 @@ void qmp_add_client(const char *protocol, const char *fdname,
         return;
     }
 
+    if (!fd_is_socket(fd)) {
+        error_setg(errp, "add_client expects a socket");
+        close(fd);
+        return;
+    }
+
     if (strcmp(protocol, "spice") == 0) {
         if (!qemu_using_spice(errp)) {
-            close(fd);
+            closesocket(fd);
             return;
         }
         skipauth = has_skipauth ? skipauth : false;
         tls = has_tls ? tls : false;
         if (qemu_spice.display_add_client(fd, skipauth, tls) < 0) {
             error_setg(errp, "spice failed to add client");
-            close(fd);
+            closesocket(fd);
         }
         return;
 #ifdef CONFIG_VNC
@@ -269,11 +276,11 @@ void qmp_add_client(const char *protocol, const char *fdname,
 #ifdef CONFIG_DBUS_DISPLAY
     } else if (strcmp(protocol, "@dbus-display") == 0) {
         if (!qemu_using_dbus_display(errp)) {
-            close(fd);
+            closesocket(fd);
             return;
         }
         if (!qemu_dbus_display.add_client(fd, errp)) {
-            close(fd);
+            closesocket(fd);
             return;
         }
         return;
@@ -281,14 +288,14 @@ void qmp_add_client(const char *protocol, const char *fdname,
     } else if ((s = qemu_chr_find(protocol)) != NULL) {
         if (qemu_chr_add_client(s, fd) < 0) {
             error_setg(errp, "failed to add client");
-            close(fd);
+            closesocket(fd);
             return;
         }
         return;
     }
 
     error_setg(errp, "protocol '%s' is invalid", protocol);
-    close(fd);
+    closesocket(fd);
 }
 
 
-- 
2.39.0

Re: [PATCH 07/10] qmp: 'add_client' actually expects sockets

[Philippe Mathieu-Daudé]

On 3/1/23 12:08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Whether it is SPICE, VNC, D-Bus, or the socket chardev, they all
> actually expect a socket kind or will fail in different ways at runtime.
> 
> Throw an error early if the given 'add_client' fd is not a socket, and
> close it to avoid leaks.
> 
> This allows to replace the close() call with a more correct & portable
> closesocket() version.
> 
> (this will allow importing sockets on Windows with a specialized command
> in the following patch, while keeping the remaining monitor associated
> sockets/add_client code & usage untouched)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   monitor/qmp-cmds.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 08/10] qmp: add 'get-win32-socket'

[marcandre.lureau]

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

A process with enough capabilities can duplicate a socket to QEMU. Add a
QMP command to import it and add it to the monitor fd list, so it can be
later used by other commands.

Note that we actually store the SOCKET in the FD list, appropriate care
must now be taken to use the correct socket functions (similar approach
is taken by our io/ code and in glib, this is internal and shouldn't
affect the QEMU/QMP users)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qapi/misc.json | 32 ++++++++++++++++++++++
 monitor/misc.c | 74 ++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 27ef5a2b20..a19dd78fab 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -272,6 +272,38 @@
 ##
 { 'command': 'getfd', 'data': {'fdname': 'str'} }
 
+##
+# @get-win32-socket:
+#
+# Add a socket that was duplicated to QEMU process with WSADuplicateSocketW()
+# via WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET
+# is considered as a kind of "file descriptor" by QMP clients, for historical
+# reasons and simplicity, although QEMU takes care to use socket functions
+# appropriately.
+#
+# @info: the WSAPROTOCOL_INFOW structure (encoded in base64)
+#
+# @fdname: file descriptor name
+#
+# Returns: Nothing on success
+#
+# Since: 8.0
+#
+# Notes: If @fdname already exists, the file descriptor assigned to
+#        it will be closed and replaced by the received file
+#        descriptor.
+#
+#        The 'closefd' command can be used to explicitly close the
+#        file descriptor when it is no longer needed.
+#
+# Example:
+#
+# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", fdname": "skclient" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 'if': 'CONFIG_WIN32' }
+
 ##
 # @closefd:
 #
diff --git a/monitor/misc.c b/monitor/misc.c
index bf3f1c67ca..4dd76afb4f 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -73,6 +73,7 @@
 #include "qapi/error.h"
 #include "qapi/qmp-event.h"
 #include "qemu/cutils.h"
+#include "qemu/sockets.h"
 
 #if defined(TARGET_S390X)
 #include "hw/s390x/storage-keys.h"
@@ -1002,27 +1003,29 @@ static void hmp_wavcapture(Monitor *mon, const QDict *qdict)
     QLIST_INSERT_HEAD (&capture_head, s, entries);
 }
 
-void qmp_getfd(const char *fdname, Error **errp)
+static void close_fd(int fd)
 {
-    Monitor *cur_mon = monitor_cur();
-    mon_fd_t *monfd;
-    int fd, tmp_fd;
-
-    fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
-    if (fd == -1) {
-        error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
-        return;
+    if (fd_is_socket(fd)) {
+        closesocket(fd);
+    } else {
+        close(fd);
     }
+}
+
+static void monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp)
+{
+    mon_fd_t *monfd;
+    int tmp_fd;
 
     if (qemu_isdigit(fdname[0])) {
-        close(fd);
+        close_fd(fd);
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
                    "a name not starting with a digit");
         return;
     }
 
-    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
-    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
+    QEMU_LOCK_GUARD(&mon->mon_lock);
+    QLIST_FOREACH(monfd, &mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
@@ -1030,7 +1033,7 @@ void qmp_getfd(const char *fdname, Error **errp)
         tmp_fd = monfd->fd;
         monfd->fd = fd;
         /* Make sure close() is outside critical section */
-        close(tmp_fd);
+        close_fd(tmp_fd);
         return;
     }
 
@@ -1038,7 +1041,21 @@ void qmp_getfd(const char *fdname, Error **errp)
     monfd->name = g_strdup(fdname);
     monfd->fd = fd;
 
-    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
+    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
+}
+
+void qmp_getfd(const char *fdname, Error **errp)
+{
+    Monitor *cur_mon = monitor_cur();
+    int fd;
+
+    fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
+    if (fd == -1) {
+        error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
+        return;
+    }
+
+    return monitor_add_fd(cur_mon, fd, fdname, errp);
 }
 
 void qmp_closefd(const char *fdname, Error **errp)
@@ -1059,7 +1076,7 @@ void qmp_closefd(const char *fdname, Error **errp)
         g_free(monfd);
         qemu_mutex_unlock(&cur_mon->mon_lock);
         /* Make sure close() is outside critical section */
-        close(tmp_fd);
+        close_fd(tmp_fd);
         return;
     }
 
@@ -1152,6 +1169,33 @@ error:
     return NULL;
 }
 
+#ifdef _WIN32
+void qmp_get_win32_socket(const char *infos, const char *fdname, Error **errp)
+{
+    g_autofree WSAPROTOCOL_INFOW *info = NULL;
+    gsize len;
+    SOCKET sk;
+
+    info = (void *)g_base64_decode(infos, &len);
+    if (len != sizeof(*info)) {
+        error_setg(errp, "Invalid WSAPROTOCOL_INFOW value");
+        return;
+    }
+
+    sk = WSASocketW(FROM_PROTOCOL_INFO,
+                    FROM_PROTOCOL_INFO,
+                    FROM_PROTOCOL_INFO,
+                    info, 0, 0);
+    if (sk == INVALID_SOCKET) {
+        g_autofree gchar *emsg = g_win32_error_message(WSAGetLastError());
+        error_setg(errp, "Couldn't create socket: %s", emsg);
+        return;
+    }
+
+    monitor_add_fd(monitor_cur(), sk, fdname, errp);
+}
+#endif
+
 void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
 {
     MonFdset *mon_fdset;
-- 
2.39.0

Re: [PATCH 08/10] qmp: add 'get-win32-socket'

[Daniel P. Berrangé]

On Tue, Jan 03, 2023 at 03:08:12PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> A process with enough capabilities can duplicate a socket to QEMU. Add a
> QMP command to import it and add it to the monitor fd list, so it can be
> later used by other commands.
> 
> Note that we actually store the SOCKET in the FD list, appropriate care
> must now be taken to use the correct socket functions (similar approach
> is taken by our io/ code and in glib, this is internal and shouldn't
> affect the QEMU/QMP users)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/misc.json | 32 ++++++++++++++++++++++
>  monitor/misc.c | 74 ++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 91 insertions(+), 15 deletions(-)
> 
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 27ef5a2b20..a19dd78fab 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -272,6 +272,38 @@
>  ##
>  { 'command': 'getfd', 'data': {'fdname': 'str'} }
>  
> +##
> +# @get-win32-socket:
> +#
> +# Add a socket that was duplicated to QEMU process with WSADuplicateSocketW()
> +# via WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A SOCKET
> +# is considered as a kind of "file descriptor" by QMP clients, for historical
> +# reasons and simplicity, although QEMU takes care to use socket functions
> +# appropriately.
> +#
> +# @info: the WSAPROTOCOL_INFOW structure (encoded in base64)
> +#
> +# @fdname: file descriptor name
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 8.0
> +#
> +# Notes: If @fdname already exists, the file descriptor assigned to
> +#        it will be closed and replaced by the received file
> +#        descriptor.
> +#
> +#        The 'closefd' command can be used to explicitly close the
> +#        file descriptor when it is no longer needed.
> +#
> +# Example:
> +#
> +# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", fdname": "skclient" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 'if': 'CONFIG_WIN32' }

IIUC, this is needed because  'getfd' doesn't work on Windows ?

Can't we just implement getfd on Windows, using this WSAPROTOCOL_INFOW
structure passing, or are you anticipating that future Windows make get
normal 'getfd' support ?  

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 08/10] qmp: add 'get-win32-socket'

[Marc-André Lureau]

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

Hi

On Mon, Jan 9, 2023 at 4:21 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Jan 03, 2023 at 03:08:12PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > A process with enough capabilities can duplicate a socket to QEMU. Add a
> > QMP command to import it and add it to the monitor fd list, so it can be
> > later used by other commands.
> >
> > Note that we actually store the SOCKET in the FD list, appropriate care
> > must now be taken to use the correct socket functions (similar approach
> > is taken by our io/ code and in glib, this is internal and shouldn't
> > affect the QEMU/QMP users)
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qapi/misc.json | 32 ++++++++++++++++++++++
> >  monitor/misc.c | 74 ++++++++++++++++++++++++++++++++++++++++----------
> >  2 files changed, 91 insertions(+), 15 deletions(-)
> >
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index 27ef5a2b20..a19dd78fab 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -272,6 +272,38 @@
> >  ##
> >  { 'command': 'getfd', 'data': {'fdname': 'str'} }
> >
> > +##
> > +# @get-win32-socket:
> > +#
> > +# Add a socket that was duplicated to QEMU process with
> WSADuplicateSocketW()
> > +# via WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name. A
> SOCKET
> > +# is considered as a kind of "file descriptor" by QMP clients, for
> historical
> > +# reasons and simplicity, although QEMU takes care to use socket
> functions
> > +# appropriately.
> > +#
> > +# @info: the WSAPROTOCOL_INFOW structure (encoded in base64)
> > +#
> > +# @fdname: file descriptor name
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 8.0
> > +#
> > +# Notes: If @fdname already exists, the file descriptor assigned to
> > +#        it will be closed and replaced by the received file
> > +#        descriptor.
> > +#
> > +#        The 'closefd' command can be used to explicitly close the
> > +#        file descriptor when it is no longer needed.
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "get-win32-socket", "arguments": { "info":
> "abcd123..", fdname": "skclient" } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname':
> 'str'}, 'if': 'CONFIG_WIN32' }
>
> IIUC, this is needed because  'getfd' doesn't work on Windows ?
>
> Can't we just implement getfd on Windows, using this WSAPROTOCOL_INFOW
> structure passing, or are you anticipating that future Windows make get
> normal 'getfd' support ?
>

Yes, I guess we could reuse 'getfd' somehow, and add an optional argument
'wsaprotocol-info' to import sockets. Other usages would fail atm, since
win32 sockets don't have SCM rights. I have some hope that one day Windows
will support it though, at least for some type of fds (sockets to start
with).

Whether this is preferable to a new command explicitely designed for win32
socket, I am not sure.


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

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

[PATCH 09/10] libqtest: make qtest_qmp_add_client work on win32

[marcandre.lureau]

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

Use the "get-win32-socket" function to pass an opened socket to QEMU,
instead of using "getfd", which relies on socket ancillary FD message
passing.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qtest/libqtest.h |  5 ++---
 tests/qtest/libqtest.c | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index fcf1c3c3b3..8d7d450963 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -758,17 +758,16 @@ void qtest_qmp_device_add_qdict(QTestState *qts, const char *drv,
 void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
                           const char *fmt, ...) G_GNUC_PRINTF(4, 5);
 
-#ifndef _WIN32
 /**
  * qtest_qmp_add_client:
  * @qts: QTestState instance to operate on
  * @protocol: the protocol to add to
  * @fd: the client file-descriptor
  *
- * Call QMP ``getfd`` followed by ``add_client`` with the given @fd.
+ * Call QMP ``getfd`` (on Windows ``get-win32-socket``) followed by
+ * ``add_client`` with the given @fd.
  */
 void qtest_qmp_add_client(QTestState *qts, const char *protocol, int fd);
-#endif /* _WIN32 */
 
 /**
  * qtest_qmp_device_del_send:
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 2fbc3b88f3..63ec328e27 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1440,13 +1440,26 @@ void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
     qobject_unref(args);
 }
 
-#ifndef _WIN32
 void qtest_qmp_add_client(QTestState *qts, const char *protocol, int fd)
 {
     QDict *resp;
 
+#ifdef WIN32
+    WSAPROTOCOL_INFOW info;
+    g_autofree char *info64  = NULL;
+
+    assert(fd_is_socket(fd));
+    if (WSADuplicateSocketW(fd, GetProcessId((HANDLE)qts->qemu_pid), &info) == SOCKET_ERROR) {
+        g_autofree char *emsg = g_win32_error_message(WSAGetLastError());
+        g_error("WSADuplicateSocketW failed: %s", emsg);
+    }
+    info64 = g_base64_encode((guchar *)&info, sizeof(info));
+    resp = qtest_qmp(qts, "{'execute': 'get-win32-socket',"
+                     "'arguments': {'fdname': 'fdname', 'info': %s}}", info64);
+#else
     resp = qtest_qmp_fds(qts, &fd, 1, "{'execute': 'getfd',"
                          "'arguments': {'fdname': 'fdname'}}");
+#endif
     g_assert(resp);
     g_assert(!qdict_haskey(resp, "event")); /* We don't expect any events */
     g_assert(!qdict_haskey(resp, "error"));
@@ -1460,7 +1473,6 @@ void qtest_qmp_add_client(QTestState *qts, const char *protocol, int fd)
     g_assert(!qdict_haskey(resp, "error"));
     qobject_unref(resp);
 }
-#endif
 
 /*
  * Generic hot-unplugging test via the device_del QMP command.
-- 
2.39.0

Re: [PATCH 09/10] libqtest: make qtest_qmp_add_client work on win32

[Thomas Huth]

On 03/01/2023 12.08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Use the "get-win32-socket" function to pass an opened socket to QEMU,
> instead of using "getfd", which relies on socket ancillary FD message
> passing.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/qtest/libqtest.h |  5 ++---
>   tests/qtest/libqtest.c | 16 ++++++++++++++--
>   2 files changed, 16 insertions(+), 5 deletions(-)

Acked-by: Thomas Huth <thuth@redhat.com>

[PATCH 10/10] qtest: enable vnc-display test on win32

[marcandre.lureau]

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

Now that qtest_qmp_add_client() works on win32, we can enable the VNC
test.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qtest/vnc-display-test.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tests/qtest/vnc-display-test.c b/tests/qtest/vnc-display-test.c
index e2a9d682bb..2997edc6ec 100644
--- a/tests/qtest/vnc-display-test.c
+++ b/tests/qtest/vnc-display-test.c
@@ -34,10 +34,6 @@ static void on_vnc_auth_failure(VncConnection *self,
 static bool
 test_setup(Test *test)
 {
-#ifdef WIN32
-    g_test_skip("Not supported on Windows yet");
-    return false;
-#else
     int pair[2];
 
     test->qts = qtest_init("-vnc none -name vnc-test");
@@ -56,7 +52,6 @@ test_setup(Test *test)
 
     test->loop = g_main_loop_new(NULL, FALSE);
     return true;
-#endif
 }
 
 static void
-- 
2.39.0

Re: [PATCH 10/10] qtest: enable vnc-display test on win32

[Thomas Huth]

On 03/01/2023 12.08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Now that qtest_qmp_add_client() works on win32, we can enable the VNC
> test.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/qtest/vnc-display-test.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/tests/qtest/vnc-display-test.c b/tests/qtest/vnc-display-test.c
> index e2a9d682bb..2997edc6ec 100644
> --- a/tests/qtest/vnc-display-test.c
> +++ b/tests/qtest/vnc-display-test.c
> @@ -34,10 +34,6 @@ static void on_vnc_auth_failure(VncConnection *self,
>   static bool
>   test_setup(Test *test)
>   {
> -#ifdef WIN32
> -    g_test_skip("Not supported on Windows yet");
> -    return false;
> -#else
>       int pair[2];
>   
>       test->qts = qtest_init("-vnc none -name vnc-test");
> @@ -56,7 +52,6 @@ test_setup(Test *test)
>   
>       test->loop = g_main_loop_new(NULL, FALSE);
>       return true;
> -#endif
>   }
>   
>   static void

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 00/10] Various win32 fixes & new 'get-win32-socket' QMP command

[Philippe Mathieu-Daudé]

Cc'ing more Windows specialists.

On 3/1/23 12:08, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> The following series first fixes a few tests on win32. The second part focuses
> on 'add_client' support, by limiting its scope to sockets and adding win32
> support. Finally, it enables vnc-display test on win32, to exercise the new code
> paths and demonstrate its usage.
> 
> 'get-win32-socket' can be used to write more robusts code & tests using sockets
> on Windows, and will be used by a follow up series testing dbus display support.
> 
> Marc-André Lureau (10):
>    ccid-card-emulated: fix cast warning/error
>    tests: fix path separator, use g_build_filename()
>    tests: fix test-io-channel-command on win32
>    tests/docker: fix a win32 error due to portability
>    tests/readconfig: spice doesn't support unix socket on windows yet
>    osdep: implement qemu_socketpair() for win32
>    qmp: 'add_client' actually expects sockets
>    qmp: add 'get-win32-socket'
>    libqtest: make qtest_qmp_add_client work on win32
>    qtest: enable vnc-display test on win32