[PATCH v2 0/9] Various win32 fixes & teach 'getfd' QMP command to import sockets

[PATCH v2 0/9] Various win32 fixes & teach 'getfd' QMP command to import sockets

[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 the usage.

A follow up series will add dbus display support on win32, with tests using this
socket import method.

v2:
- replce the propose new command in v1, with 'wsa-info' argument in 'getfd'
- fix qapi/qmp for commands/events with optional arguments
- rebase, and tags

Marc-André Lureau (9):
  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
  osdep: implement qemu_socketpair() for win32
  qmp: 'add_client' actually expects sockets
  qapi: implement conditional command arguments
  qmp: teach 'getfd' to import sockets on win32
  libqtest: make qtest_qmp_add_client work on win32
  qtest: enable vnc-display test on win32

 qapi/misc.json                          |  16 +++-
 include/qemu/sockets.h                  |   2 -
 tests/qtest/libqtest.h                  |   2 -
 monitor/hmp-cmds.c                      |   6 +-
 monitor/misc.c                          |  75 ++++++++++++----
 monitor/qmp-cmds.c                      |  13 ++-
 tests/qtest/libqtest.c                  |  16 +++-
 tests/qtest/vnc-display-test.c          |   5 --
 tests/unit/test-io-channel-command.c    |   8 +-
 util/oslib-win32.c                      | 110 ++++++++++++++++++++++++
 scripts/qapi/commands.py                |   4 +
 scripts/qapi/gen.py                     |  19 ++--
 scripts/qapi/visit.py                   |   2 +
 tests/docker/docker.py                  |   6 +-
 tests/qapi-schema/qapi-schema-test.json |   3 +-
 15 files changed, 245 insertions(+), 42 deletions(-)

-- 
2.39.1

[PATCH v2 1/9] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@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.1

[PATCH v2 2/9] 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.1

Re: [PATCH v2 2/9] tests: fix test-io-channel-command on win32

[Thomas Huth]

On 29/01/2023 19.24, marcandre.lureau@redhat.com wrote:
> 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;

Question from a Windows ignorant: Won't this cause a race condition in case 
someone is trying to run tests in parallel (i.e. shouldn't there be a random 
part in the name)? Or are these named pipes local to the current process?

  Thomas

> +#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
>   }
>   
>   

Re: [PATCH v2 2/9] tests: fix test-io-channel-command on win32

[Marc-André Lureau]

Hi

On Mon, Feb 6, 2023 at 12:09 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 29/01/2023 19.24, marcandre.lureau@redhat.com wrote:
> > 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;
>
> Question from a Windows ignorant: Won't this cause a race condition in case
> someone is trying to run tests in parallel (i.e. shouldn't there be a random
> part in the name)? Or are these named pipes local to the current process?
>

You are right, they are global. Furthermore, socat PIPE: is actually
not using windows named-pipes, at least directly (despite debugging
log saying something like that). It's actually mkfifo() cygwin/newlib
implementation that does some magic (in msys2). It looks like it
creates some kind of "fake" file/links with metadata and actually
creates the named pipe later on demand. So it still maps to some kind
of file, and it looks like create/open races would still be possible,
even if "cygwin" mkfifo was called before socat. The alternative would
be to create pipe() beforehand and handing those fd to socat, perhaps.
I might give it a try.

thanks

>   Thomas
>
> > +#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
> >   }
> >
> >
>
>


-- 
Marc-André Lureau

Re: [PATCH v2 2/9] tests: fix test-io-channel-command on win32

[Philippe Mathieu-Daudé]

On 29/1/23 19:24, marcandre.lureau@redhat.com wrote:
> 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(+)

This doesn't apply cleanly on top of c906e6fbaa ("tests/unit: drop hacky
race avoidance in test-io-channel-command").

[PATCH v2 3/9] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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.1

[PATCH v2 4/9] 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.1

[PATCH v2 5/9] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 monitor/qmp-cmds.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index bf22a8c5a6..01cfc23407 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -22,6 +22,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/config-file.h"
 #include "qemu/uuid.h"
+#include "qemu/sockets.h"
 #include "chardev/char.h"
 #include "sysemu/kvm.h"
 #include "sysemu/runstate.h"
@@ -190,11 +191,17 @@ 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;
+    }
+
     for (i = 0; i < ARRAY_SIZE(protocol_table); i++) {
         if (!strcmp(protocol, protocol_table[i].name)) {
             if (!protocol_table[i].add_client(fd, has_skipauth, skipauth,
                                               has_tls, tls, errp)) {
-                close(fd);
+                closesocket(fd);
             }
             return;
         }
@@ -203,12 +210,12 @@ void qmp_add_client(const char *protocol, const char *fdname,
     s = qemu_chr_find(protocol);
     if (!s) {
         error_setg(errp, "protocol '%s' is invalid", protocol);
-        close(fd);
+        closesocket(fd);
         return;
     }
     if (qemu_chr_add_client(s, fd) < 0) {
         error_setg(errp, "failed to add client");
-        close(fd);
+        closesocket(fd);
         return;
     }
 }
-- 
2.39.1

[PATCH v2 6/9] qapi: implement conditional command arguments

[marcandre.lureau]

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

The generated code doesn't quite handle the conditional arguments.
For example, 'bar' in 'test-if-cmd' is not correctly surrounded by #if
conditions. See generated code in qmp_marshal_test_if_cmd().

Note that if there are multiple optional arguments at the last position,
there might be compilation issues due to extra comas. I left an assert
and FIXME for later.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/commands.py                |  4 ++++
 scripts/qapi/gen.py                     | 19 ++++++++++++++-----
 scripts/qapi/visit.py                   |  2 ++
 tests/qapi-schema/qapi-schema-test.json |  3 ++-
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 79c5e5c3a9..07997d1586 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -64,9 +64,13 @@ def gen_call(name: str,
     elif arg_type:
         assert not arg_type.variants
         for memb in arg_type.members:
+            if memb.ifcond.is_present():
+                argstr += '\n' + memb.ifcond.gen_if()
             if memb.need_has():
                 argstr += 'arg.has_%s, ' % c_name(memb.name)
             argstr += 'arg.%s, ' % c_name(memb.name)
+            if memb.ifcond.is_present():
+                argstr += '\n' + memb.ifcond.gen_endif()
 
     lhs = ''
     if ret_type:
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b5a8d03e8e..ba57e72c9b 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -111,22 +111,31 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
                  boxed: bool,
                  extra: Optional[str] = None) -> str:
     ret = ''
-    sep = ''
     if boxed:
         assert arg_type
         ret += '%s arg' % arg_type.c_param_type()
-        sep = ', '
+        if extra:
+            ret += ', '
     elif arg_type:
         assert not arg_type.variants
+        n = 0
         for memb in arg_type.members:
-            ret += sep
-            sep = ', '
+            n += 1
+            if memb.ifcond.is_present():
+                ret += '\n' + memb.ifcond.gen_if()
             if memb.need_has():
                 ret += 'bool has_%s, ' % c_name(memb.name)
             ret += '%s %s' % (memb.type.c_param_type(),
                               c_name(memb.name))
+            if extra or n != len(arg_type.members):
+                ret += ', '
+            else:
+                # FIXME: optional last argument may break compilation
+                assert not memb.ifcond.is_present()
+            if memb.ifcond.is_present():
+                ret += '\n' + memb.ifcond.gen_endif()
     if extra:
-        ret += sep + extra
+        ret += extra
     return ret if ret else 'void'
 
 
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 26a584ee4c..c56ea4d724 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -74,11 +74,13 @@ def gen_visit_object_members(name: str,
     sep = ''
     for memb in members:
         if memb.optional and not memb.need_has():
+            ret += memb.ifcond.gen_if()
             ret += mcgen('''
     bool has_%(c_name)s = !!obj->%(c_name)s;
 ''',
                          c_name=c_name(memb.name))
             sep = '\n'
+            ret += memb.ifcond.gen_endif()
     ret += sep
 
     if base:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index ba7302f42b..baa4e69f63 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -258,7 +258,8 @@
 
 { 'event': 'TEST_IF_EVENT',
   'data': { 'foo': 'TestIfStruct',
-            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
+            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' },
+            'baz': 'int' },
   'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
 
 { 'event': 'TEST_IF_EVENT2', 'data': {},
-- 
2.39.1

[PATCH v2 7/9] qmp: teach 'getfd' to import sockets on win32

[marcandre.lureau]

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

A process with enough capabilities can duplicate a socket to QEMU.
Modify 'getfd' 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     | 16 ++++++++--
 monitor/hmp-cmds.c |  6 +++-
 monitor/misc.c     | 75 ++++++++++++++++++++++++++++++++++++----------
 3 files changed, 79 insertions(+), 18 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 27ef5a2b20..cd36d8befb 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -249,10 +249,18 @@
 ##
 # @getfd:
 #
-# Receive a file descriptor via SCM rights and assign it a name
+# On UNIX, receive a file descriptor via SCM rights and assign it a name.
+#
+# On Windows, (where ancillary socket fd-passing isn't an option yet), 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" in QMP context, for historical
+# reasons and simplicity. QEMU takes care to use socket functions appropriately.
 #
 # @fdname: file descriptor name
 #
+# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0.
+#
 # Returns: Nothing on success
 #
 # Since: 0.14
@@ -270,7 +278,11 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'getfd', 'data': {'fdname': 'str'} }
+{ 'command': 'getfd', 'data': {
+    'fdname': 'str',
+    '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'}
+  }
+}
 
 ##
 # @closefd:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 1dba973092..fc9145b8fa 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1232,7 +1232,11 @@ void hmp_getfd(Monitor *mon, const QDict *qdict)
     const char *fdname = qdict_get_str(qdict, "fdname");
     Error *err = NULL;
 
-    qmp_getfd(fdname, &err);
+    qmp_getfd(fdname,
+#ifdef WIN32
+              NULL,
+#endif
+              &err);
     hmp_handle_error(mon, err);
 }
 
diff --git a/monitor/misc.c b/monitor/misc.c
index 053af4045e..96c4977e5a 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -71,6 +71,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"
@@ -957,27 +958,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;
         }
@@ -985,7 +988,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;
     }
 
@@ -993,7 +996,49 @@ 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,
+#ifdef WIN32
+               const char *wsa_info,
+#endif
+               Error **errp)
+{
+    Monitor *cur_mon = monitor_cur();
+    int fd;
+
+#ifdef WIN32
+    if (wsa_info) {
+        g_autofree WSAPROTOCOL_INFOW *info = NULL;
+        gsize len;
+        SOCKET sk;
+
+        info = (void *)g_base64_decode(wsa_info, &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;
+        }
+
+        return monitor_add_fd(cur_mon, sk, fdname, errp);
+    }
+#endif
+
+    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)
@@ -1014,7 +1059,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;
     }
 
-- 
2.39.1

[PATCH v2 8/9] libqtest: make qtest_qmp_add_client work on win32

[marcandre.lureau]

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

Duplicate a socket to QEMU, and add it via 'getfd' on win32.

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

diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index fcf1c3c3b3..36186cd946 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -758,7 +758,6 @@ 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
@@ -768,7 +767,6 @@ void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
  * Call QMP ``getfd`` 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 6b2216cb20..7542c169da 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1456,13 +1456,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': 'getfd',"
+                     "'arguments': {'fdname': 'fdname', 'wsa-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"));
@@ -1476,7 +1489,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.1

[PATCH v2 9/9] 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>
Acked-by: Thomas Huth <thuth@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.1

Re: [PATCH v2 0/9] Various win32 fixes & teach 'getfd' QMP command to import sockets

[Marc-André Lureau]

Hi

On Sun, Jan 29, 2023 at 10:25 PM <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 the usage.
>
> A follow up series will add dbus display support on win32, with tests using this
> socket import method.
>
> v2:
> - replce the propose new command in v1, with 'wsa-info' argument in 'getfd'
> - fix qapi/qmp for commands/events with optional arguments
> - rebase, and tags

ping

>
> Marc-André Lureau (9):
>   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
>   osdep: implement qemu_socketpair() for win32
>   qmp: 'add_client' actually expects sockets
>   qapi: implement conditional command arguments
>   qmp: teach 'getfd' to import sockets on win32
>   libqtest: make qtest_qmp_add_client work on win32
>   qtest: enable vnc-display test on win32
>
>  qapi/misc.json                          |  16 +++-
>  include/qemu/sockets.h                  |   2 -
>  tests/qtest/libqtest.h                  |   2 -
>  monitor/hmp-cmds.c                      |   6 +-
>  monitor/misc.c                          |  75 ++++++++++++----
>  monitor/qmp-cmds.c                      |  13 ++-
>  tests/qtest/libqtest.c                  |  16 +++-
>  tests/qtest/vnc-display-test.c          |   5 --
>  tests/unit/test-io-channel-command.c    |   8 +-
>  util/oslib-win32.c                      | 110 ++++++++++++++++++++++++
>  scripts/qapi/commands.py                |   4 +
>  scripts/qapi/gen.py                     |  19 ++--
>  scripts/qapi/visit.py                   |   2 +
>  tests/docker/docker.py                  |   6 +-
>  tests/qapi-schema/qapi-schema-test.json |   3 +-
>  15 files changed, 245 insertions(+), 42 deletions(-)
>
> --
> 2.39.1
>
>


-- 
Marc-André Lureau