[PATCH v3 00/10] Teach 'getfd' QMP command to import win32 sockets

[PATCH v3 00/10] Teach 'getfd' QMP command to import win32 sockets

[marcandre.lureau]

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

Hi,

The series focuses on 'getfd'/'add_client' win32 support, by limiting its scope
to sockets and adding a new argument to import sockets there. 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.

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

Marc-André Lureau (10):
  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: 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 -
 chardev/char.c                          |   2 -
 monitor/fds.c                           |  77 +++++++++++++----
 monitor/hmp-cmds.c                      |   6 +-
 monitor/qmp-cmds.c                      |  11 ++-
 tests/qtest/libqtest.c                  |  16 +++-
 tests/qtest/vnc-display-test.c          |   7 +-
 tests/unit/test-io-channel-command.c    |   2 +-
 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 +-
 16 files changed, 242 insertions(+), 43 deletions(-)

-- 
2.39.1

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

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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 425e2f5594..75d8de43fd 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.1

[PATCH v3 02/10] 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>
---
 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.1

Re: [PATCH v3 02/10] char: do not double-close fd when failing to add client

[Thomas Huth]

On 07/02/2023 15.25, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>   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;

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

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

[marcandre.lureau]

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

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

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

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

[Alex Bennée]

marcandre.lureau@redhat.com writes:

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

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[marcandre.lureau]

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

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

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

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

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

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

[marcandre.lureau]

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

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

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

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

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 monitor/qmp-cmds.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 859012aef4..2dae6bb10f 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,11 +140,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;
         }
@@ -151,7 +158,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
 
     if (!qmp_add_client_char(fd, has_skipauth, skipauth, has_tls, tls,
                              protocol, errp)) {
-        close(fd);
+        closesocket(fd);
     }
 }
 
-- 
2.39.1

Re: [PATCH v3 05/10] 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>
> ---
>  monitor/qmp-cmds.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 859012aef4..2dae6bb10f 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,11 +140,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;
>          }
> @@ -151,7 +158,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
>  
>      if (!qmp_add_client_char(fd, has_skipauth, skipauth, has_tls, tls,
>                               protocol, errp)) {
> -        close(fd);
> +        closesocket(fd);
>      }
>  }

Please update add_client's doc comment in qapi/misc.json to state
explicitly that a socket is required.

[PATCH v3 06/10] 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>
---
 monitor/fds.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/monitor/fds.c b/monitor/fds.c
index 26b39a0ce6..03c5e97c35 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
         return;
     }
 
-    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
+    qemu_mutex_lock(&cur_mon->mon_lock);
     QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
@@ -88,6 +88,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 +99,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.1

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

[Philippe Mathieu-Daudé]

On 7/2/23 15:25, marcandre.lureau@redhat.com wrote:
> 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>
> ---
>   monitor/fds.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor/fds.c b/monitor/fds.c
> index 26b39a0ce6..03c5e97c35 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>           return;
>       }
>   
> -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> +    qemu_mutex_lock(&cur_mon->mon_lock);

If you respin, please add /* See close() call below. */ comment.

>       QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>           if (strcmp(monfd->name, fdname) != 0) {
>               continue;
> @@ -88,6 +88,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 +99,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);
>   }

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

Re: [PATCH v3 06/10] 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>
> ---
>  monitor/fds.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/monitor/fds.c b/monitor/fds.c
> index 26b39a0ce6..03c5e97c35 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>          return;
>      }
>  
> -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> +    qemu_mutex_lock(&cur_mon->mon_lock);
>      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
> @@ -88,6 +88,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 +99,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)

This confused me.  I think I understand now, but let's double-check.

You're reverting commit 0210c3b39bef08 for qmp_getfd() because it
extended the criticial section beyond the close(), invalidating the
comment.  Correct?

Did it actually break anything?

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

[Marc-André Lureau]

Hi

On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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>
> > ---
> >  monitor/fds.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/monitor/fds.c b/monitor/fds.c
> > index 26b39a0ce6..03c5e97c35 100644
> > --- a/monitor/fds.c
> > +++ b/monitor/fds.c
> > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> >          return;
> >      }
> >
> > -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> > +    qemu_mutex_lock(&cur_mon->mon_lock);
> >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >          if (strcmp(monfd->name, fdname) != 0) {
> >              continue;
> > @@ -88,6 +88,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 +99,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)
>
> This confused me.  I think I understand now, but let's double-check.
>
> You're reverting commit 0210c3b39bef08 for qmp_getfd() because it
> extended the criticial section beyond the close(), invalidating the
> comment.  Correct?

Correct

>
> Did it actually break anything?

Not that I know of (David admitted over IRC that this was not intended)

-- 
Marc-André Lureau

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

[Daniel P. Berrangé]

On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > 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>
> > > ---
> > >  monitor/fds.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/monitor/fds.c b/monitor/fds.c
> > > index 26b39a0ce6..03c5e97c35 100644
> > > --- a/monitor/fds.c
> > > +++ b/monitor/fds.c
> > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> > >          return;
> > >      }
> > >
> > > -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> > > +    qemu_mutex_lock(&cur_mon->mon_lock);
> > >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> > >          if (strcmp(monfd->name, fdname) != 0) {
> > >              continue;
> > > @@ -88,6 +88,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 +99,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)
> >
> > This confused me.  I think I understand now, but let's double-check.
> >
> > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it
> > extended the criticial section beyond the close(), invalidating the
> > comment.  Correct?
> 
> Correct
> 
> > Did it actually break anything?
> 
> Not that I know of (David admitted over IRC that this was not intended)

Conceptually the only risk here is that 'close()' blocks for a
prolonged period of time, which prevents another thread from
acquiring the mutex.

First, the chances of close() blocking are incredibly low for
socket FDs which have not yet been used to transmit data. It
would require a malicious mgmt app to pass an unexpected FD
type that could block but that's quite hard, and we consider
the QMP client be a trusted entity anyway.

As for another thread blocking on the mutex I'm not convinced
that'll happen either. The FD set is scoped to the current
monitor. Almost certainly the FD is going to be consumed by
a later QMP device-add/object-add command, in the same thread.
Processing of that later QMP command will be delayed regardless
of whether the close is inside or outside the critical section.

AFAICT keeping close() oujtside the critical section serves
no purpose and we could just stick with the lock guard and
delete the comment.

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

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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote:
>> Hi
>> 
>> On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> > 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>
>> > > ---
>> > >  monitor/fds.c | 4 +++-
>> > >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/monitor/fds.c b/monitor/fds.c
>> > > index 26b39a0ce6..03c5e97c35 100644
>> > > --- a/monitor/fds.c
>> > > +++ b/monitor/fds.c
>> > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>> > >          return;
>> > >      }
>> > >
>> > > -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
>> > > +    qemu_mutex_lock(&cur_mon->mon_lock);
>> > >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>> > >          if (strcmp(monfd->name, fdname) != 0) {
>> > >              continue;
>> > > @@ -88,6 +88,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 +99,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)
>> >
>> > This confused me.  I think I understand now, but let's double-check.
>> >
>> > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it
>> > extended the criticial section beyond the close(), invalidating the
>> > comment.  Correct?
>> 
>> Correct
>> 
>> > Did it actually break anything?
>> 
>> Not that I know of (David admitted over IRC that this was not intended)
>
> Conceptually the only risk here is that 'close()' blocks for a
> prolonged period of time, which prevents another thread from
> acquiring the mutex.
>
> First, the chances of close() blocking are incredibly low for
> socket FDs which have not yet been used to transmit data. It
> would require a malicious mgmt app to pass an unexpected FD
> type that could block but that's quite hard, and we consider
> the QMP client be a trusted entity anyway.
>
> As for another thread blocking on the mutex I'm not convinced
> that'll happen either. The FD set is scoped to the current
> monitor. Almost certainly the FD is going to be consumed by
> a later QMP device-add/object-add command, in the same thread.
> Processing of that later QMP command will be delayed regardless
> of whether the close is inside or outside the critical section.
>
> AFAICT keeping close() oujtside the critical section serves
> no purpose and we could just stick with the lock guard and
> delete the comment.

Makes sense to me.

There's another one in monitor_add_fd().

Both are from Peter's commit 9409fc05fe2 "monitor: protect mon->fds with
mon_lock".  Peter, do you remember why you took the trouble to keep
close() outside the critical section?  I know it's been a while...

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

[Peter Xu]

On Tue, Feb 14, 2023 at 05:23:08PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote:
> >> Hi
> >> 
> >> On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> > 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>
> >> > > ---
> >> > >  monitor/fds.c | 4 +++-
> >> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/monitor/fds.c b/monitor/fds.c
> >> > > index 26b39a0ce6..03c5e97c35 100644
> >> > > --- a/monitor/fds.c
> >> > > +++ b/monitor/fds.c
> >> > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> >> > >          return;
> >> > >      }
> >> > >
> >> > > -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> >> > > +    qemu_mutex_lock(&cur_mon->mon_lock);
> >> > >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >> > >          if (strcmp(monfd->name, fdname) != 0) {
> >> > >              continue;
> >> > > @@ -88,6 +88,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 +99,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)
> >> >
> >> > This confused me.  I think I understand now, but let's double-check.
> >> >
> >> > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it
> >> > extended the criticial section beyond the close(), invalidating the
> >> > comment.  Correct?
> >> 
> >> Correct
> >> 
> >> > Did it actually break anything?
> >> 
> >> Not that I know of (David admitted over IRC that this was not intended)
> >
> > Conceptually the only risk here is that 'close()' blocks for a
> > prolonged period of time, which prevents another thread from
> > acquiring the mutex.
> >
> > First, the chances of close() blocking are incredibly low for
> > socket FDs which have not yet been used to transmit data. It
> > would require a malicious mgmt app to pass an unexpected FD
> > type that could block but that's quite hard, and we consider
> > the QMP client be a trusted entity anyway.
> >
> > As for another thread blocking on the mutex I'm not convinced
> > that'll happen either. The FD set is scoped to the current
> > monitor. Almost certainly the FD is going to be consumed by
> > a later QMP device-add/object-add command, in the same thread.
> > Processing of that later QMP command will be delayed regardless
> > of whether the close is inside or outside the critical section.
> >
> > AFAICT keeping close() oujtside the critical section serves
> > no purpose and we could just stick with the lock guard and
> > delete the comment.
> 
> Makes sense to me.
> 
> There's another one in monitor_add_fd().
> 
> Both are from Peter's commit 9409fc05fe2 "monitor: protect mon->fds with
> mon_lock".  Peter, do you remember why you took the trouble to keep
> close() outside the critical section?  I know it's been a while...

IIRC the whole purpose of keeping close() out of the mutex section was to
make sure the mutex won't take for too long in any possible way since the
mutex will be held too in the monitor iothread (which will service the
out-of-band commands), at that time we figured the close() has a chance of
getting blocked (even if unlikely!).

So to me it still makes sense to keep the close() out of the mutex section,
unless the monitor code changed in the past few years on that, and sorry in
advance if I didn't really follow what's happening..

What's the major beneift if we move it into the critical section?  We can
use the lock guard, but IMHO that's for making programming convenient only,
we should not pay for it if there's an unwanted functional difference.

In this case of close() I think it introduces back the possiblity of having
a very slow close() - I'd bet it happen only if there's a remote socket
connection to the QMP server and with unreliable network, but I really
can't really tell.  I think I used to discuss this with Dave.

I'm wondering whether I should have used a userspace spinlock, that sounds
even more proper for this case, but that's slightly off topic.  It's just
that if the original goal of "trying our best to make sure out-of-band
monitor channels is always responsive" doesn't change, hence IMHO the
comment on the lock should still be valid to me.

Thanks,

-- 
Peter Xu

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

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > 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>
> > > > ---
> > > >  monitor/fds.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/monitor/fds.c b/monitor/fds.c
> > > > index 26b39a0ce6..03c5e97c35 100644
> > > > --- a/monitor/fds.c
> > > > +++ b/monitor/fds.c
> > > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> > > >          return;
> > > >      }
> > > >
> > > > -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> > > > +    qemu_mutex_lock(&cur_mon->mon_lock);
> > > >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> > > >          if (strcmp(monfd->name, fdname) != 0) {
> > > >              continue;
> > > > @@ -88,6 +88,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 +99,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)
> > >
> > > This confused me.  I think I understand now, but let's double-check.
> > >
> > > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it
> > > extended the criticial section beyond the close(), invalidating the
> > > comment.  Correct?
> > 
> > Correct
> > 
> > > Did it actually break anything?
> > 
> > Not that I know of (David admitted over IRC that this was not intended)
> 
> Conceptually the only risk here is that 'close()' blocks for a
> prolonged period of time, which prevents another thread from
> acquiring the mutex.
> 
> First, the chances of close() blocking are incredibly low for
> socket FDs which have not yet been used to transmit data. It
> would require a malicious mgmt app to pass an unexpected FD
> type that could block but that's quite hard, and we consider
> the QMP client be a trusted entity anyway.

I agree it's unlikely; I'm not sure it actually requires something
malicious though; e.g. a managmeent app that is itself blocked,
a socket connection connection over a dead network etc are the ones
we're worrying about - stuff that's not so much slow
as either deadlocked or taking minutes for recovery/timeout.

Dave

> As for another thread blocking on the mutex I'm not convinced
> that'll happen either. The FD set is scoped to the current
> monitor. Almost certainly the FD is going to be consumed by
> a later QMP device-add/object-add command, in the same thread.
> Processing of that later QMP command will be delayed regardless
> of whether the close is inside or outside the critical section.
> 
> AFAICT keeping close() oujtside the critical section serves
> no purpose and we could just stick with the lock guard and
> delete the comment.
> 
> 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 :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Alex Bennée]

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>

I know this is already merged but as an academic exercise we could have
kept the lock guard with a little restructuring like this:

  void qmp_closefd(const char *fdname, Error **errp)
  {
      Monitor *cur_mon = monitor_cur();
      mon_fd_t *monfd;
      int tmp_fd = -1;

      WITH_QEMU_LOCK_GUARD(&cur_mon->mon_lock) {
          QLIST_FOREACH(monfd, &cur_mon->fds, next) {
              if (strcmp(monfd->name, fdname) != 0) {
                  continue;
              }

              QLIST_REMOVE(monfd, next);
              tmp_fd = monfd->fd;
              g_free(monfd->name);
              g_free(monfd);
              break;
          }
      }

      if (tmp_fd > 0) {
          /* close() must be outside critical section */
          close(tmp_fd);
      } else {
          error_setg(errp, "File descriptor named '%s' not found", fdname);
      }
  }

To my mind it makes it easier to reason about locking but I probably
have an irrational aversion to multiple exit paths for locks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> 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>
>
> I know this is already merged but as an academic exercise we could have
> kept the lock guard with a little restructuring like this:
>
>   void qmp_closefd(const char *fdname, Error **errp)
>   {
>       Monitor *cur_mon = monitor_cur();
>       mon_fd_t *monfd;
>       int tmp_fd = -1;
>
>       WITH_QEMU_LOCK_GUARD(&cur_mon->mon_lock) {
>           QLIST_FOREACH(monfd, &cur_mon->fds, next) {

No need for QLIST_FOREACH_SAFE(), because ....

>               if (strcmp(monfd->name, fdname) != 0) {
>                   continue;
>               }
>
>               QLIST_REMOVE(monfd, next);
>               tmp_fd = monfd->fd;
>               g_free(monfd->name);
>               g_free(monfd);
>               break;

... we break the loop right after modifying the list.  Correct?

>           }
>       }
>
>       if (tmp_fd > 0) {

You mean >=

>           /* close() must be outside critical section */
>           close(tmp_fd);
>       } else {
>           error_setg(errp, "File descriptor named '%s' not found", fdname);

This error is new.  See also my reply to v4 of this patch.

>       }

If we don't need the new error, we could simply

        close(tmp_fd);

unconditionally.

>   }
>
> To my mind it makes it easier to reason about locking but I probably
> have an irrational aversion to multiple exit paths for locks.

My (possibly irrational) aversion is to

    if (good) {
        do some more
    } else {
        handle error
    }

I prefer

    if (!good) {
        handle error
        bail out
    }
    do some more

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

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> 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>
>
> I know this is already merged but as an academic exercise we could have
> kept the lock guard with a little restructuring like this:
>
>   void qmp_closefd(const char *fdname, Error **errp)

Marc-André is patching qmp_getfd(), not qmp_closefd().

>   {
>       Monitor *cur_mon = monitor_cur();
>       mon_fd_t *monfd;
>       int tmp_fd = -1;
>
>       WITH_QEMU_LOCK_GUARD(&cur_mon->mon_lock) {
>           QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>               if (strcmp(monfd->name, fdname) != 0) {
>                   continue;
>               }
>
>               QLIST_REMOVE(monfd, next);
>               tmp_fd = monfd->fd;
>               g_free(monfd->name);
>               g_free(monfd);
>               break;
>           }
>       }
>
>       if (tmp_fd > 0) {
>           /* close() must be outside critical section */
>           close(tmp_fd);
>       } else {
>           error_setg(errp, "File descriptor named '%s' not found", fdname);
>       }
>   }
>
> To my mind it makes it easier to reason about locking but I probably
> have an irrational aversion to multiple exit paths for locks.

[PATCH v3 07/10] 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

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

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

Fails "make check" for me:

2/2 qemu:qapi-schema+qapi-frontend / QAPI schema regression tests        FAIL            0.09s   exit status 1
>>> MALLOC_PERTURB_=241 PYTHONPATH=/work/armbru/qemu/scripts /usr/bin/python3 [...]
――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
stdout:
--- /work/armbru/qemu/bld-clang/../tests/qapi-schema/qapi-schema-test.out
+++ 
@@ -297,6 +297,7 @@
     member foo: TestIfStruct optional=False
     member bar: TestIfEnumList optional=False
         if TEST_IF_EVT_BAR
+    member baz: int optional=False
     if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
 event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
     boxed=False
stderr:
qapi-schema-test FAIL

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

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

Hi Markus

On Thu, Feb 9, 2023 at 4:42 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > 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>
>
> Fails "make check" for me:
>
> 2/2 qemu:qapi-schema+qapi-frontend / QAPI schema regression tests
> FAIL            0.09s   exit status 1
> >>> MALLOC_PERTURB_=241 PYTHONPATH=/work/armbru/qemu/scripts
> /usr/bin/python3 [...]
> ――――――――――――――――――――――――――――――――――――― ✀
> ―――――――――――――――――――――――――――――――――――――
> stdout:
> --- /work/armbru/qemu/bld-clang/../tests/qapi-schema/qapi-schema-test.out
> +++
> @@ -297,6 +297,7 @@
>      member foo: TestIfStruct optional=False
>      member bar: TestIfEnumList optional=False
>          if TEST_IF_EVT_BAR
> +    member baz: int optional=False
>      if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
>  event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
>      boxed=False
> stderr:
> qapi-schema-test FAIL
>

This is trivially fixed. Can you review the patch, and in particular
comment on the FIXME left, whether it is acceptable?

thanks

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

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

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

@argstr is emitted further down:

       %(lhs)sqmp_%(name)s(%(args)s&err);
   ''',
                    name=name, args=argstr, lhs=lhs)

       ret += mcgen('''
       if (err) {
   ''')

Before the patch, @argstr contains no newlines.  Works.

After the patch, it may contain newlines, and if it does, intentation is
messed up.  For instance, in the code generated for
qapi-schema-test.json:

        retval = qmp_test_if_cmd(arg.foo, 
    #if defined(TEST_IF_CMD_BAR)
    arg.bar, 
    #endif /* defined(TEST_IF_CMD_BAR) */
    &err);

Strings interpolated into the mcgen() argument should not contain
newlines.  I'm afraid you have to rewrite the code emitting the call.

> 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()

Does the assertion guard against the C compilation failure?

Is it possible to write schema code that triggers it?

> +            if memb.ifcond.is_present():
> +                ret += '\n' + memb.ifcond.gen_endif()
>      if extra:
> -        ret += sep + extra
> +        ret += extra
>      return ret if ret else 'void'
>  
>  

Same newline issue as in gen_call().  Generated code:

    UserDefThree *qmp_test_if_cmd(TestIfStruct *foo, 
    #if defined(TEST_IF_CMD_BAR)
    TestIfEnum bar, 
    #endif /* defined(TEST_IF_CMD_BAR) */
    Error **errp);

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

This hunk has no effect on the code generated for our schemas as far as
I can tell.  Is it superfluous?  Incorrect?  Gap in test coverage?  Or
am I confused?

> 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': {},

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

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

Hi Markus

On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <armbru@redhat.com>
wrote:

> marcandre.lureau@redhat.com writes:
>
> > 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:
>
> @argstr is emitted further down:
>
>        %(lhs)sqmp_%(name)s(%(args)s&err);
>    ''',
>                     name=name, args=argstr, lhs=lhs)
>
>        ret += mcgen('''
>        if (err) {
>    ''')
>
> Before the patch, @argstr contains no newlines.  Works.
>
> After the patch, it may contain newlines, and if it does, intentation is
> messed up.  For instance, in the code generated for
> qapi-schema-test.json:
>
>         retval = qmp_test_if_cmd(arg.foo,
>     #if defined(TEST_IF_CMD_BAR)
>     arg.bar,
>     #endif /* defined(TEST_IF_CMD_BAR) */
>     &err);
>
> Strings interpolated into the mcgen() argument should not contain
> newlines.  I'm afraid you have to rewrite the code emitting the call.
>

Why it should not contain newlines?

What are you asking exactly? that the caller be changed? (this does not
work well if there are multiple optional arguments..)

    #if defined(TEST_IF_CMD_BAR)
        retval = qmp_test_if_cmd(arg.foo, arg.bar, &err);
    #else
        retval = qmp_test_if_cmd(arg.foo, &err);
    #endif /* defined(TEST_IF_CMD_BAR) */


>
> > 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()
>
> Does the assertion guard against the C compilation failure?
>

Yes


>
> Is it possible to write schema code that triggers it?
>

Yes, the one we have for TEST_IF_EVENT for example:

{ 'event': 'TEST_IF_EVENT',
  'data': { 'foo': 'TestIfStruct',
            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },

produces:

void qapi_event_send_test_if_event(TestIfStruct *foo,
#if defined(TEST_IF_EVT_BAR)
TestIfEnumList *bar,
#endif /* defined(TEST_IF_EVT_BAR) */
);

Which will fail to compile if TEST_IF_EVT_BAR is undefined.

So I would rather assert that we don't introduce such a schema, until we
fix the code generator. Or we acknowledge the limitation, and treat it as a
schema error. Other ideas?


> > +            if memb.ifcond.is_present():
> > +                ret += '\n' + memb.ifcond.gen_endif()
> >      if extra:
> > -        ret += sep + extra
> > +        ret += extra
> >      return ret if ret else 'void'
> >
> >
>
> Same newline issue as in gen_call().  Generated code:
>
>     UserDefThree *qmp_test_if_cmd(TestIfStruct *foo,
>     #if defined(TEST_IF_CMD_BAR)
>     TestIfEnum bar,
>     #endif /* defined(TEST_IF_CMD_BAR) */
>     Error **errp);
>
> > 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:
>
> This hunk has no effect on the code generated for our schemas as far as
> I can tell.  Is it superfluous?  Incorrect?  Gap in test coverage?  Or
> am I confused?
>
>
Right, we could change the test this way to exercise it:

--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -250,7 +250,7 @@
 { 'command': 'test-if-cmd',
   'data': {
     'foo': 'TestIfStruct',
-    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_BAR' } },
+    '*bar': { 'type': 'TestIfStruct', 'if': 'TEST_IF_STRUCT' } },
   'returns': 'UserDefThree',


> > 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': {},
>
>

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

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

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

> Hi Markus
>
> On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > 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:
>>
>> @argstr is emitted further down:
>>
>>        %(lhs)sqmp_%(name)s(%(args)s&err);
>>    ''',
>>                     name=name, args=argstr, lhs=lhs)
>>
>>        ret += mcgen('''
>>        if (err) {
>>    ''')
>>
>> Before the patch, @argstr contains no newlines.  Works.
>>
>> After the patch, it may contain newlines, and if it does, intentation is
>> messed up.  For instance, in the code generated for
>> qapi-schema-test.json:
>>
>>         retval = qmp_test_if_cmd(arg.foo,
>>     #if defined(TEST_IF_CMD_BAR)
>>     arg.bar,
>>     #endif /* defined(TEST_IF_CMD_BAR) */
>>     &err);
>>
>> Strings interpolated into the mcgen() argument should not contain
>> newlines.  I'm afraid you have to rewrite the code emitting the call.
>>
>
> Why it should not contain newlines?

They mess up indentation.  I think.  It's been a while...  All I really
know for sure is that the generated code's indentation is messed up
right there.

> What are you asking exactly? that the caller be changed? (this does not
> work well if there are multiple optional arguments..)
>
>     #if defined(TEST_IF_CMD_BAR)
>         retval = qmp_test_if_cmd(arg.foo, arg.bar, &err);
>     #else
>         retval = qmp_test_if_cmd(arg.foo, &err);
>     #endif /* defined(TEST_IF_CMD_BAR) */

I'm asking for better indentation.  In handwritten code, we'd do

        retval = qmp_test_if_cmd(arg.foo,
    #if defined(TEST_IF_CMD_BAR)
                                 arg.bar,
    #endif /* defined(TEST_IF_CMD_BAR) */
                                 &err);

Keeping track of how far to indent the arguments is bothersome in the
generator, though.  Perhaps we could create infrastructure to make it
not bothersome, but I'm not asking for that.  Something like this should
be good enough:

        retval = qmp_test_if_cmd(arg.foo,
    #if defined(TEST_IF_CMD_BAR)
                    arg.bar,
    #endif /* defined(TEST_IF_CMD_BAR) */
                    &err);

I.e. indent to the function call and then some.

>> > 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()
>>
>> Does the assertion guard against the C compilation failure?
>
> Yes
>
>>
>> Is it possible to write schema code that triggers it?
>
> Yes, the one we have for TEST_IF_EVENT for example:
>
> { 'event': 'TEST_IF_EVENT',
>   'data': { 'foo': 'TestIfStruct',
>             'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },

This is the one you put in qapi-schema-test.json less the last
parameter, so that the conditional parameter becomes the last one.

> produces:
>
> void qapi_event_send_test_if_event(TestIfStruct *foo,
> #if defined(TEST_IF_EVT_BAR)
> TestIfEnumList *bar,
> #endif /* defined(TEST_IF_EVT_BAR) */
> );
>
> Which will fail to compile if TEST_IF_EVT_BAR is undefined.

I think it'll fail to compile always, because the parameter list has a
trailing comma regardless of TEST_IF_EVT_BAR.

> So I would rather assert that we don't introduce such a schema, until we
> fix the code generator. Or we acknowledge the limitation, and treat it as a
> schema error. Other ideas?

Yes: throw an error.  Assertions are for programming errors.  This isn't
a programming error, it's a limitation of the current implementation.

How hard would it be to lift the limitation?

>> > +            if memb.ifcond.is_present():
>> > +                ret += '\n' + memb.ifcond.gen_endif()
>> >      if extra:
>> > -        ret += sep + extra
>> > +        ret += extra
>> >      return ret if ret else 'void'
>> >
>> >
>>
>> Same newline issue as in gen_call().  Generated code:
>>
>>     UserDefThree *qmp_test_if_cmd(TestIfStruct *foo,
>>     #if defined(TEST_IF_CMD_BAR)
>>     TestIfEnum bar,
>>     #endif /* defined(TEST_IF_CMD_BAR) */
>>     Error **errp);
>>
>> > 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:
>>
>> This hunk has no effect on the code generated for our schemas as far as
>> I can tell.  Is it superfluous?  Incorrect?  Gap in test coverage?  Or
>> am I confused?
>>
>>
> Right, we could change the test this way to exercise it:
>
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -250,7 +250,7 @@
>  { 'command': 'test-if-cmd',
>    'data': {
>      'foo': 'TestIfStruct',
> -    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_BAR' } },
> +    '*bar': { 'type': 'TestIfStruct', 'if': 'TEST_IF_STRUCT' } },
>    'returns': 'UserDefThree',

Please exercise it :)

>> > 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': {},
>>
>>

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

Hi

On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi Markus
> >
> > On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <armbru@redhat.com>
> > wrote:
> >
> >> marcandre.lureau@redhat.com writes:
> >>
> >> > 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:
> >>
> >> @argstr is emitted further down:
> >>
> >>        %(lhs)sqmp_%(name)s(%(args)s&err);
> >>    ''',
> >>                     name=name, args=argstr, lhs=lhs)
> >>
> >>        ret += mcgen('''
> >>        if (err) {
> >>    ''')
> >>
> >> Before the patch, @argstr contains no newlines.  Works.
> >>
> >> After the patch, it may contain newlines, and if it does, intentation is
> >> messed up.  For instance, in the code generated for
> >> qapi-schema-test.json:
> >>
> >>         retval = qmp_test_if_cmd(arg.foo,
> >>     #if defined(TEST_IF_CMD_BAR)
> >>     arg.bar,
> >>     #endif /* defined(TEST_IF_CMD_BAR) */
> >>     &err);
> >>
> >> Strings interpolated into the mcgen() argument should not contain
> >> newlines.  I'm afraid you have to rewrite the code emitting the call.
> >>
> >
> > Why it should not contain newlines?
>
> They mess up indentation.  I think.  It's been a while...  All I really
> know for sure is that the generated code's indentation is messed up
> right there.
>
> > What are you asking exactly? that the caller be changed? (this does not
> > work well if there are multiple optional arguments..)
> >
> >     #if defined(TEST_IF_CMD_BAR)
> >         retval = qmp_test_if_cmd(arg.foo, arg.bar, &err);
> >     #else
> >         retval = qmp_test_if_cmd(arg.foo, &err);
> >     #endif /* defined(TEST_IF_CMD_BAR) */
>
> I'm asking for better indentation.  In handwritten code, we'd do
>
>         retval = qmp_test_if_cmd(arg.foo,
>     #if defined(TEST_IF_CMD_BAR)
>                                  arg.bar,
>     #endif /* defined(TEST_IF_CMD_BAR) */
>                                  &err);
>
> Keeping track of how far to indent the arguments is bothersome in the
> generator, though.  Perhaps we could create infrastructure to make it
> not bothersome, but I'm not asking for that.  Something like this should
> be good enough:
>
>         retval = qmp_test_if_cmd(arg.foo,
>     #if defined(TEST_IF_CMD_BAR)
>                     arg.bar,
>     #endif /* defined(TEST_IF_CMD_BAR) */
>                     &err);
>
> I.e. indent to the function call and then some.

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.

>
> >> > 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()
> >>
> >> Does the assertion guard against the C compilation failure?
> >
> > Yes
> >
> >>
> >> Is it possible to write schema code that triggers it?
> >
> > Yes, the one we have for TEST_IF_EVENT for example:
> >
> > { 'event': 'TEST_IF_EVENT',
> >   'data': { 'foo': 'TestIfStruct',
> >             'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
>
> This is the one you put in qapi-schema-test.json less the last
> parameter, so that the conditional parameter becomes the last one.
>
> > produces:
> >
> > void qapi_event_send_test_if_event(TestIfStruct *foo,
> > #if defined(TEST_IF_EVT_BAR)
> > TestIfEnumList *bar,
> > #endif /* defined(TEST_IF_EVT_BAR) */
> > );
> >
> > Which will fail to compile if TEST_IF_EVT_BAR is undefined.
>
> I think it'll fail to compile always, because the parameter list has a
> trailing comma regardless of TEST_IF_EVT_BAR.

Yes, I think I hand-wrote that example, the actual generator does not
leave a trailing comma here.

>
> > So I would rather assert that we don't introduce such a schema, until we
> > fix the code generator. Or we acknowledge the limitation, and treat it as a
> > schema error. Other ideas?
>
> Yes: throw an error.  Assertions are for programming errors.  This isn't
> a programming error, it's a limitation of the current implementation.
>
> How hard would it be to lift the limitation?

Taking this as a problematic example:

void function(first,
#ifdef A
    a,
#endif
#ifdef B
    b
#endif
)

I think it would mean that we would have to pass arguments as a
structure, as they don't have the limitation of trailing coma in
initializers. That would not be idiomatic C though, and we would need
to refactor a lot of code..

Another option is to always pass a dummy last argument? :)

void command(first,
#ifdef A
    a,
#endif
#ifdef B
    b,
#endif
    dummy)


>
> >> > +            if memb.ifcond.is_present():
> >> > +                ret += '\n' + memb.ifcond.gen_endif()
> >> >      if extra:
> >> > -        ret += sep + extra
> >> > +        ret += extra
> >> >      return ret if ret else 'void'
> >> >
> >> >
> >>
> >> Same newline issue as in gen_call().  Generated code:
> >>
> >>     UserDefThree *qmp_test_if_cmd(TestIfStruct *foo,
> >>     #if defined(TEST_IF_CMD_BAR)
> >>     TestIfEnum bar,
> >>     #endif /* defined(TEST_IF_CMD_BAR) */
> >>     Error **errp);
> >>
> >> > 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:
> >>
> >> This hunk has no effect on the code generated for our schemas as far as
> >> I can tell.  Is it superfluous?  Incorrect?  Gap in test coverage?  Or
> >> am I confused?
> >>
> >>
> > Right, we could change the test this way to exercise it:
> >
> > --- a/tests/qapi-schema/qapi-schema-test.json
> > +++ b/tests/qapi-schema/qapi-schema-test.json
> > @@ -250,7 +250,7 @@
> >  { 'command': 'test-if-cmd',
> >    'data': {
> >      'foo': 'TestIfStruct',
> > -    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_BAR' } },
> > +    '*bar': { 'type': 'TestIfStruct', 'if': 'TEST_IF_STRUCT' } },
> >    'returns': 'UserDefThree',
>
> Please exercise it :)

ok

>
> >> > 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': {},
> >>
> >>
>
>

thanks

-- 
Marc-André Lureau

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Hi Markus
>> >
>> > On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <armbru@redhat.com>
>> > wrote:
>> >
>> >> marcandre.lureau@redhat.com writes:
>> >>
>> >> > 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:
>> >>
>> >> @argstr is emitted further down:
>> >>
>> >>        %(lhs)sqmp_%(name)s(%(args)s&err);
>> >>    ''',
>> >>                     name=name, args=argstr, lhs=lhs)
>> >>
>> >>        ret += mcgen('''
>> >>        if (err) {
>> >>    ''')
>> >>
>> >> Before the patch, @argstr contains no newlines.  Works.
>> >>
>> >> After the patch, it may contain newlines, and if it does, intentation is
>> >> messed up.  For instance, in the code generated for
>> >> qapi-schema-test.json:
>> >>
>> >>         retval = qmp_test_if_cmd(arg.foo,
>> >>     #if defined(TEST_IF_CMD_BAR)
>> >>     arg.bar,
>> >>     #endif /* defined(TEST_IF_CMD_BAR) */
>> >>     &err);
>> >>
>> >> Strings interpolated into the mcgen() argument should not contain
>> >> newlines.  I'm afraid you have to rewrite the code emitting the call.
>> >>
>> >
>> > Why it should not contain newlines?
>>
>> They mess up indentation.  I think.  It's been a while...  All I really
>> know for sure is that the generated code's indentation is messed up
>> right there.
>>
>> > What are you asking exactly? that the caller be changed? (this does not
>> > work well if there are multiple optional arguments..)
>> >
>> >     #if defined(TEST_IF_CMD_BAR)
>> >         retval = qmp_test_if_cmd(arg.foo, arg.bar, &err);
>> >     #else
>> >         retval = qmp_test_if_cmd(arg.foo, &err);
>> >     #endif /* defined(TEST_IF_CMD_BAR) */
>>
>> I'm asking for better indentation.  In handwritten code, we'd do
>>
>>         retval = qmp_test_if_cmd(arg.foo,
>>     #if defined(TEST_IF_CMD_BAR)
>>                                  arg.bar,
>>     #endif /* defined(TEST_IF_CMD_BAR) */
>>                                  &err);
>>
>> Keeping track of how far to indent the arguments is bothersome in the
>> generator, though.  Perhaps we could create infrastructure to make it
>> not bothersome, but I'm not asking for that.  Something like this should
>> be good enough:
>>
>>         retval = qmp_test_if_cmd(arg.foo,
>>     #if defined(TEST_IF_CMD_BAR)
>>                     arg.bar,
>>     #endif /* defined(TEST_IF_CMD_BAR) */
>>                     &err);
>>
>> I.e. indent to the function call and then some.
>
> 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.

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

We might want to dumb down generator code then.

>> >> > 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()
>> >>
>> >> Does the assertion guard against the C compilation failure?
>> >
>> > Yes
>> >
>> >>
>> >> Is it possible to write schema code that triggers it?
>> >
>> > Yes, the one we have for TEST_IF_EVENT for example:
>> >
>> > { 'event': 'TEST_IF_EVENT',
>> >   'data': { 'foo': 'TestIfStruct',
>> >             'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
>>
>> This is the one you put in qapi-schema-test.json less the last
>> parameter, so that the conditional parameter becomes the last one.
>>
>> > produces:
>> >
>> > void qapi_event_send_test_if_event(TestIfStruct *foo,
>> > #if defined(TEST_IF_EVT_BAR)
>> > TestIfEnumList *bar,
>> > #endif /* defined(TEST_IF_EVT_BAR) */
>> > );
>> >
>> > Which will fail to compile if TEST_IF_EVT_BAR is undefined.
>>
>> I think it'll fail to compile always, because the parameter list has a
>> trailing comma regardless of TEST_IF_EVT_BAR.
>
> Yes, I think I hand-wrote that example, the actual generator does not
> leave a trailing comma here.
>
>>
>> > So I would rather assert that we don't introduce such a schema, until we
>> > fix the code generator. Or we acknowledge the limitation, and treat it as a
>> > schema error. Other ideas?
>>
>> Yes: throw an error.  Assertions are for programming errors.  This isn't
>> a programming error, it's a limitation of the current implementation.
>>
>> How hard would it be to lift the limitation?
>
> Taking this as a problematic example:
>
> void function(first,
> #ifdef A
>     a,
> #endif
> #ifdef B
>     b
> #endif
> )
>
> I think it would mean that we would have to pass arguments as a
> structure, as they don't have the limitation of trailing coma in
> initializers. That would not be idiomatic C though, and we would need
> to refactor a lot of code..
>
> Another option is to always pass a dummy last argument? :)
>
> void command(first,
> #ifdef A
>     a,
> #endif
> #ifdef B
>     b,
> #endif
>     dummy)

Yet another option:

  void command(first
  #ifdef A
      , a
  #endif
  #ifdef B
      , b
  #endif
      )

[...]

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

Hi

On Wed, Feb 22, 2023 at 2:23 PM Markus Armbruster <armbru@redhat.com> wrote:
> > Another option is to always pass a dummy last argument? :)
> >
> > void command(first,
> > #ifdef A
> >     a,
> > #endif
> > #ifdef B
> >     b,
> > #endif
> >     dummy)
>
> Yet another option:
>
>   void command(first
>   #ifdef A
>       , a
>   #endif
>   #ifdef B
>       , b
>   #endif
>       )
>
> [...]
>

Since I think we always have a first argument, that might be indeed
the best solution. I'll try that. thanks

-- 
Marc-André Lureau

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

Hi

On Wed, Feb 22, 2023 at 2:29 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Feb 22, 2023 at 2:23 PM Markus Armbruster <armbru@redhat.com> wrote:
> > > Another option is to always pass a dummy last argument? :)
> > >
> > > void command(first,
> > > #ifdef A
> > >     a,
> > > #endif
> > > #ifdef B
> > >     b,
> > > #endif
> > >     dummy)
> >
> > Yet another option:
> >
> >   void command(first
> >   #ifdef A
> >       , a
> >   #endif
> >   #ifdef B
> >       , b
> >   #endif
> >       )
> >
> > [...]
> >
>
> Since I think we always have a first argument, that might be indeed
> the best solution. I'll try that. thanks
>

Actually, this is just moving the problem to the first argument. (and
it also breaks clang-format, which doesn't have a allow-leading-commas
option or similar). So I'll just add an error when using conditional
fields/args last.


-- 
Marc-André Lureau

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Eric Blake]

On Wed, Feb 22, 2023 at 11:23:03AM +0100, Markus Armbruster wrote:
> > 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.
> 
> Piping through indent or clang-format may well give us neater results
> for less effort.
> 
> We might want to dumb down generator code then.

Indeed, this approach seems like it might be worth pursuing (our
generator doesn't have to worry about spacing, because we do that in a
second pass with something that will still produce human-legible final
results).

> >> > So I would rather assert that we don't introduce such a schema, until we
> >> > fix the code generator. Or we acknowledge the limitation, and treat it as a
> >> > schema error. Other ideas?
> >>
> >> Yes: throw an error.  Assertions are for programming errors.  This isn't
> >> a programming error, it's a limitation of the current implementation.
> >>
> >> How hard would it be to lift the limitation?
> >
> > Taking this as a problematic example:
> >
> > void function(first,
> > #ifdef A
> >     a,
> > #endif
> > #ifdef B
> >     b
> > #endif
> > )

I am NOT a fan of preprocessor conditionals mid-function-signature.
It gets really nasty, really fast.  Is there any way we can have:

struct S {
#ifdef A
  type a;
#endif
#ifdef B
  type b;
#endif
};

void function(struct S)

so that the preprocessor conditionals never appear inside ()?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Daniel P. Berrangé]

On Tue, Feb 28, 2023 at 09:58:01AM -0600, Eric Blake wrote:
> On Wed, Feb 22, 2023 at 11:23:03AM +0100, Markus Armbruster wrote:
> > > 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.
> > 
> > Piping through indent or clang-format may well give us neater results
> > for less effort.
> > 
> > We might want to dumb down generator code then.
> 
> Indeed, this approach seems like it might be worth pursuing (our
> generator doesn't have to worry about spacing, because we do that in a
> second pass with something that will still produce human-legible final
> results).
> 
> > >> > So I would rather assert that we don't introduce such a schema, until we
> > >> > fix the code generator. Or we acknowledge the limitation, and treat it as a
> > >> > schema error. Other ideas?
> > >>
> > >> Yes: throw an error.  Assertions are for programming errors.  This isn't
> > >> a programming error, it's a limitation of the current implementation.
> > >>
> > >> How hard would it be to lift the limitation?
> > >
> > > Taking this as a problematic example:
> > >
> > > void function(first,
> > > #ifdef A
> > >     a,
> > > #endif
> > > #ifdef B
> > >     b
> > > #endif
> > > )
> 
> I am NOT a fan of preprocessor conditionals mid-function-signature.
> It gets really nasty, really fast.  Is there any way we can have:
> 
> struct S {
> #ifdef A
>   type a;
> #endif
> #ifdef B
>   type b;
> #endif
> };
> 
> void function(struct S)
> 
> so that the preprocessor conditionals never appear inside ()?

I'd question whether we should be doing conditional arguments
at all.

IMHO having an API contract that changes based on configuration
file settings is going to be nothing but trouble. Not only does
it make the declaration ugly, but all callers become ugly too
with conditionals. It will lead to bugs where a caller is written
and tested with one build combination, and find it forgot the
conditional calling needed for a different build combination.

Any fields that we conditionally disable must already be marked
as optional in the schema, to indicate to mgmt apps that they
may or may not be present depend on what QEMU build the app is
talking to.

So if they're optional, what is wrong with generating the arguments
unconditionally and just leaving them unused/unset in builds that
don't require them ?  I think it'd be fine if the qmp_getfd API
decl in QEMU had an 'const char *wsainfo' field even on Linux
builds. The Linux impl can simply ignore it, or raise an error if
it is set.

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

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 28, 2023 at 09:58:01AM -0600, Eric Blake wrote:
>> On Wed, Feb 22, 2023 at 11:23:03AM +0100, Markus Armbruster wrote:
>> > > 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.
>> > 
>> > Piping through indent or clang-format may well give us neater results
>> > for less effort.
>> > 
>> > We might want to dumb down generator code then.
>> 
>> Indeed, this approach seems like it might be worth pursuing (our
>> generator doesn't have to worry about spacing, because we do that in a
>> second pass with something that will still produce human-legible final
>> results).
>> 
>> > >> > So I would rather assert that we don't introduce such a schema, until we
>> > >> > fix the code generator. Or we acknowledge the limitation, and treat it as a
>> > >> > schema error. Other ideas?
>> > >>
>> > >> Yes: throw an error.  Assertions are for programming errors.  This isn't
>> > >> a programming error, it's a limitation of the current implementation.
>> > >>
>> > >> How hard would it be to lift the limitation?
>> > >
>> > > Taking this as a problematic example:
>> > >
>> > > void function(first,
>> > > #ifdef A
>> > >     a,
>> > > #endif
>> > > #ifdef B
>> > >     b
>> > > #endif
>> > > )
>> 
>> I am NOT a fan of preprocessor conditionals mid-function-signature.
>> It gets really nasty, really fast.  Is there any way we can have:
>> 
>> struct S {
>> #ifdef A
>>   type a;
>> #endif
>> #ifdef B
>>   type b;
>> #endif
>> };
>> 
>> void function(struct S)
>> 
>> so that the preprocessor conditionals never appear inside ()?

Yes, there is a way: we have conditional object type members, and we
have 'boxed': true for commands.

> I'd question whether we should be doing conditional arguments
> at all.

Note that conditional arguments already exist.  This patch merely makes
them work as advertized even without 'boxed': true.

If we decide they shouldn't exist, we need to make the generator reject
them.  But what *exactly* shouldn't exist?

In my view, a command takes *one* argument (of complex type) and returns
*one* result (of any type, but only because we had to grandfather
non-complex types).

For *struct* arguments, we support a special way to pass arguments:
unboxed.  Actually used for most commands.

Support for conditional members of complex types implies support for
conditional members of (complex) argument types.

We could restrict them to boxed.  Perhaps we should.

> IMHO having an API contract that changes based on configuration
> file settings is going to be nothing but trouble. Not only does
> it make the declaration ugly, but all callers become ugly too
> with conditionals. It will lead to bugs where a caller is written
> and tested with one build combination, and find it forgot the
> conditional calling needed for a different build combination.

Two separate points: ugly and test matrix.

"Ugly" is a valid point.  Ugly in two generated places (generated
declaration and handwritten definition) is kind of bad, ugly in all
places (plus all callers) is worse.

"Test matrix" applies to conditionals anywhere, not just command
arguments.  You could perhaps argue certain conditionals are more
problematic than others.

> Any fields that we conditionally disable must already be marked
> as optional in the schema, to indicate to mgmt apps that they
> may or may not be present depend on what QEMU build the app is
> talking to.

Not quite.

Neither the code nor docs/devel/qapi-code-gen.txt require conditional
members of complex types to be optional.  qapi-schema-test.json covers
conditional mandatory members:

    { 'struct': 'TestIfStruct',
      'data': { 'foo': 'int',
--->            'bar': { 'type': 'int', 'if': 'TEST_IF_STRUCT_BAR'} },
      'if': 'TEST_IF_STRUCT' }

"Optional" isn't for indicating "that a member may or may not be present
depend on what QEMU build" via introspection, it's for indicating the
member exists in *this* QEMU build, but it need not be present on the
wire, no more, no less.

We could perhaps derive "conditional member must be optional" from our
compatibility promise, but my cold-addled brain is not up to that task
right now.

> So if they're optional, what is wrong with generating the arguments
> unconditionally and just leaving them unused/unset in builds that
> don't require them ?  I think it'd be fine if the qmp_getfd API
> decl in QEMU had an 'const char *wsainfo' field even on Linux
> builds. The Linux impl can simply ignore it, or raise an error if
> it is set.

Raise an error.  Silently ignoring is no good.

Taking a step back...  both proposed solutions are kind of ugly.  Let me
recap:

1. QMP command getfd takes an additional argument when QEMU runs on a
   Windows host.

   a. The additional argument is mandatory.

   b. The additional argument in optional, but only nominally; if
      absent, the command fails.

2. QMP comamnd getfd takes an optional argument.  If it's present and
   QEMU runs on a POSIX host, the command fails.  Likewise when it's
   absent and QEMU runs on a Windows host.

What about 3. have an additional command conditional on CONFIG_WIN32?
Existing getfd stays the same: always fails when QEMU runs on a Windows
host.  The new command exists only when QEMU runs on a Windows host.

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

Hi

On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote:
> What about 3. have an additional command conditional on CONFIG_WIN32?
> Existing getfd stays the same: always fails when QEMU runs on a Windows
> host.  The new command exists only when QEMU runs on a Windows host.

This is what was suggested initially:
https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/

I also like it better, as a specific command for windows sockets, less
ways to use it wrongly.


-- 
Marc-André Lureau

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>> What about 3. have an additional command conditional on CONFIG_WIN32?
>> Existing getfd stays the same: always fails when QEMU runs on a Windows
>> host.  The new command exists only when QEMU runs on a Windows host.

We could additionally deprecate getfd for Windows.

> This is what was suggested initially:
> https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/
>
> I also like it better, as a specific command for windows sockets, less
> ways to use it wrongly.

Daniel, what do you think?

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Daniel P. Berrangé]

On Thu, Mar 02, 2023 at 07:58:28AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > Hi
> >
> > On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote:
> >> What about 3. have an additional command conditional on CONFIG_WIN32?
> >> Existing getfd stays the same: always fails when QEMU runs on a Windows
> >> host.  The new command exists only when QEMU runs on a Windows host.
> 
> We could additionally deprecate getfd for Windows.
> 
> > This is what was suggested initially:
> > https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/
> >
> > I also like it better, as a specific command for windows sockets, less
> > ways to use it wrongly.
> 
> Daniel, what do you think?

I wasn't especially a fan of platform specific APIs, but perhaps in
retrospect it will be the lesser of two evils.

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

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Mar 02, 2023 at 07:58:28AM +0100, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> 
>> > Hi
>> >
>> > On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>> >> What about 3. have an additional command conditional on CONFIG_WIN32?
>> >> Existing getfd stays the same: always fails when QEMU runs on a Windows
>> >> host.  The new command exists only when QEMU runs on a Windows host.
>> 
>> We could additionally deprecate getfd for Windows.
>> 
>> > This is what was suggested initially:
>> > https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/
>> >
>> > I also like it better, as a specific command for windows sockets, less
>> > ways to use it wrongly.
>> 
>> Daniel, what do you think?
>
> I wasn't especially a fan of platform specific APIs, but perhaps in
> retrospect it will be the lesser of two evils.

Marc-André, let's go to your initial approach then.

This breaks the dependence between this patch and the remainder of the
series.

We still need to consider this patch anyway.  It partially fills gaps in
code generation for unboxed conditional arguments.  Partially, because
the last argument must not be conditional.  It needs work to either lift
this restriction, or reject such arguments properly.  Either way, the
generated code will be ugly, and the handwritten code interfacing with
it will be ugly, too.

I'd like to tighten the restriction instead: conditional arguments
require boxed.  Less ugly and less awkward to document, I think.

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

[...]

> I'd like to tighten the restriction instead: conditional arguments
> require boxed.  Less ugly and less awkward to document, I think.

I had a look, and realized that doing it right will be a bit more
involved than I thought, since a couple of test cases need fixing.  I'll
give it a try.

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Eric Blake]

On Tue, Feb 07, 2023 at 06:25:32PM +0400, marcandre.lureau@redhat.com wrote:
> 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.

Would it be simpler to just state that ALL commands that want to use
optional arguments must use a boxed type?  Then the function signature
is simple (a single boxed type), and the logic of the optional
parameter is now contained within the type rather than within the
function signature.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

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

Hi

On Tue, Feb 28, 2023 at 7:55 PM Eric Blake <eblake@redhat.com> wrote:

> On Tue, Feb 07, 2023 at 06:25:32PM +0400, marcandre.lureau@redhat.com
> wrote:
> > 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.
>
> Would it be simpler to just state that ALL commands that want to use
> optional arguments must use a boxed type?  Then the function signature
> is simple (a single boxed type), and the logic of the optional
> parameter is now contained within the type rather than within the
> function signature.
>
>
Possibly, but I would prefer if we leave that as a FIXME, because review
rate isn't quite fast, and it delays other patches that are pending for
months already ...

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

[PATCH v3 08/10] 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
 monitor/hmp-cmds.c |  6 +++-
 3 files changed, 81 insertions(+), 20 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/fds.c b/monitor/fds.c
index 03c5e97c35..a876e1128e 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -29,6 +29,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/ctype.h"
 #include "qemu/cutils.h"
+#include "qemu/sockets.h"
 #include "sysemu/runstate.h"
 
 /* file descriptors passed via SCM_RIGHTS */
@@ -61,36 +62,38 @@ struct MonFdset {
 static QemuMutex mon_fdsets_lock;
 static QLIST_HEAD(, MonFdset) mon_fdsets;
 
-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_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) {
         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);
+        close_fd(tmp_fd);
         return;
     }
 
@@ -98,8 +101,50 @@ void qmp_getfd(const char *fdname, Error **errp)
     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);
+}
+
+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)
@@ -120,7 +165,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;
     }
 
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 34bd8c67d7..46930bc1a9 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -197,7 +197,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);
 }
 
-- 
2.39.1

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Philippe Mathieu-Daudé]

On 7/2/23 15:25, marcandre.lureau@redhat.com wrote:
> 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
>   monitor/hmp-cmds.c |  6 +++-
>   3 files changed, 81 insertions(+), 20 deletions(-)


> +void qmp_getfd(const char *fdname,
> +#ifdef WIN32
> +               const char *wsa_info,

Rename as 'optional_b64_context' and remove #ifdef'ry?

Preferrably change qmp_getfd() prototype and use close_fd()
in a preliminary patch. Otherwise LGTM.

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

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Daniel P. Berrangé]

On Tue, Feb 07, 2023 at 06:25:33PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> A process with enough capabilities can duplicate a socket to QEMU.
> 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
>  monitor/hmp-cmds.c |  6 +++-
>  3 files changed, 81 insertions(+), 20 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.

This is a clever trick, but it also feels pretty gross from
POV of QMP design normal practice, which would be to define
a struct in QAPI to represent the WSAPROTOCOL_INFOW contents.

The main downside would be that its more verbose to convert
between the windows and QAPI structs.


> @@ -270,7 +278,11 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> +{ 'command': 'getfd', 'data': {
> +    'fdname': 'str',
> +    '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'}
> +  }
> +}

snip

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


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

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Marc-André Lureau]

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

Hi

On Tue, Feb 7, 2023 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Feb 07, 2023 at 06:25:33PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > A process with enough capabilities can duplicate a socket to QEMU.
> > 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
> >  monitor/hmp-cmds.c |  6 +++-
> >  3 files changed, 81 insertions(+), 20 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.
>
> This is a clever trick, but it also feels pretty gross from
> POV of QMP design normal practice, which would be to define
> a struct in QAPI to represent the WSAPROTOCOL_INFOW contents.
>
> The main downside would be that its more verbose to convert
> between the windows and QAPI structs.


WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved files,
it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays,
GUID, and utf16 string. QAPI-fying is going to be painful for no real gain.
It is opaque and simply given back to WSASocketW.

Markus, did you have a chance to look at the series? Can you review/comment
before I do further work?

thanks


> > @@ -270,7 +278,11 @@
> >  # <- { "return": {} }
> >  #
> >  ##
> > -{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> > +{ 'command': 'getfd', 'data': {
> > +    'fdname': 'str',
> > +    '*wsa-info': {'type': 'str', 'if': 'CONFIG_WIN32'}
> > +  }
> > +}
>
> snip
>
> > +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;
> > +        }
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

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

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[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.
> 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
>  monitor/hmp-cmds.c |  6 +++-
>  3 files changed, 81 insertions(+), 20 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.

The Windows part explains things in terms of the C socket API.  Less
than ideal for the QEMU QMP Reference Manual, isn't it?  I don't know
nearly enough about this stuff to suggest concrete improvements...

What does this command do under Windows before this patch?  Fail always?

Wrap your lines a bit earlier, please.

>  #
>  # @fdname: file descriptor name
>  #
> +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0.
> +#

No way around passing a binary blob?

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

What happens when QEMU runs on a Windows host and the client doesn't
pass @wsa-info?

>  
>  ##
>  # @closefd:

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Marc-André Lureau]

Hi Markus

On Fri, Feb 17, 2023 at 1: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.
> > 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
> >  monitor/hmp-cmds.c |  6 +++-
> >  3 files changed, 81 insertions(+), 20 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.
>
> The Windows part explains things in terms of the C socket API.  Less
> than ideal for the QEMU QMP Reference Manual, isn't it?  I don't know
> nearly enough about this stuff to suggest concrete improvements...

We don't have to, after all we don't explain how to use sendmsg/cmsg
stuff to pass FDs.

I will drop the part about "A SOCKET is considered as a kind of "file
descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not
mix SOCKET and fd space"
(https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/)
merged.


>
> What does this command do under Windows before this patch?  Fail always?

Without ancillary data support on Windows, you can't make it work.

>
> Wrap your lines a bit earlier, please.
>
> >  #
> >  # @fdname: file descriptor name
> >  #
> > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0.
> > +#
>
> No way around passing a binary blob?


WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields,
it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays,
GUID, and utf16 string.

QAPI'fying that structure back and forth would be tedious and
error-prone. Better to treat it as an opaque blob imho.



>
> >  # 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'}
> > +  }
> > +}
>
> What happens when QEMU runs on a Windows host and the client doesn't
> pass @wsa-info?

It attempts to get the fd from the last recv, but it will fail on
Windows, this is not available.



-- 
Marc-André Lureau

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi Markus
>
> On Fri, Feb 17, 2023 at 1: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.
>> > 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
>> >  monitor/hmp-cmds.c |  6 +++-
>> >  3 files changed, 81 insertions(+), 20 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.
>>
>> The Windows part explains things in terms of the C socket API.  Less
>> than ideal for the QEMU QMP Reference Manual, isn't it?  I don't know
>> nearly enough about this stuff to suggest concrete improvements...
>
> We don't have to, after all we don't explain how to use sendmsg/cmsg
> stuff to pass FDs.
>
> I will drop the part about "A SOCKET is considered as a kind of "file
> descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not
> mix SOCKET and fd space"
> (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/)
> merged.

Would it make sense to rebase this series on top of that one, so we
can have simpler documentation from the start?

>> What does this command do under Windows before this patch?  Fail always?
>
> Without ancillary data support on Windows, you can't make it work.

Yes, but how does it fail?  Hmm, you actually answer that below.

>> Wrap your lines a bit earlier, please.
>>
>> >  #
>> >  # @fdname: file descriptor name
>> >  #
>> > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0.
>> > +#
>>
>> No way around passing a binary blob?
>
> WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields,
> it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays,
> GUID, and utf16 string.
>
> QAPI'fying that structure back and forth would be tedious and
> error-prone. Better to treat it as an opaque blob imho.

I worry about potential consequences of baking Windows ABI into QMP.

What if the memory representation of this struct changes?

Such ABI changes are unpleasant, but they are not impossible.

>> >  # 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'}
>> > +  }
>> > +}
>>
>> What happens when QEMU runs on a Windows host and the client doesn't
>> pass @wsa-info?
>
> It attempts to get the fd from the last recv, but it will fail on
> Windows, this is not available.

So it fails exactly like it fails on a POSIX host when you execute getfd
without passing along a file descriptor with SCM_RIGHTS.  Correct?

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Daniel P. Berrangé]

On Mon, Feb 20, 2023 at 09:26:24AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > Hi Markus
> >
> > On Fri, Feb 17, 2023 at 1: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.
> >> > 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
> >> >  monitor/hmp-cmds.c |  6 +++-
> >> >  3 files changed, 81 insertions(+), 20 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.
> >>
> >> The Windows part explains things in terms of the C socket API.  Less
> >> than ideal for the QEMU QMP Reference Manual, isn't it?  I don't know
> >> nearly enough about this stuff to suggest concrete improvements...
> >
> > We don't have to, after all we don't explain how to use sendmsg/cmsg
> > stuff to pass FDs.
> >
> > I will drop the part about "A SOCKET is considered as a kind of "file
> > descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not
> > mix SOCKET and fd space"
> > (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/)
> > merged.
> 
> Would it make sense to rebase this series on top of that one, so we
> can have simpler documentation from the start?
> 
> >> What does this command do under Windows before this patch?  Fail always?
> >
> > Without ancillary data support on Windows, you can't make it work.
> 
> Yes, but how does it fail?  Hmm, you actually answer that below.
> 
> >> Wrap your lines a bit earlier, please.
> >>
> >> >  #
> >> >  # @fdname: file descriptor name
> >> >  #
> >> > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0.
> >> > +#
> >>
> >> No way around passing a binary blob?
> >
> > WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields,
> > it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays,
> > GUID, and utf16 string.
> >
> > QAPI'fying that structure back and forth would be tedious and
> > error-prone. Better to treat it as an opaque blob imho.
> 
> I worry about potential consequences of baking Windows ABI into QMP.
> 
> What if the memory representation of this struct changes?
> 
> Such ABI changes are unpleasant, but they are not impossible.

IIUC, the Windows API aims to be append only. So any need to change
this struct would instead result in creating a new struct + new
corresponding API.

FWIW, there's also a WSAPROTOCOL_INFOA version of this struct which
has an ascii string instead of utf16 string.

I'm not especially happy about encoding a struct as a blob either,
but in this case I'm coming around to the view that it is probably
the least worst option.

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

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Marc-André Lureau]

Hi

On Mon, Feb 20, 2023 at 12:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi Markus
> >
> > On Fri, Feb 17, 2023 at 1: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.
> >> > 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
> >> >  monitor/hmp-cmds.c |  6 +++-
> >> >  3 files changed, 81 insertions(+), 20 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.
> >>
> >> The Windows part explains things in terms of the C socket API.  Less
> >> than ideal for the QEMU QMP Reference Manual, isn't it?  I don't know
> >> nearly enough about this stuff to suggest concrete improvements...
> >
> > We don't have to, after all we don't explain how to use sendmsg/cmsg
> > stuff to pass FDs.
> >
> > I will drop the part about "A SOCKET is considered as a kind of "file
> > descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not
> > mix SOCKET and fd space"
> > (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/)
> > merged.
>
> Would it make sense to rebase this series on top of that one, so we
> can have simpler documentation from the start?

Sure, if only I had more reviews/acks...


>
> >> What does this command do under Windows before this patch?  Fail always?
> >
> > Without ancillary data support on Windows, you can't make it work.
>
> Yes, but how does it fail?  Hmm, you actually answer that below.
>
> >> Wrap your lines a bit earlier, please.
> >>
> >> >  #
> >> >  # @fdname: file descriptor name
> >> >  #
> >> > +# @wsa-info: a WSAPROTOCOL_INFOW structure (encoded in base64). Since 8.0.
> >> > +#
> >>
> >> No way around passing a binary blob?
> >
> > WSAPROTOCOL_INFOW is a fairly big structure, with private/reserved fields,
> > it contains another structure (WSAPROTOCOLCHAIN), has fixed-length arrays,
> > GUID, and utf16 string.
> >
> > QAPI'fying that structure back and forth would be tedious and
> > error-prone. Better to treat it as an opaque blob imho.
>
> I worry about potential consequences of baking Windows ABI into QMP.
>
> What if the memory representation of this struct changes?
>
> Such ABI changes are unpleasant, but they are not impossible.

This is unlikely, the API users are typically sharing that structure
between processes since it was introduced, back in 2000. (see also
Daniel reply)

>
> >> >  # 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'}
> >> > +  }
> >> > +}
> >>
> >> What happens when QEMU runs on a Windows host and the client doesn't
> >> pass @wsa-info?
> >
> > It attempts to get the fd from the last recv, but it will fail on
> > Windows, this is not available.
>
> So it fails exactly like it fails on a POSIX host when you execute getfd
> without passing along a file descriptor with SCM_RIGHTS.  Correct?

Correct, I get something like:
Error { class: GenericError, desc: "No file descriptor supplied via
SCM_RIGHTS", id: None }

-- 
Marc-André Lureau

Re: [PATCH v3 08/10] qmp: teach 'getfd' to import sockets on win32

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Feb 20, 2023 at 12:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>
>> > Hi Markus
>> >
>> > On Fri, Feb 17, 2023 at 1: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.
>> >> > 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/fds.c      | 79 ++++++++++++++++++++++++++++++++++++----------
>> >> >  monitor/hmp-cmds.c |  6 +++-
>> >> >  3 files changed, 81 insertions(+), 20 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.
>> >>
>> >> The Windows part explains things in terms of the C socket API.  Less
>> >> than ideal for the QEMU QMP Reference Manual, isn't it?  I don't know
>> >> nearly enough about this stuff to suggest concrete improvements...
>> >
>> > We don't have to, after all we don't explain how to use sendmsg/cmsg
>> > stuff to pass FDs.
>> >
>> > I will drop the part about "A SOCKET is considered as a kind of "file
>> > descriptor" in QMP context", after we get "[PATCH 0/4] win32: do not
>> > mix SOCKET and fd space"
>> > (https://patchew.org/QEMU/20230212204942.1905959-1-marcandre.lureau@redhat.com/)
>> > merged.
>>
>> Would it make sense to rebase this series on top of that one, so we
>> can have simpler documentation from the start?
>
> Sure, if only I had more reviews/acks...

I'll try to have a look.

>> >> What does this command do under Windows before this patch?  Fail always?
>> >
>> > Without ancillary data support on Windows, you can't make it work.
>>
>> Yes, but how does it fail?  Hmm, you actually answer that below.

[...]

>> >> >  # 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'}
>> >> > +  }
>> >> > +}
>> >>
>> >> What happens when QEMU runs on a Windows host and the client doesn't
>> >> pass @wsa-info?
>> >
>> > It attempts to get the fd from the last recv, but it will fail on
>> > Windows, this is not available.
>>
>> So it fails exactly like it fails on a POSIX host when you execute getfd
>> without passing along a file descriptor with SCM_RIGHTS.  Correct?
>
> Correct, I get something like:
> Error { class: GenericError, desc: "No file descriptor supplied via
> SCM_RIGHTS", id: None }

Works for me, thanks!

[PATCH v3 09/10] 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 d658222a19..30177ea784 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1460,13 +1460,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"));
@@ -1480,7 +1493,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

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

[Philippe Mathieu-Daudé]

On 7/2/23 15:25, marcandre.lureau@redhat.com wrote:
> 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(-)

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

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

[marcandre.lureau]

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

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

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

diff --git a/tests/qtest/vnc-display-test.c b/tests/qtest/vnc-display-test.c
index e52a4326ec..2c47db8d4c 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
-- 
2.39.1

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

[Philippe Mathieu-Daudé]

On 7/2/23 15:25, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Now that qtest_qmp_add_client() works on win32, we can enable the VNC
> test.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qtest/vnc-display-test.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

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