[PATCH v4 00/11] QMP command to import win32 sockets

[PATCH v4 00/11] QMP command to import win32 sockets

[marcandre.lureau]

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

Hi,

The series focuses on 'add_client' win32 support, by limiting its scope to
sockets and adding a new command to import sockets. This enables vnc-display
test on win32, exercising the new code paths.

(a follow up series will add dbus display support on win32, with tests using
this socket import method)

v4:
- back to new get-win32-socket command, as in v1
- drop qapi conditional fixes
- add "QMP/HMP: only actually implement getfd on CONFIG_POSIX"
- add "qapi/gen: run C code through clang-format, if possible"
- rebased on pending fd/socket mix series

v3:
- drop "tests: fix test-io-channel-command on win32", not good enough
- include "char: do not double-close fd when failing to add client"
- add "monitor: release the lock before calling close()"
- rebase after recent QMP code move

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

Based-on: <20230221124802.4103554-1-marcandre.lureau@redhat.com>
("[PATCH v3 00/16] win32: do not mix SOCKET and fd space")

Marc-André Lureau (11):
  tests: fix path separator, use g_build_filename()
  char: do not double-close fd when failing to add client
  tests/docker: fix a win32 error due to portability
  osdep: implement qemu_socketpair() for win32
  qmp: 'add_client' actually expects sockets
  monitor: release the lock before calling close()
  qapi/gen: run C code through clang-format, if possible
  qmp: add 'get-win32-socket'
  libqtest: make qtest_qmp_add_client work on win32
  qtest: enable vnc-display test on win32
  QMP/HMP: only actually implement getfd on CONFIG_POSIX

 qapi/misc.json                       |  35 ++++++++-
 include/qemu/sockets.h               |   2 -
 tests/qtest/libqtest.h               |   5 +-
 chardev/char.c                       |   2 -
 monitor/fds.c                        |  77 +++++++++++++++----
 monitor/hmp-cmds.c                   |   2 +
 monitor/qmp-cmds.c                   |   7 ++
 tests/qtest/libqtest.c               |  18 ++++-
 tests/qtest/vnc-display-test.c       |  12 +--
 tests/unit/test-io-channel-command.c |   2 +-
 util/oslib-win32.c                   | 110 +++++++++++++++++++++++++++
 hmp-commands.hx                      |   2 +
 scripts/qapi/gen.py                  |  15 +++-
 scripts/qapi/introspect.py           |   2 +
 tests/docker/docker.py               |   6 +-
 15 files changed, 263 insertions(+), 34 deletions(-)

-- 
2.39.2

[PATCH v4 01/11] 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 c6e66a8c33..4f022617df 100644
--- a/tests/unit/test-io-channel-command.c
+++ b/tests/unit/test-io-channel-command.c
@@ -35,7 +35,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.2

[PATCH v4 02/11] char: do not double-close fd when failing to add client

[marcandre.lureau]

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

The caller is already closing the fd on failure.

Fixes: c3054a6e6a ("char: Factor out qmp_add_client() parts and move to chardev/")
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>
---
 chardev/char.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 11eab7764c..e69390601f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1175,12 +1175,10 @@ bool qmp_add_client_char(int fd, bool has_skipauth, bool skipauth,
 
     if (!s) {
         error_setg(errp, "protocol '%s' is invalid", protocol);
-        close(fd);
         return false;
     }
     if (qemu_chr_add_client(s, fd) < 0) {
         error_setg(errp, "failed to add client");
-        close(fd);
         return false;
     }
     return true;
-- 
2.39.2

[PATCH v4 03/11] 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.2

[PATCH v4 04/11] 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 29a667ae3d..16f8a67f7e 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
     return qemu_socket_select(sockfd, NULL, 0, errp);
 }
 
+int qemu_socketpair(int domain, int type, int protocol, int sv[2])
+{
+    struct sockaddr_un addr = {
+        0,
+    };
+    socklen_t socklen;
+    int listener = -1;
+    int client = -1;
+    int server = -1;
+    g_autofree char *path = NULL;
+    int tmpfd;
+    u_long arg;
+    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 || !path) {
+        errno = EACCES;
+        goto out;
+    }
+
+    close(tmpfd);
+
+    if (strlen(path) >= sizeof(addr.sun_path)) {
+        errno = EINVAL;
+        goto out;
+    }
+
+    strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
+
+    listener = socket(domain, type, protocol);
+    if (listener == -1) {
+        goto out;
+    }
+
+    if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
+        errno = EACCES;
+        goto out;
+    }
+    g_clear_pointer(&path, g_free);
+
+    if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
+        goto out;
+    }
+
+    if (listen(listener, 1) == -1) {
+        goto out;
+    }
+
+    client = socket(domain, type, protocol);
+    if (client == -1) {
+        goto out;
+    }
+
+    arg = 1;
+    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
+        goto out;
+    }
+
+    if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
+        WSAGetLastError() != WSAEWOULDBLOCK) {
+        goto out;
+    }
+
+    server = accept(listener, NULL, NULL);
+    if (server == -1) {
+        goto out;
+    }
+
+    arg = 0;
+    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
+        goto out;
+    }
+
+    arg = 0;
+    if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
+        goto out;
+    }
+
+    if (arg != GetCurrentProcessId()) {
+        errno = EPERM;
+        goto out;
+    }
+
+    sv[0] = server;
+    server = -1;
+    sv[1] = client;
+    client = -1;
+    ret = 0;
+
+out:
+    if (listener != -1) {
+        close(listener);
+    }
+    if (client != -1) {
+        close(client);
+    }
+    if (server != -1) {
+        close(server);
+    }
+    if (path) {
+        DeleteFile(path);
+    }
+    return ret;
+}
+
 #undef connect
 int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
                       socklen_t addrlen)
-- 
2.39.2

Re: [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32

[Daniel P. Berrangé]

On Mon, Mar 06, 2023 at 04:27:44PM +0400, marcandre.lureau@redhat.com wrote:
> 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 29a667ae3d..16f8a67f7e 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
>      return qemu_socket_select(sockfd, NULL, 0, errp);
>  }
>  
> +int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> +{
> +    struct sockaddr_un addr = {
> +        0,
> +    };
> +    socklen_t socklen;
> +    int listener = -1;
> +    int client = -1;
> +    int server = -1;
> +    g_autofree char *path = NULL;
> +    int tmpfd;
> +    u_long arg;
> +    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 || !path) {
> +        errno = EACCES;
> +        goto out;
> +    }
> +
> +    close(tmpfd);
> +
> +    if (strlen(path) >= sizeof(addr.sun_path)) {
> +        errno = EINVAL;
> +        goto out;
> +    }
> +
> +    strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> +
> +    listener = socket(domain, type, protocol);
> +    if (listener == -1) {
> +        goto out;
> +    }
> +
> +    if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
> +        errno = EACCES;
> +        goto out;
> +    }
> +    g_clear_pointer(&path, g_free);
> +
> +    if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
> +        goto out;
> +    }
> +
> +    if (listen(listener, 1) == -1) {
> +        goto out;
> +    }
> +
> +    client = socket(domain, type, protocol);
> +    if (client == -1) {
> +        goto out;
> +    }
> +
> +    arg = 1;
> +    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> +        goto out;
> +    }
> +
> +    if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
> +        WSAGetLastError() != WSAEWOULDBLOCK) {
> +        goto out;
> +    }
> +
> +    server = accept(listener, NULL, NULL);
> +    if (server == -1) {
> +        goto out;
> +    }

In theory at this point 'client' if connect() returned WSAEWOULDBLOCK,
then at this point it should be fully connected. I wonder if that is
actually guaranteed though, or should we do something to validate
there's no race condition ?

> +
> +    arg = 0;
> +    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> +        goto out;
> +    }
> +
> +    arg = 0;
> +    if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
> +        goto out;
> +    }

Maybe this will force a synchronization point ?

Alteratively select() + getsockopt(SO_ERROR) is what we used to
do to check for connect() completion (logic removed now but can
be seen in b2587932582333197c88bf663785b19f441989d7)



> +
> +    if (arg != GetCurrentProcessId()) {
> +        errno = EPERM;
> +        goto out;
> +    }
> +
> +    sv[0] = server;
> +    server = -1;
> +    sv[1] = client;
> +    client = -1;
> +    ret = 0;
> +
> +out:
> +    if (listener != -1) {
> +        close(listener);
> +    }
> +    if (client != -1) {
> +        close(client);
> +    }
> +    if (server != -1) {
> +        close(server);
> +    }
> +    if (path) {
> +        DeleteFile(path);
> +    }
> +    return ret;
> +}
> +
>  #undef connect
>  int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
>                        socklen_t addrlen)
> -- 
> 2.39.2
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32

[Marc-André Lureau]

Hi

On Tue, Mar 7, 2023 at 6:50 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Mar 06, 2023 at 04:27:44PM +0400, marcandre.lureau@redhat.com wrote:
> > 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 29a667ae3d..16f8a67f7e 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
> >      return qemu_socket_select(sockfd, NULL, 0, errp);
> >  }
> >
> > +int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> > +{
> > +    struct sockaddr_un addr = {
> > +        0,
> > +    };
> > +    socklen_t socklen;
> > +    int listener = -1;
> > +    int client = -1;
> > +    int server = -1;
> > +    g_autofree char *path = NULL;
> > +    int tmpfd;
> > +    u_long arg;
> > +    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 || !path) {
> > +        errno = EACCES;
> > +        goto out;
> > +    }
> > +
> > +    close(tmpfd);
> > +
> > +    if (strlen(path) >= sizeof(addr.sun_path)) {
> > +        errno = EINVAL;
> > +        goto out;
> > +    }
> > +
> > +    strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> > +
> > +    listener = socket(domain, type, protocol);
> > +    if (listener == -1) {
> > +        goto out;
> > +    }
> > +
> > +    if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
> > +        errno = EACCES;
> > +        goto out;
> > +    }
> > +    g_clear_pointer(&path, g_free);
> > +
> > +    if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
> > +        goto out;
> > +    }
> > +
> > +    if (listen(listener, 1) == -1) {
> > +        goto out;
> > +    }
> > +
> > +    client = socket(domain, type, protocol);
> > +    if (client == -1) {
> > +        goto out;
> > +    }
> > +
> > +    arg = 1;
> > +    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > +        goto out;
> > +    }
> > +
> > +    if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
> > +        WSAGetLastError() != WSAEWOULDBLOCK) {
> > +        goto out;
> > +    }
> > +
> > +    server = accept(listener, NULL, NULL);
> > +    if (server == -1) {
> > +        goto out;
> > +    }
>
> In theory at this point 'client' if connect() returned WSAEWOULDBLOCK,
> then at this point it should be fully connected. I wonder if that is
> actually guaranteed though, or should we do something to validate
> there's no race condition ?
>
> > +
> > +    arg = 0;
> > +    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > +        goto out;
> > +    }
> > +
> > +    arg = 0;
> > +    if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
> > +        goto out;
> > +    }
>
> Maybe this will force a synchronization point ?

yeah, I guess switching back to sync and getting the peer pid,

I assume the unix socket pair to be ready at this point.

>
> Alteratively select() + getsockopt(SO_ERROR) is what we used to
> do to check for connect() completion (logic removed now but can
> be seen in b2587932582333197c88bf663785b19f441989d7)
>

That's hopefully not necessary.

thanks

>
>
> > +
> > +    if (arg != GetCurrentProcessId()) {
> > +        errno = EPERM;
> > +        goto out;
> > +    }
> > +
> > +    sv[0] = server;
> > +    server = -1;
> > +    sv[1] = client;
> > +    client = -1;
> > +    ret = 0;
> > +
> > +out:
> > +    if (listener != -1) {
> > +        close(listener);
> > +    }
> > +    if (client != -1) {
> > +        close(client);
> > +    }
> > +    if (server != -1) {
> > +        close(server);
> > +    }
> > +    if (path) {
> > +        DeleteFile(path);
> > +    }
> > +    return ret;
> > +}
> > +
> >  #undef connect
> >  int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
> >                        socklen_t addrlen)
> > --
> > 2.39.2
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>


-- 
Marc-André Lureau

Re: [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32

[Daniel P. Berrangé]

On Wed, Mar 08, 2023 at 10:53:13AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Mar 7, 2023 at 6:50 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, Mar 06, 2023 at 04:27:44PM +0400, marcandre.lureau@redhat.com wrote:
> > > 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 29a667ae3d..16f8a67f7e 100644
> > > --- a/util/oslib-win32.c
> > > +++ b/util/oslib-win32.c
> > > @@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
> > >      return qemu_socket_select(sockfd, NULL, 0, errp);
> > >  }
> > >
> > > +int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> > > +{
> > > +    struct sockaddr_un addr = {
> > > +        0,
> > > +    };
> > > +    socklen_t socklen;
> > > +    int listener = -1;
> > > +    int client = -1;
> > > +    int server = -1;
> > > +    g_autofree char *path = NULL;
> > > +    int tmpfd;
> > > +    u_long arg;
> > > +    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 || !path) {
> > > +        errno = EACCES;
> > > +        goto out;
> > > +    }
> > > +
> > > +    close(tmpfd);
> > > +
> > > +    if (strlen(path) >= sizeof(addr.sun_path)) {
> > > +        errno = EINVAL;
> > > +        goto out;
> > > +    }
> > > +
> > > +    strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> > > +
> > > +    listener = socket(domain, type, protocol);
> > > +    if (listener == -1) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
> > > +        errno = EACCES;
> > > +        goto out;
> > > +    }
> > > +    g_clear_pointer(&path, g_free);
> > > +
> > > +    if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (listen(listener, 1) == -1) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    client = socket(domain, type, protocol);
> > > +    if (client == -1) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    arg = 1;
> > > +    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
> > > +        WSAGetLastError() != WSAEWOULDBLOCK) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    server = accept(listener, NULL, NULL);
> > > +    if (server == -1) {
> > > +        goto out;
> > > +    }
> >
> > In theory at this point 'client' if connect() returned WSAEWOULDBLOCK,
> > then at this point it should be fully connected. I wonder if that is
> > actually guaranteed though, or should we do something to validate
> > there's no race condition ?
> >
> > > +
> > > +    arg = 0;
> > > +    if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    arg = 0;
> > > +    if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
> > > +        goto out;
> > > +    }
> >
> > Maybe this will force a synchronization point ?
> 
> yeah, I guess switching back to sync and getting the peer pid,
> 
> I assume the unix socket pair to be ready at this point.

Ok, lets hope its ok, but remember this if we see any non-deterministic
failures 

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



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v4 05/11] 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>
---
 qapi/misc.json     | 3 +++
 monitor/qmp-cmds.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/qapi/misc.json b/qapi/misc.json
index 27ef5a2b20..f0217cfba0 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -14,6 +14,9 @@
 # Allow client connections for VNC, Spice and socket based
 # character devices to be passed in to QEMU via SCM_RIGHTS.
 #
+# If the FD associated with @fdname is not a socket, the command will fail and
+# the FD will be closed.
+#
 # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
 #            the name of a character device (eg. from -chardev id=XXXX)
 #
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 859012aef4..9f7751beeb 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -14,6 +14,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/sockets.h"
 #include "monitor-internal.h"
 #include "monitor/qdev.h"
 #include "monitor/qmp-helpers.h"
@@ -139,6 +140,12 @@ 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,
-- 
2.39.2

Re: [PATCH v4 05/11] qmp: 'add_client' actually expects sockets

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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>
> ---
>  qapi/misc.json     | 3 +++
>  monitor/qmp-cmds.c | 7 +++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 27ef5a2b20..f0217cfba0 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -14,6 +14,9 @@
>  # Allow client connections for VNC, Spice and socket based
>  # character devices to be passed in to QEMU via SCM_RIGHTS.
>  #
> +# If the FD associated with @fdname is not a socket, the command will fail and
> +# the FD will be closed.
> +#
>  # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
>  #            the name of a character device (eg. from -chardev id=XXXX)
>  #
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 859012aef4..9f7751beeb 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/sockets.h"
>  #include "monitor-internal.h"
>  #include "monitor/qdev.h"
>  #include "monitor/qmp-helpers.h"
> @@ -139,6 +140,12 @@ void qmp_add_client(const char *protocol, const char *fdname,
>          return;
>      }
>  
> +    if (!fd_is_socket(fd)) {
> +        error_setg(errp, "add_client expects a socket");

Let's mention the parameter name: "parameter @fdname must name 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,

Acked-by: Markus Armbruster <armbru@redhat.com>

[PATCH v4 06/11] monitor: release the lock before calling close()

[marcandre.lureau]

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

As per comment, presumably to avoid syscall in critical section.

Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 monitor/fds.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/monitor/fds.c b/monitor/fds.c
index 26b39a0ce6..7daf1064e1 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -80,7 +80,8 @@ void qmp_getfd(const char *fdname, Error **errp)
         return;
     }
 
-    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
+    /* See close() call below. */
+    qemu_mutex_lock(&cur_mon->mon_lock);
     QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
@@ -88,6 +89,7 @@ void qmp_getfd(const char *fdname, Error **errp)
 
         tmp_fd = monfd->fd;
         monfd->fd = fd;
+        qemu_mutex_unlock(&cur_mon->mon_lock);
         /* Make sure close() is outside critical section */
         close(tmp_fd);
         return;
@@ -98,6 +100,7 @@ void qmp_getfd(const char *fdname, Error **errp)
     monfd->fd = fd;
 
     QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
+    qemu_mutex_unlock(&cur_mon->mon_lock);
 }
 
 void qmp_closefd(const char *fdname, Error **errp)
-- 
2.39.2

Re: [PATCH v4 06/11] monitor: release the lock before calling close()

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> As per comment, presumably to avoid syscall in critical section.
>
> Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  monitor/fds.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/monitor/fds.c b/monitor/fds.c
> index 26b39a0ce6..7daf1064e1 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -80,7 +80,8 @@ void qmp_getfd(const char *fdname, Error **errp)
>          return;
>      }
>  
> -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> +    /* See close() call below. */
> +    qemu_mutex_lock(&cur_mon->mon_lock);
>      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
> @@ -88,6 +89,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>  
>          tmp_fd = monfd->fd;
>          monfd->fd = fd;
> +        qemu_mutex_unlock(&cur_mon->mon_lock);
>          /* Make sure close() is outside critical section */
>          close(tmp_fd);
>          return;

Not changed by your patch, but odd: when no fd named @fdname exists, the
command does nothing silently.  Shouldn't it fail then?

> @@ -98,6 +100,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>      monfd->fd = fd;
>  
>      QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> +    qemu_mutex_unlock(&cur_mon->mon_lock);
>  }
>  
>  void qmp_closefd(const char *fdname, Error **errp)

Alex suggested a different way to do this in reply to v3 of this patch.
Please have a look and reply there.

Re: [PATCH v4 06/11] monitor: release the lock before calling close()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> As per comment, presumably to avoid syscall in critical section.
>>
>> Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>  monitor/fds.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/monitor/fds.c b/monitor/fds.c
>> index 26b39a0ce6..7daf1064e1 100644
>> --- a/monitor/fds.c
>> +++ b/monitor/fds.c
>> @@ -80,7 +80,8 @@ void qmp_getfd(const char *fdname, Error **errp)
>>          return;
>>      }
>>  
>> -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
>> +    /* See close() call below. */
>> +    qemu_mutex_lock(&cur_mon->mon_lock);
>>      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>>          if (strcmp(monfd->name, fdname) != 0) {
>>              continue;
>> @@ -88,6 +89,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>>  
>>          tmp_fd = monfd->fd;
>>          monfd->fd = fd;
>> +        qemu_mutex_unlock(&cur_mon->mon_lock);
>>          /* Make sure close() is outside critical section */
>>          close(tmp_fd);
>>          return;
>
> Not changed by your patch, but odd: when no fd named @fdname exists, the
> command does nothing silently.  Shouldn't it fail then?

Alex confused me.  Ignore this.

>> @@ -98,6 +100,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>>      monfd->fd = fd;
>>  
>>      QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
>> +    qemu_mutex_unlock(&cur_mon->mon_lock);
>>  }
>>  
>>  void qmp_closefd(const char *fdname, Error **errp)
>
> Alex suggested a different way to do this in reply to v3 of this patch.
> Please have a look and reply there.

[PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible

[marcandre.lureau]

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

Make the resulting code even prettier, if possible.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/gen.py        | 15 ++++++++++++++-
 scripts/qapi/introspect.py |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b5a8d03e8e..c0ec9aa412 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -14,6 +14,7 @@
 from contextlib import contextmanager
 import os
 import re
+import subprocess
 from typing import (
     Dict,
     Iterator,
@@ -133,6 +134,7 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
 class QAPIGenCCode(QAPIGen):
     def __init__(self, fname: str):
         super().__init__(fname)
+        self.skip_format: bool = False
         self._start_if: Optional[Tuple[QAPISchemaIfCond, str, str]] = None
 
     def start_if(self, ifcond: QAPISchemaIfCond) -> None:
@@ -149,7 +151,18 @@ def end_if(self) -> None:
 
     def get_content(self) -> str:
         assert self._start_if is None
-        return super().get_content()
+
+        text = super().get_content()
+        if not self.skip_format:
+            try:
+                text = subprocess.run(["clang-format"],
+                                      input=text,
+                                      text=True,
+                                      capture_output=True,
+                                      check=True).stdout
+            except FileNotFoundError:
+                pass
+        return text
 
 
 class QAPIGenC(QAPIGenCCode):
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae..1a8cac37ef 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -174,6 +174,8 @@ def __init__(self, prefix: str, unmask: bool):
         super().__init__(
             prefix, 'qapi-introspect',
             ' * QAPI/QMP schema introspection', __doc__)
+        # for some reasons, the generated code is making clang-format go crazy
+        self._genc.skip_format = True
         self._unmask = unmask
         self._schema: Optional[QAPISchema] = None
         self._trees: List[Annotated[SchemaInfo]] = []
-- 
2.39.2

Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible

[Markus Armbruster]

Does anything in the series depend on this patch?

Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible

[Marc-André Lureau]

Hi

On Mon, Mar 6, 2023 at 8:03 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Does anything in the series depend on this patch?
>

Indeed, it's no longer directly relevant for this series. It's a
left-over from various iterations. Nevertheless, feedback welcome.

-- 
Marc-André Lureau

Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible

[Peter Maydell]

On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Make the resulting code even prettier, if possible.

This seems to be a bit short on rationale. This is generated
code, so in general nobody is going to be reading it, and
running clang-format on it every time we generate code feels
like it would be a bit of a waste of cycles...

thanks
-- PMM

Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible

[Marc-André Lureau]

Hi

On Mon, Mar 6, 2023 at 8:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Make the resulting code even prettier, if possible.
>
> This seems to be a bit short on rationale. This is generated
> code, so in general nobody is going to be reading it, and
> running clang-format on it every time we generate code feels
> like it would be a bit of a waste of cycles...

With this reasoning, why do we care about indentation of generated code at all?

I think it still makes sense, because you have many reasons to read
through it eventually, and making it a bit more friendly helps.

Whether it is a waste of time or not, hmm. Indeed, my experience with
clang-format has teached me that it is not the most CPU-friendly
sometimes...

Perhaps the solution is only to enable formatting when debugging is
enabled, for example?

-- 
Marc-André Lureau

Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible

[Peter Maydell]

On Mon, 6 Mar 2023 at 18:29, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Mon, Mar 6, 2023 at 8:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Make the resulting code even prettier, if possible.
> >
> > This seems to be a bit short on rationale. This is generated
> > code, so in general nobody is going to be reading it, and
> > running clang-format on it every time we generate code feels
> > like it would be a bit of a waste of cycles...
>
> With this reasoning, why do we care about indentation of generated code at all?
>
> I think it still makes sense, because you have many reasons to read
> through it eventually, and making it a bit more friendly helps.

Yeah, sometimes you have to read through it, so not printing
it all one one long line is helpful. But it's a tradeoff --
"make it basically kinda readable by tracking indent level" is
easy and quick; running the whole output through a pretty-printer
is more expensive and doesn't improve the output by very much
over what we already have. (If I'm wrong about that last part,
it would be useful for the commit message to give an example
of currently unreadable output that clang-format makes more usable.)

thanks
-- PMM

Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 6 Mar 2023 at 18:29, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>>
>> Hi
>>
>> On Mon, Mar 6, 2023 at 8:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> >
>> > On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
>> > >
>> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > >
>> > > Make the resulting code even prettier, if possible.
>> >
>> > This seems to be a bit short on rationale. This is generated
>> > code, so in general nobody is going to be reading it, and
>> > running clang-format on it every time we generate code feels
>> > like it would be a bit of a waste of cycles...
>>
>> With this reasoning, why do we care about indentation of generated code at all?
>>
>> I think it still makes sense, because you have many reasons to read
>> through it eventually, and making it a bit more friendly helps.
>
> Yeah, sometimes you have to read through it, so not printing

I have to read it frequently enough to make ugly code painful.

> it all one one long line is helpful. But it's a tradeoff --
> "make it basically kinda readable by tracking indent level" is
> easy and quick; running the whole output through a pretty-printer
> is more expensive and doesn't improve the output by very much
> over what we already have. (If I'm wrong about that last part,
> it would be useful for the commit message to give an example
> of currently unreadable output that clang-format makes more usable.)

Tracking indent level is certainly quick, but it can take a bit of
effort.  In review of v3, I asked for more effort, and Marc-André
floated the idea of leaving the job to readily available clang-format
instead:

    ok, I improved the indentation a bit.

    However, I think it would be simpler, and better, if we piped the
    generated code to clang-format (when available). I made a simple patch
    for that too.

My reply was

    Piping through indent or clang-format may well give us neater results
    for less effort.

    We might want to dumb down generator code then.

Message-ID: <87356yq9rs.fsf@pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg06311.html

v4 no longer contains the indentation issue that triggered this
exchange.  Let's treat this patch as if it was separate, so it doesn't
delay the remainder of the series.

You're certainly right to ask for an assessment of costs and benefits.

Costs:

* Yet another dependency, albeit optional

* Running the indenter (not sure it's noticable, but numbers wanted)

Benefits:

* Result is maybe tidier (examples wanted)

* Not in this patch: we could dumb down the code generator some (the
  dependency becomes de facto mandatory for serious QAPI developers
  then)

We may choose to shelve this patch until the next time decent formatting
takes us effort.

[PATCH v4 08/11] 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.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qapi/misc.json | 30 ++++++++++++++++++++
 monitor/fds.c  | 76 +++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index f0217cfba0..031c94050c 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -275,6 +275,36 @@
 ##
 { '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 (the SOCKET
+# is associated with a CRT file descriptor)
+#
+# @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/fds.c b/monitor/fds.c
index 7daf1064e1..9ed4197358 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -61,46 +61,55 @@ struct MonFdset {
 static QemuMutex mon_fdsets_lock;
 static QLIST_HEAD(, MonFdset) mon_fdsets;
 
-void qmp_getfd(const char *fdname, Error **errp)
+static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp)
 {
-    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 (qemu_isdigit(fdname[0])) {
         close(fd);
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
                    "a name not starting with a digit");
-        return;
+        return false;
     }
 
     /* See close() call below. */
-    qemu_mutex_lock(&cur_mon->mon_lock);
-    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
+    qemu_mutex_lock(&mon->mon_lock);
+    QLIST_FOREACH(monfd, &mon->fds, next) {
+        int tmp_fd;
+
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
 
         tmp_fd = monfd->fd;
         monfd->fd = fd;
-        qemu_mutex_unlock(&cur_mon->mon_lock);
+        qemu_mutex_unlock(&mon->mon_lock);
         /* Make sure close() is outside critical section */
         close(tmp_fd);
-        return;
+        return true;
     }
 
     monfd = g_new0(mon_fd_t, 1);
     monfd->name = g_strdup(fdname);
     monfd->fd = fd;
 
-    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
-    qemu_mutex_unlock(&cur_mon->mon_lock);
+    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
+    qemu_mutex_unlock(&mon->mon_lock);
+    return true;
+}
+
+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;
+    }
+
+    monitor_add_fd(cur_mon, fd, fdname, errp);
 }
 
 void qmp_closefd(const char *fdname, Error **errp)
@@ -214,6 +223,41 @@ 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;
+    int fd;
+
+    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) {
+        error_setg_win32(errp, WSAGetLastError(), "Couldn't import socket");
+        return;
+    }
+
+    fd = _open_osfhandle(sk, _O_BINARY);
+    if (fd < 0) {
+        error_setg_errno(errp, errno, "Failed to associate a FD with the SOCKET");
+        closesocket(sk);
+        return;
+    }
+
+    monitor_add_fd(monitor_cur(), fd, fdname, errp);
+}
+#endif
+
+
 void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
 {
     MonFdset *mon_fdset;
-- 
2.39.2

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

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/misc.json | 30 ++++++++++++++++++++
>  monitor/fds.c  | 76 +++++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 90 insertions(+), 16 deletions(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index f0217cfba0..031c94050c 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -275,6 +275,36 @@
>  ##
>  { '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 (the SOCKET
> +# is associated with a CRT file descriptor)

Long line.

> +#
> +# @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/fds.c b/monitor/fds.c
> index 7daf1064e1..9ed4197358 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -61,46 +61,55 @@ struct MonFdset {
>  static QemuMutex mon_fdsets_lock;
>  static QLIST_HEAD(, MonFdset) mon_fdsets;
>  
> -void qmp_getfd(const char *fdname, Error **errp)
> +static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp)
>  {
> -    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 (qemu_isdigit(fdname[0])) {
>          close(fd);
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>                     "a name not starting with a digit");
> -        return;
> +        return false;
>      }
>  
>      /* See close() call below. */
> -    qemu_mutex_lock(&cur_mon->mon_lock);
> -    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> +    qemu_mutex_lock(&mon->mon_lock);
> +    QLIST_FOREACH(monfd, &mon->fds, next) {
> +        int tmp_fd;
> +
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
>          }
>  
>          tmp_fd = monfd->fd;
>          monfd->fd = fd;
> -        qemu_mutex_unlock(&cur_mon->mon_lock);
> +        qemu_mutex_unlock(&mon->mon_lock);
>          /* Make sure close() is outside critical section */
>          close(tmp_fd);
> -        return;
> +        return true;
>      }

Have you considered factoring out the loop searching ->fds?

>  
>      monfd = g_new0(mon_fd_t, 1);
>      monfd->name = g_strdup(fdname);
>      monfd->fd = fd;
>  
> -    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> -    qemu_mutex_unlock(&cur_mon->mon_lock);
> +    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
> +    qemu_mutex_unlock(&mon->mon_lock);
> +    return true;
> +}
> +
> +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;
> +    }
> +
> +    monitor_add_fd(cur_mon, fd, fdname, errp);
>  }
>  
>  void qmp_closefd(const char *fdname, Error **errp)
> @@ -214,6 +223,41 @@ error:
>      return NULL;
>  }

With the doc comment tidied up:
Acked-by: Markus Armbruster <armbru@redhat.com>

The remainder is greek to me :)

>  
> +#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;
> +    int fd;
> +
> +    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) {
> +        error_setg_win32(errp, WSAGetLastError(), "Couldn't import socket");
> +        return;
> +    }
> +
> +    fd = _open_osfhandle(sk, _O_BINARY);
> +    if (fd < 0) {
> +        error_setg_errno(errp, errno, "Failed to associate a FD with the SOCKET");
> +        closesocket(sk);
> +        return;
> +    }
> +
> +    monitor_add_fd(monitor_cur(), fd, fdname, errp);
> +}
> +#endif
> +
> +
>  void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
>  {
>      MonFdset *mon_fdset;

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

[Marc-André Lureau]

Hi

On Mon, Mar 6, 2023 at 7:49 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > 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.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qapi/misc.json | 30 ++++++++++++++++++++
> >  monitor/fds.c  | 76 +++++++++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 90 insertions(+), 16 deletions(-)
> >
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index f0217cfba0..031c94050c 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -275,6 +275,36 @@
> >  ##
> >  { '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 (the SOCKET
> > +# is associated with a CRT file descriptor)
>
> Long line.
>

Ok.. see related .editorconfig patch to make everyone's life easier.

> > +#
> > +# @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/fds.c b/monitor/fds.c
> > index 7daf1064e1..9ed4197358 100644
> > --- a/monitor/fds.c
> > +++ b/monitor/fds.c
> > @@ -61,46 +61,55 @@ struct MonFdset {
> >  static QemuMutex mon_fdsets_lock;
> >  static QLIST_HEAD(, MonFdset) mon_fdsets;
> >
> > -void qmp_getfd(const char *fdname, Error **errp)
> > +static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp)
> >  {
> > -    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 (qemu_isdigit(fdname[0])) {
> >          close(fd);
> >          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> >                     "a name not starting with a digit");
> > -        return;
> > +        return false;
> >      }
> >
> >      /* See close() call below. */
> > -    qemu_mutex_lock(&cur_mon->mon_lock);
> > -    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> > +    qemu_mutex_lock(&mon->mon_lock);
> > +    QLIST_FOREACH(monfd, &mon->fds, next) {
> > +        int tmp_fd;
> > +
> >          if (strcmp(monfd->name, fdname) != 0) {
> >              continue;
> >          }
> >
> >          tmp_fd = monfd->fd;
> >          monfd->fd = fd;
> > -        qemu_mutex_unlock(&cur_mon->mon_lock);
> > +        qemu_mutex_unlock(&mon->mon_lock);
> >          /* Make sure close() is outside critical section */
> >          close(tmp_fd);
> > -        return;
> > +        return true;
> >      }
>
> Have you considered factoring out the loop searching ->fds?

Not really, it doesn't look trivial at first sight because they do
different things.

>
> >
> >      monfd = g_new0(mon_fd_t, 1);
> >      monfd->name = g_strdup(fdname);
> >      monfd->fd = fd;
> >
> > -    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> > -    qemu_mutex_unlock(&cur_mon->mon_lock);
> > +    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
> > +    qemu_mutex_unlock(&mon->mon_lock);
> > +    return true;
> > +}
> > +
> > +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;
> > +    }
> > +
> > +    monitor_add_fd(cur_mon, fd, fdname, errp);
> >  }
> >
> >  void qmp_closefd(const char *fdname, Error **errp)
> > @@ -214,6 +223,41 @@ error:
> >      return NULL;
> >  }
>
> With the doc comment tidied up:
> Acked-by: Markus Armbruster <armbru@redhat.com>
>
> The remainder is greek to me :)

ok, thanks

>
> >
> > +#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;
> > +    int fd;
> > +
> > +    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) {
> > +        error_setg_win32(errp, WSAGetLastError(), "Couldn't import socket");
> > +        return;
> > +    }
> > +
> > +    fd = _open_osfhandle(sk, _O_BINARY);
> > +    if (fd < 0) {
> > +        error_setg_errno(errp, errno, "Failed to associate a FD with the SOCKET");
> > +        closesocket(sk);
> > +        return;
> > +    }
> > +
> > +    monitor_add_fd(monitor_cur(), fd, fdname, errp);
> > +}
> > +#endif
> > +
> > +
> >  void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> >  {
> >      MonFdset *mon_fdset;
>
>


-- 
Marc-André Lureau

[PATCH v4 09/11] 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 | 18 ++++++++++++++++--
 2 files changed, 18 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 dee2032331..c3a0ef5bb4 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1478,13 +1478,28 @@ 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;
+    SOCKET s;
+
+    assert(fd_is_socket(fd));
+    s = _get_osfhandle(fd);
+    if (WSADuplicateSocketW(s, 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"));
@@ -1498,7 +1513,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.2

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

[Daniel P. Berrangé]

On Mon, Mar 06, 2023 at 04:27:49PM +0400, 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 | 18 ++++++++++++++++--
>  2 files changed, 18 insertions(+), 5 deletions(-)

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

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v4 10/11] 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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/vnc-display-test.c b/tests/qtest/vnc-display-test.c
index e52a4326ec..f8933b0761 100644
--- a/tests/qtest/vnc-display-test.c
+++ b/tests/qtest/vnc-display-test.c
@@ -19,7 +19,7 @@ typedef struct Test {
     GMainLoop *loop;
 } Test;
 
-#if !defined(WIN32) && !defined(CONFIG_DARWIN)
+#if !defined(CONFIG_DARWIN)
 
 static void on_vnc_error(VncConnection* self,
                          const char* msg)
@@ -38,10 +38,7 @@ 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;
-#elif defined(CONFIG_DARWIN)
+#if defined(CONFIG_DARWIN)
     g_test_skip("Broken on Darwin");
     return false;
 #else
@@ -59,7 +56,12 @@ test_setup(Test *test)
     g_signal_connect(test->conn, "vnc-auth-failure",
                      G_CALLBACK(on_vnc_auth_failure), NULL);
     vnc_connection_set_auth_type(test->conn, VNC_CONNECTION_AUTH_NONE);
+
+#ifdef WIN32
+    vnc_connection_open_fd(test->conn, _get_osfhandle(pair[0]));
+#else
     vnc_connection_open_fd(test->conn, pair[0]);
+#endif
 
     test->loop = g_main_loop_new(NULL, FALSE);
     return true;
-- 
2.39.2

[PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX

[marcandre.lureau]

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

Currently, the function will simply fail if ancillary fds are not
provided, for ex on unsupported platforms.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qapi/misc.json     | 2 +-
 monitor/fds.c      | 2 ++
 monitor/hmp-cmds.c | 2 ++
 hmp-commands.hx    | 2 ++
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 031c94050c..96c053e305 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -273,7 +273,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'getfd', 'data': {'fdname': 'str'} }
+{ 'command': 'getfd', 'data': {'fdname': 'str'}, 'if': 'CONFIG_POSIX' }
 
 ##
 # @get-win32-socket:
diff --git a/monitor/fds.c b/monitor/fds.c
index 9ed4197358..d86c2c674c 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -98,6 +98,7 @@ static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **err
     return true;
 }
 
+#ifdef CONFIG_POSIX
 void qmp_getfd(const char *fdname, Error **errp)
 {
     Monitor *cur_mon = monitor_cur();
@@ -111,6 +112,7 @@ void qmp_getfd(const char *fdname, Error **errp)
 
     monitor_add_fd(cur_mon, fd, fdname, errp);
 }
+#endif
 
 void qmp_closefd(const char *fdname, Error **errp)
 {
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 34bd8c67d7..6c559b48c8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -192,6 +192,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
+#ifdef CONFIG_POSIX
 void hmp_getfd(Monitor *mon, const QDict *qdict)
 {
     const char *fdname = qdict_get_str(qdict, "fdname");
@@ -200,6 +201,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict)
     qmp_getfd(fdname, &err);
     hmp_handle_error(mon, err);
 }
+#endif
 
 void hmp_closefd(Monitor *mon, const QDict *qdict)
 {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index b87c250e23..bb85ee1d26 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1486,6 +1486,7 @@ SRST
   Inject an MCE on the given CPU (x86 only).
 ERST
 
+#ifdef CONFIG_POSIX
     {
         .name       = "getfd",
         .args_type  = "fdname:s",
@@ -1501,6 +1502,7 @@ SRST
   mechanism on unix sockets, it is stored using the name *fdname* for
   later use by other monitor commands.
 ERST
+#endif
 
     {
         .name       = "closefd",
-- 
2.39.2

Re: [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX

[Markus Armbruster]

Regarding the subject line:

1. We use "qmp:" for qmp-only patches, "hmp:" for hmp-only patches, and
"monitor:" for both.  "qmp hmp:" would work there, too.

2. We're not implementing anything, we're restricting the existing
implementation to hosts where it is actually useful.

Suggest "monitor: Restrict command getfd to POSIX hosts"

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Currently, the function will simply fail if ancillary fds are not
> provided, for ex on unsupported platforms.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/misc.json     | 2 +-
>  monitor/fds.c      | 2 ++
>  monitor/hmp-cmds.c | 2 ++
>  hmp-commands.hx    | 2 ++
>  4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 031c94050c..96c053e305 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -273,7 +273,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> +{ 'command': 'getfd', 'data': {'fdname': 'str'}, 'if': 'CONFIG_POSIX' }
>  
>  ##
>  # @get-win32-socket:

This changes the failure from

    {"error": {"class": "GenericError", "desc": "No file descriptor supplied via SCM_RIGHTS"}}

to

    {"error": {"class": "CommandNotFound", "desc": "The command getfd has not been found"}}

when CONFIG_POSIX is off.  I think this is fine.  But I'd like the
commit message to document it.


> diff --git a/monitor/fds.c b/monitor/fds.c
> index 9ed4197358..d86c2c674c 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -98,6 +98,7 @@ static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **err
>      return true;
>  }
>  
> +#ifdef CONFIG_POSIX
>  void qmp_getfd(const char *fdname, Error **errp)
>  {
>      Monitor *cur_mon = monitor_cur();
> @@ -111,6 +112,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>  
>      monitor_add_fd(cur_mon, fd, fdname, errp);
>  }
> +#endif
>  
>  void qmp_closefd(const char *fdname, Error **errp)
>  {
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 34bd8c67d7..6c559b48c8 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -192,6 +192,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, err);
>  }
>  
> +#ifdef CONFIG_POSIX
>  void hmp_getfd(Monitor *mon, const QDict *qdict)
>  {
>      const char *fdname = qdict_get_str(qdict, "fdname");
> @@ -200,6 +201,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict)
>      qmp_getfd(fdname, &err);
>      hmp_handle_error(mon, err);
>  }
> +#endif
>  
>  void hmp_closefd(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index b87c250e23..bb85ee1d26 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1486,6 +1486,7 @@ SRST
>    Inject an MCE on the given CPU (x86 only).
>  ERST
>  
> +#ifdef CONFIG_POSIX
>      {
>          .name       = "getfd",
>          .args_type  = "fdname:s",
> @@ -1501,6 +1502,7 @@ SRST
>    mechanism on unix sockets, it is stored using the name *fdname* for
>    later use by other monitor commands.
>  ERST
> +#endif
>  
>      {
>          .name       = "closefd",

With the commit message brushed up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>