* [PATCH v4 00/11] QMP command to import win32 sockets
@ 2023-03-06 12:27 marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 01/11] tests: fix path separator, use g_build_filename() marcandre.lureau
` (10 more replies)
0 siblings, 11 replies; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Hi,
The series focuses on 'add_client' win32 support, by limiting its scope to
sockets and adding a new command to import sockets. This enables vnc-display
test on win32, exercising the new code paths.
(a follow up series will add dbus display support on win32, with tests using
this socket import method)
v4:
- back to new get-win32-socket command, as in v1
- drop qapi conditional fixes
- add "QMP/HMP: only actually implement getfd on CONFIG_POSIX"
- add "qapi/gen: run C code through clang-format, if possible"
- rebased on pending fd/socket mix series
v3:
- drop "tests: fix test-io-channel-command on win32", not good enough
- include "char: do not double-close fd when failing to add client"
- add "monitor: release the lock before calling close()"
- rebase after recent QMP code move
v2:
- replace the propose new command in v1, with 'wsa-info' argument in 'getfd'
- fix qapi/qmp for commands/events with optional arguments
- rebase, and tags
Based-on: <20230221124802.4103554-1-marcandre.lureau@redhat.com>
("[PATCH v3 00/16] win32: do not mix SOCKET and fd space")
Marc-André Lureau (11):
tests: fix path separator, use g_build_filename()
char: do not double-close fd when failing to add client
tests/docker: fix a win32 error due to portability
osdep: implement qemu_socketpair() for win32
qmp: 'add_client' actually expects sockets
monitor: release the lock before calling close()
qapi/gen: run C code through clang-format, if possible
qmp: add 'get-win32-socket'
libqtest: make qtest_qmp_add_client work on win32
qtest: enable vnc-display test on win32
QMP/HMP: only actually implement getfd on CONFIG_POSIX
qapi/misc.json | 35 ++++++++-
include/qemu/sockets.h | 2 -
tests/qtest/libqtest.h | 5 +-
chardev/char.c | 2 -
monitor/fds.c | 77 +++++++++++++++----
monitor/hmp-cmds.c | 2 +
monitor/qmp-cmds.c | 7 ++
tests/qtest/libqtest.c | 18 ++++-
tests/qtest/vnc-display-test.c | 12 +--
tests/unit/test-io-channel-command.c | 2 +-
util/oslib-win32.c | 110 +++++++++++++++++++++++++++
hmp-commands.hx | 2 +
scripts/qapi/gen.py | 15 +++-
scripts/qapi/introspect.py | 2 +
tests/docker/docker.py | 6 +-
15 files changed, 263 insertions(+), 34 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v4 01/11] tests: fix path separator, use g_build_filename()
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 02/11] char: do not double-close fd when failing to add client marcandre.lureau
` (9 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
tests/unit/test-io-channel-command.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/unit/test-io-channel-command.c b/tests/unit/test-io-channel-command.c
index c6e66a8c33..4f022617df 100644
--- a/tests/unit/test-io-channel-command.c
+++ b/tests/unit/test-io-channel-command.c
@@ -35,7 +35,7 @@ static char *socat = NULL;
static void test_io_channel_command_fifo(bool async)
{
g_autofree gchar *tmpdir = g_dir_make_tmp("qemu-test-io-channel.XXXXXX", NULL);
- g_autofree gchar *fifo = g_strdup_printf("%s/%s", tmpdir, TEST_FIFO);
+ g_autofree gchar *fifo = g_build_filename(tmpdir, TEST_FIFO, NULL);
g_autofree gchar *srcargs = g_strdup_printf("%s - PIPE:%s,wronly", socat, fifo);
g_autofree gchar *dstargs = g_strdup_printf("%s PIPE:%s,rdonly -", socat, fifo);
g_auto(GStrv) srcargv = g_strsplit(srcargs, " ", -1);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 02/11] char: do not double-close fd when failing to add client
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 01/11] tests: fix path separator, use g_build_filename() marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 03/11] tests/docker: fix a win32 error due to portability marcandre.lureau
` (8 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
The caller is already closing the fd on failure.
Fixes: c3054a6e6a ("char: Factor out qmp_add_client() parts and move to chardev/")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
chardev/char.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index 11eab7764c..e69390601f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1175,12 +1175,10 @@ bool qmp_add_client_char(int fd, bool has_skipauth, bool skipauth,
if (!s) {
error_setg(errp, "protocol '%s' is invalid", protocol);
- close(fd);
return false;
}
if (qemu_chr_add_client(s, fd) < 0) {
error_setg(errp, "failed to add client");
- close(fd);
return false;
}
return true;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 03/11] tests/docker: fix a win32 error due to portability
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 01/11] tests: fix path separator, use g_build_filename() marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 02/11] char: do not double-close fd when failing to add client marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32 marcandre.lureau
` (7 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
docker.py is run during configure, and produces an error: No module
named 'pwd'.
Use a more portable and recommended alternative to lookup the user
"login name".
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
tests/docker/docker.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 3a1ed7cb18..688ef62989 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -23,10 +23,10 @@
import tempfile
import re
import signal
+import getpass
from tarfile import TarFile, TarInfo
from io import StringIO, BytesIO
from shutil import copy, rmtree
-from pwd import getpwuid
from datetime import datetime, timedelta
@@ -316,7 +316,7 @@ def build_image(self, tag, docker_dir, dockerfile,
if user:
uid = os.getuid()
- uname = getpwuid(uid).pw_name
+ uname = getpass.getuser()
tmp_df.write("\n")
tmp_df.write("RUN id %s 2>/dev/null || useradd -u %d -U %s" %
(uname, uid, uname))
@@ -570,7 +570,7 @@ def run(self, args, argv):
if args.user:
uid = os.getuid()
- uname = getpwuid(uid).pw_name
+ uname = getpass.getuser()
df.write("\n")
df.write("RUN id %s 2>/dev/null || useradd -u %d -U %s" %
(uname, uid, uname))
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (2 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 03/11] tests/docker: fix a win32 error due to portability marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-07 14:50 ` Daniel P. Berrangé
2023-03-06 12:27 ` [PATCH v4 05/11] qmp: 'add_client' actually expects sockets marcandre.lureau
` (6 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Manually implement a socketpair() function, using UNIX sockets and
simple peer credential checking.
QEMU doesn't make much use of socketpair, beside vhost-user which is not
available for win32 at this point. However, I intend to use it for
writing some new portable tests.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/qemu/sockets.h | 2 -
util/oslib-win32.c | 110 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 110 insertions(+), 2 deletions(-)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 2b0698a7c9..d935fd80da 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -15,7 +15,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
bool fd_is_socket(int fd);
int qemu_socket(int domain, int type, int protocol);
-#ifndef WIN32
/**
* qemu_socketpair:
* @domain: specifies a communication domain, such as PF_UNIX
@@ -30,7 +29,6 @@ int qemu_socket(int domain, int type, int protocol);
* Return 0 on success.
*/
int qemu_socketpair(int domain, int type, int protocol, int sv[2]);
-#endif
int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
/*
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 29a667ae3d..16f8a67f7e 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
return qemu_socket_select(sockfd, NULL, 0, errp);
}
+int qemu_socketpair(int domain, int type, int protocol, int sv[2])
+{
+ struct sockaddr_un addr = {
+ 0,
+ };
+ socklen_t socklen;
+ int listener = -1;
+ int client = -1;
+ int server = -1;
+ g_autofree char *path = NULL;
+ int tmpfd;
+ u_long arg;
+ int ret = -1;
+
+ g_return_val_if_fail(sv != NULL, -1);
+
+ addr.sun_family = AF_UNIX;
+ socklen = sizeof(addr);
+
+ tmpfd = g_file_open_tmp(NULL, &path, NULL);
+ if (tmpfd == -1 || !path) {
+ errno = EACCES;
+ goto out;
+ }
+
+ close(tmpfd);
+
+ if (strlen(path) >= sizeof(addr.sun_path)) {
+ errno = EINVAL;
+ goto out;
+ }
+
+ strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
+
+ listener = socket(domain, type, protocol);
+ if (listener == -1) {
+ goto out;
+ }
+
+ if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
+ errno = EACCES;
+ goto out;
+ }
+ g_clear_pointer(&path, g_free);
+
+ if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
+ goto out;
+ }
+
+ if (listen(listener, 1) == -1) {
+ goto out;
+ }
+
+ client = socket(domain, type, protocol);
+ if (client == -1) {
+ goto out;
+ }
+
+ arg = 1;
+ if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
+ goto out;
+ }
+
+ if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
+ WSAGetLastError() != WSAEWOULDBLOCK) {
+ goto out;
+ }
+
+ server = accept(listener, NULL, NULL);
+ if (server == -1) {
+ goto out;
+ }
+
+ arg = 0;
+ if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
+ goto out;
+ }
+
+ arg = 0;
+ if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
+ goto out;
+ }
+
+ if (arg != GetCurrentProcessId()) {
+ errno = EPERM;
+ goto out;
+ }
+
+ sv[0] = server;
+ server = -1;
+ sv[1] = client;
+ client = -1;
+ ret = 0;
+
+out:
+ if (listener != -1) {
+ close(listener);
+ }
+ if (client != -1) {
+ close(client);
+ }
+ if (server != -1) {
+ close(server);
+ }
+ if (path) {
+ DeleteFile(path);
+ }
+ return ret;
+}
+
#undef connect
int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
socklen_t addrlen)
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 05/11] qmp: 'add_client' actually expects sockets
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (3 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32 marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 15:02 ` Markus Armbruster
2023-03-06 12:27 ` [PATCH v4 06/11] monitor: release the lock before calling close() marcandre.lureau
` (5 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Whether it is SPICE, VNC, D-Bus, or the socket chardev, they all
actually expect a socket kind or will fail in different ways at runtime.
Throw an error early if the given 'add_client' fd is not a socket, and
close it to avoid leaks.
This allows to replace the close() call with a more correct & portable
closesocket() version.
(this will allow importing sockets on Windows with a specialized command
in the following patch, while keeping the remaining monitor associated
sockets/add_client code & usage untouched)
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
qapi/misc.json | 3 +++
monitor/qmp-cmds.c | 7 +++++++
2 files changed, 10 insertions(+)
diff --git a/qapi/misc.json b/qapi/misc.json
index 27ef5a2b20..f0217cfba0 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -14,6 +14,9 @@
# Allow client connections for VNC, Spice and socket based
# character devices to be passed in to QEMU via SCM_RIGHTS.
#
+# If the FD associated with @fdname is not a socket, the command will fail and
+# the FD will be closed.
+#
# @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
# the name of a character device (eg. from -chardev id=XXXX)
#
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 859012aef4..9f7751beeb 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -14,6 +14,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/sockets.h"
#include "monitor-internal.h"
#include "monitor/qdev.h"
#include "monitor/qmp-helpers.h"
@@ -139,6 +140,12 @@ void qmp_add_client(const char *protocol, const char *fdname,
return;
}
+ if (!fd_is_socket(fd)) {
+ error_setg(errp, "add_client expects a socket");
+ close(fd);
+ return;
+ }
+
for (i = 0; i < ARRAY_SIZE(protocol_table); i++) {
if (!strcmp(protocol, protocol_table[i].name)) {
if (!protocol_table[i].add_client(fd, has_skipauth, skipauth,
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 06/11] monitor: release the lock before calling close()
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (4 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 05/11] qmp: 'add_client' actually expects sockets marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 15:29 ` Markus Armbruster
2023-03-06 12:27 ` [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible marcandre.lureau
` (4 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
As per comment, presumably to avoid syscall in critical section.
Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
monitor/fds.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/monitor/fds.c b/monitor/fds.c
index 26b39a0ce6..7daf1064e1 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -80,7 +80,8 @@ void qmp_getfd(const char *fdname, Error **errp)
return;
}
- QEMU_LOCK_GUARD(&cur_mon->mon_lock);
+ /* See close() call below. */
+ qemu_mutex_lock(&cur_mon->mon_lock);
QLIST_FOREACH(monfd, &cur_mon->fds, next) {
if (strcmp(monfd->name, fdname) != 0) {
continue;
@@ -88,6 +89,7 @@ void qmp_getfd(const char *fdname, Error **errp)
tmp_fd = monfd->fd;
monfd->fd = fd;
+ qemu_mutex_unlock(&cur_mon->mon_lock);
/* Make sure close() is outside critical section */
close(tmp_fd);
return;
@@ -98,6 +100,7 @@ void qmp_getfd(const char *fdname, Error **errp)
monfd->fd = fd;
QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
+ qemu_mutex_unlock(&cur_mon->mon_lock);
}
void qmp_closefd(const char *fdname, Error **errp)
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (5 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 06/11] monitor: release the lock before calling close() marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 16:02 ` Markus Armbruster
2023-03-06 16:05 ` Peter Maydell
2023-03-06 12:27 ` [PATCH v4 08/11] qmp: add 'get-win32-socket' marcandre.lureau
` (3 subsequent siblings)
10 siblings, 2 replies; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Make the resulting code even prettier, if possible.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/qapi/gen.py | 15 ++++++++++++++-
scripts/qapi/introspect.py | 2 ++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b5a8d03e8e..c0ec9aa412 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -14,6 +14,7 @@
from contextlib import contextmanager
import os
import re
+import subprocess
from typing import (
Dict,
Iterator,
@@ -133,6 +134,7 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
class QAPIGenCCode(QAPIGen):
def __init__(self, fname: str):
super().__init__(fname)
+ self.skip_format: bool = False
self._start_if: Optional[Tuple[QAPISchemaIfCond, str, str]] = None
def start_if(self, ifcond: QAPISchemaIfCond) -> None:
@@ -149,7 +151,18 @@ def end_if(self) -> None:
def get_content(self) -> str:
assert self._start_if is None
- return super().get_content()
+
+ text = super().get_content()
+ if not self.skip_format:
+ try:
+ text = subprocess.run(["clang-format"],
+ input=text,
+ text=True,
+ capture_output=True,
+ check=True).stdout
+ except FileNotFoundError:
+ pass
+ return text
class QAPIGenC(QAPIGenCCode):
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae..1a8cac37ef 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -174,6 +174,8 @@ def __init__(self, prefix: str, unmask: bool):
super().__init__(
prefix, 'qapi-introspect',
' * QAPI/QMP schema introspection', __doc__)
+ # for some reasons, the generated code is making clang-format go crazy
+ self._genc.skip_format = True
self._unmask = unmask
self._schema: Optional[QAPISchema] = None
self._trees: List[Annotated[SchemaInfo]] = []
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 08/11] qmp: add 'get-win32-socket'
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (6 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 15:47 ` Markus Armbruster
2023-03-06 12:27 ` [PATCH v4 09/11] libqtest: make qtest_qmp_add_client work on win32 marcandre.lureau
` (2 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
A process with enough capabilities can duplicate a socket to QEMU. Add a
QMP command to import it and add it to the monitor fd list, so it can be
later used by other commands.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qapi/misc.json | 30 ++++++++++++++++++++
monitor/fds.c | 76 +++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 90 insertions(+), 16 deletions(-)
diff --git a/qapi/misc.json b/qapi/misc.json
index f0217cfba0..031c94050c 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -275,6 +275,36 @@
##
{ 'command': 'getfd', 'data': {'fdname': 'str'} }
+##
+# @get-win32-socket:
+#
+# Add a socket that was duplicated to QEMU process with WSADuplicateSocketW()
+# via WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name (the SOCKET
+# is associated with a CRT file descriptor)
+#
+# @info: the WSAPROTOCOL_INFOW structure (encoded in base64)
+#
+# @fdname: file descriptor name
+#
+# Returns: Nothing on success
+#
+# Since: 8.0
+#
+# Notes: If @fdname already exists, the file descriptor assigned to
+# it will be closed and replaced by the received file
+# descriptor.
+#
+# The 'closefd' command can be used to explicitly close the
+# file descriptor when it is no longer needed.
+#
+# Example:
+#
+# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", fdname": "skclient" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 'if': 'CONFIG_WIN32' }
+
##
# @closefd:
#
diff --git a/monitor/fds.c b/monitor/fds.c
index 7daf1064e1..9ed4197358 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -61,46 +61,55 @@ struct MonFdset {
static QemuMutex mon_fdsets_lock;
static QLIST_HEAD(, MonFdset) mon_fdsets;
-void qmp_getfd(const char *fdname, Error **errp)
+static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp)
{
- Monitor *cur_mon = monitor_cur();
mon_fd_t *monfd;
- int fd, tmp_fd;
-
- fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
- if (fd == -1) {
- error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
- return;
- }
if (qemu_isdigit(fdname[0])) {
close(fd);
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
"a name not starting with a digit");
- return;
+ return false;
}
/* See close() call below. */
- qemu_mutex_lock(&cur_mon->mon_lock);
- QLIST_FOREACH(monfd, &cur_mon->fds, next) {
+ qemu_mutex_lock(&mon->mon_lock);
+ QLIST_FOREACH(monfd, &mon->fds, next) {
+ int tmp_fd;
+
if (strcmp(monfd->name, fdname) != 0) {
continue;
}
tmp_fd = monfd->fd;
monfd->fd = fd;
- qemu_mutex_unlock(&cur_mon->mon_lock);
+ qemu_mutex_unlock(&mon->mon_lock);
/* Make sure close() is outside critical section */
close(tmp_fd);
- return;
+ return true;
}
monfd = g_new0(mon_fd_t, 1);
monfd->name = g_strdup(fdname);
monfd->fd = fd;
- QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
- qemu_mutex_unlock(&cur_mon->mon_lock);
+ QLIST_INSERT_HEAD(&mon->fds, monfd, next);
+ qemu_mutex_unlock(&mon->mon_lock);
+ return true;
+}
+
+void qmp_getfd(const char *fdname, Error **errp)
+{
+ Monitor *cur_mon = monitor_cur();
+ int fd;
+
+ fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
+ if (fd == -1) {
+ error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
+ return;
+ }
+
+ monitor_add_fd(cur_mon, fd, fdname, errp);
}
void qmp_closefd(const char *fdname, Error **errp)
@@ -214,6 +223,41 @@ error:
return NULL;
}
+#ifdef WIN32
+void qmp_get_win32_socket(const char *infos, const char *fdname, Error **errp)
+{
+ g_autofree WSAPROTOCOL_INFOW *info = NULL;
+ gsize len;
+ SOCKET sk;
+ int fd;
+
+ info = (void *)g_base64_decode(infos, &len);
+ if (len != sizeof(*info)) {
+ error_setg(errp, "Invalid WSAPROTOCOL_INFOW value");
+ return;
+ }
+
+ sk = WSASocketW(FROM_PROTOCOL_INFO,
+ FROM_PROTOCOL_INFO,
+ FROM_PROTOCOL_INFO,
+ info, 0, 0);
+ if (sk == INVALID_SOCKET) {
+ error_setg_win32(errp, WSAGetLastError(), "Couldn't import socket");
+ return;
+ }
+
+ fd = _open_osfhandle(sk, _O_BINARY);
+ if (fd < 0) {
+ error_setg_errno(errp, errno, "Failed to associate a FD with the SOCKET");
+ closesocket(sk);
+ return;
+ }
+
+ monitor_add_fd(monitor_cur(), fd, fdname, errp);
+}
+#endif
+
+
void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
{
MonFdset *mon_fdset;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 09/11] libqtest: make qtest_qmp_add_client work on win32
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (7 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 08/11] qmp: add 'get-win32-socket' marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-07 14:54 ` Daniel P. Berrangé
2023-03-06 12:27 ` [PATCH v4 10/11] qtest: enable vnc-display test " marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX marcandre.lureau
10 siblings, 1 reply; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Use the "get-win32-socket" function to pass an opened socket to QEMU,
instead of using "getfd", which relies on socket ancillary FD message
passing.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
tests/qtest/libqtest.h | 5 ++---
tests/qtest/libqtest.c | 18 ++++++++++++++++--
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index fcf1c3c3b3..8d7d450963 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -758,17 +758,16 @@ void qtest_qmp_device_add_qdict(QTestState *qts, const char *drv,
void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
const char *fmt, ...) G_GNUC_PRINTF(4, 5);
-#ifndef _WIN32
/**
* qtest_qmp_add_client:
* @qts: QTestState instance to operate on
* @protocol: the protocol to add to
* @fd: the client file-descriptor
*
- * Call QMP ``getfd`` followed by ``add_client`` with the given @fd.
+ * Call QMP ``getfd`` (on Windows ``get-win32-socket``) followed by
+ * ``add_client`` with the given @fd.
*/
void qtest_qmp_add_client(QTestState *qts, const char *protocol, int fd);
-#endif /* _WIN32 */
/**
* qtest_qmp_device_del_send:
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index dee2032331..c3a0ef5bb4 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1478,13 +1478,28 @@ void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
qobject_unref(args);
}
-#ifndef _WIN32
void qtest_qmp_add_client(QTestState *qts, const char *protocol, int fd)
{
QDict *resp;
+#ifdef WIN32
+ WSAPROTOCOL_INFOW info;
+ g_autofree char *info64 = NULL;
+ SOCKET s;
+
+ assert(fd_is_socket(fd));
+ s = _get_osfhandle(fd);
+ if (WSADuplicateSocketW(s, GetProcessId((HANDLE)qts->qemu_pid), &info) == SOCKET_ERROR) {
+ g_autofree char *emsg = g_win32_error_message(WSAGetLastError());
+ g_error("WSADuplicateSocketW failed: %s", emsg);
+ }
+ info64 = g_base64_encode((guchar *)&info, sizeof(info));
+ resp = qtest_qmp(qts, "{'execute': 'get-win32-socket',"
+ "'arguments': {'fdname': 'fdname', 'info': %s}}", info64);
+#else
resp = qtest_qmp_fds(qts, &fd, 1, "{'execute': 'getfd',"
"'arguments': {'fdname': 'fdname'}}");
+#endif
g_assert(resp);
g_assert(!qdict_haskey(resp, "event")); /* We don't expect any events */
g_assert(!qdict_haskey(resp, "error"));
@@ -1498,7 +1513,6 @@ void qtest_qmp_add_client(QTestState *qts, const char *protocol, int fd)
g_assert(!qdict_haskey(resp, "error"));
qobject_unref(resp);
}
-#endif
/*
* Generic hot-unplugging test via the device_del QMP command.
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 10/11] qtest: enable vnc-display test on win32
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (8 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 09/11] libqtest: make qtest_qmp_add_client work on win32 marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX marcandre.lureau
10 siblings, 0 replies; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Now that qtest_qmp_add_client() works on win32, we can enable the VNC
test.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
tests/qtest/vnc-display-test.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/vnc-display-test.c b/tests/qtest/vnc-display-test.c
index e52a4326ec..f8933b0761 100644
--- a/tests/qtest/vnc-display-test.c
+++ b/tests/qtest/vnc-display-test.c
@@ -19,7 +19,7 @@ typedef struct Test {
GMainLoop *loop;
} Test;
-#if !defined(WIN32) && !defined(CONFIG_DARWIN)
+#if !defined(CONFIG_DARWIN)
static void on_vnc_error(VncConnection* self,
const char* msg)
@@ -38,10 +38,7 @@ static void on_vnc_auth_failure(VncConnection *self,
static bool
test_setup(Test *test)
{
-#ifdef WIN32
- g_test_skip("Not supported on Windows yet");
- return false;
-#elif defined(CONFIG_DARWIN)
+#if defined(CONFIG_DARWIN)
g_test_skip("Broken on Darwin");
return false;
#else
@@ -59,7 +56,12 @@ test_setup(Test *test)
g_signal_connect(test->conn, "vnc-auth-failure",
G_CALLBACK(on_vnc_auth_failure), NULL);
vnc_connection_set_auth_type(test->conn, VNC_CONNECTION_AUTH_NONE);
+
+#ifdef WIN32
+ vnc_connection_open_fd(test->conn, _get_osfhandle(pair[0]));
+#else
vnc_connection_open_fd(test->conn, pair[0]);
+#endif
test->loop = g_main_loop_new(NULL, FALSE);
return true;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
` (9 preceding siblings ...)
2023-03-06 12:27 ` [PATCH v4 10/11] qtest: enable vnc-display test " marcandre.lureau
@ 2023-03-06 12:27 ` marcandre.lureau
2023-03-06 16:01 ` Markus Armbruster
10 siblings, 1 reply; 28+ messages in thread
From: marcandre.lureau @ 2023-03-06 12:27 UTC (permalink / raw
To: qemu-devel
Cc: Thomas Huth, Gerd Hoffmann, Alex Bennée, Michael Roth,
Marc-André Lureau, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Currently, the function will simply fail if ancillary fds are not
provided, for ex on unsupported platforms.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qapi/misc.json | 2 +-
monitor/fds.c | 2 ++
monitor/hmp-cmds.c | 2 ++
hmp-commands.hx | 2 ++
4 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/qapi/misc.json b/qapi/misc.json
index 031c94050c..96c053e305 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -273,7 +273,7 @@
# <- { "return": {} }
#
##
-{ 'command': 'getfd', 'data': {'fdname': 'str'} }
+{ 'command': 'getfd', 'data': {'fdname': 'str'}, 'if': 'CONFIG_POSIX' }
##
# @get-win32-socket:
diff --git a/monitor/fds.c b/monitor/fds.c
index 9ed4197358..d86c2c674c 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -98,6 +98,7 @@ static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **err
return true;
}
+#ifdef CONFIG_POSIX
void qmp_getfd(const char *fdname, Error **errp)
{
Monitor *cur_mon = monitor_cur();
@@ -111,6 +112,7 @@ void qmp_getfd(const char *fdname, Error **errp)
monitor_add_fd(cur_mon, fd, fdname, errp);
}
+#endif
void qmp_closefd(const char *fdname, Error **errp)
{
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 34bd8c67d7..6c559b48c8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -192,6 +192,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, err);
}
+#ifdef CONFIG_POSIX
void hmp_getfd(Monitor *mon, const QDict *qdict)
{
const char *fdname = qdict_get_str(qdict, "fdname");
@@ -200,6 +201,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict)
qmp_getfd(fdname, &err);
hmp_handle_error(mon, err);
}
+#endif
void hmp_closefd(Monitor *mon, const QDict *qdict)
{
diff --git a/hmp-commands.hx b/hmp-commands.hx
index b87c250e23..bb85ee1d26 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1486,6 +1486,7 @@ SRST
Inject an MCE on the given CPU (x86 only).
ERST
+#ifdef CONFIG_POSIX
{
.name = "getfd",
.args_type = "fdname:s",
@@ -1501,6 +1502,7 @@ SRST
mechanism on unix sockets, it is stored using the name *fdname* for
later use by other monitor commands.
ERST
+#endif
{
.name = "closefd",
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v4 05/11] qmp: 'add_client' actually expects sockets
2023-03-06 12:27 ` [PATCH v4 05/11] qmp: 'add_client' actually expects sockets marcandre.lureau
@ 2023-03-06 15:02 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2023-03-06 15:02 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Whether it is SPICE, VNC, D-Bus, or the socket chardev, they all
> actually expect a socket kind or will fail in different ways at runtime.
>
> Throw an error early if the given 'add_client' fd is not a socket, and
> close it to avoid leaks.
>
> This allows to replace the close() call with a more correct & portable
> closesocket() version.
>
> (this will allow importing sockets on Windows with a specialized command
> in the following patch, while keeping the remaining monitor associated
> sockets/add_client code & usage untouched)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> qapi/misc.json | 3 +++
> monitor/qmp-cmds.c | 7 +++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 27ef5a2b20..f0217cfba0 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -14,6 +14,9 @@
> # Allow client connections for VNC, Spice and socket based
> # character devices to be passed in to QEMU via SCM_RIGHTS.
> #
> +# If the FD associated with @fdname is not a socket, the command will fail and
> +# the FD will be closed.
> +#
> # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
> # the name of a character device (eg. from -chardev id=XXXX)
> #
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 859012aef4..9f7751beeb 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -14,6 +14,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/sockets.h"
> #include "monitor-internal.h"
> #include "monitor/qdev.h"
> #include "monitor/qmp-helpers.h"
> @@ -139,6 +140,12 @@ void qmp_add_client(const char *protocol, const char *fdname,
> return;
> }
>
> + if (!fd_is_socket(fd)) {
> + error_setg(errp, "add_client expects a socket");
Let's mention the parameter name: "parameter @fdname must name a
socket".
> + close(fd);
> + return;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(protocol_table); i++) {
> if (!strcmp(protocol, protocol_table[i].name)) {
> if (!protocol_table[i].add_client(fd, has_skipauth, skipauth,
Acked-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 06/11] monitor: release the lock before calling close()
2023-03-06 12:27 ` [PATCH v4 06/11] monitor: release the lock before calling close() marcandre.lureau
@ 2023-03-06 15:29 ` Markus Armbruster
2023-03-06 15:44 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2023-03-06 15:29 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> As per comment, presumably to avoid syscall in critical section.
>
> Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> monitor/fds.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/monitor/fds.c b/monitor/fds.c
> index 26b39a0ce6..7daf1064e1 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -80,7 +80,8 @@ void qmp_getfd(const char *fdname, Error **errp)
> return;
> }
>
> - QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> + /* See close() call below. */
> + qemu_mutex_lock(&cur_mon->mon_lock);
> QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> if (strcmp(monfd->name, fdname) != 0) {
> continue;
> @@ -88,6 +89,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>
> tmp_fd = monfd->fd;
> monfd->fd = fd;
> + qemu_mutex_unlock(&cur_mon->mon_lock);
> /* Make sure close() is outside critical section */
> close(tmp_fd);
> return;
Not changed by your patch, but odd: when no fd named @fdname exists, the
command does nothing silently. Shouldn't it fail then?
> @@ -98,6 +100,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> monfd->fd = fd;
>
> QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> + qemu_mutex_unlock(&cur_mon->mon_lock);
> }
>
> void qmp_closefd(const char *fdname, Error **errp)
Alex suggested a different way to do this in reply to v3 of this patch.
Please have a look and reply there.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 06/11] monitor: release the lock before calling close()
2023-03-06 15:29 ` Markus Armbruster
@ 2023-03-06 15:44 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2023-03-06 15:44 UTC (permalink / raw
To: Markus Armbruster
Cc: marcandre.lureau, qemu-devel, Thomas Huth, Gerd Hoffmann,
Alex Bennée, Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
Markus Armbruster <armbru@redhat.com> writes:
> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> As per comment, presumably to avoid syscall in critical section.
>>
>> Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> monitor/fds.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/monitor/fds.c b/monitor/fds.c
>> index 26b39a0ce6..7daf1064e1 100644
>> --- a/monitor/fds.c
>> +++ b/monitor/fds.c
>> @@ -80,7 +80,8 @@ void qmp_getfd(const char *fdname, Error **errp)
>> return;
>> }
>>
>> - QEMU_LOCK_GUARD(&cur_mon->mon_lock);
>> + /* See close() call below. */
>> + qemu_mutex_lock(&cur_mon->mon_lock);
>> QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>> if (strcmp(monfd->name, fdname) != 0) {
>> continue;
>> @@ -88,6 +89,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>>
>> tmp_fd = monfd->fd;
>> monfd->fd = fd;
>> + qemu_mutex_unlock(&cur_mon->mon_lock);
>> /* Make sure close() is outside critical section */
>> close(tmp_fd);
>> return;
>
> Not changed by your patch, but odd: when no fd named @fdname exists, the
> command does nothing silently. Shouldn't it fail then?
Alex confused me. Ignore this.
>> @@ -98,6 +100,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>> monfd->fd = fd;
>>
>> QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
>> + qemu_mutex_unlock(&cur_mon->mon_lock);
>> }
>>
>> void qmp_closefd(const char *fdname, Error **errp)
>
> Alex suggested a different way to do this in reply to v3 of this patch.
> Please have a look and reply there.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 08/11] qmp: add 'get-win32-socket'
2023-03-06 12:27 ` [PATCH v4 08/11] qmp: add 'get-win32-socket' marcandre.lureau
@ 2023-03-06 15:47 ` Markus Armbruster
2023-03-07 12:31 ` Marc-André Lureau
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2023-03-06 15:47 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> A process with enough capabilities can duplicate a socket to QEMU. Add a
> QMP command to import it and add it to the monitor fd list, so it can be
> later used by other commands.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi/misc.json | 30 ++++++++++++++++++++
> monitor/fds.c | 76 +++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 90 insertions(+), 16 deletions(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index f0217cfba0..031c94050c 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -275,6 +275,36 @@
> ##
> { 'command': 'getfd', 'data': {'fdname': 'str'} }
>
> +##
> +# @get-win32-socket:
> +#
> +# Add a socket that was duplicated to QEMU process with WSADuplicateSocketW()
> +# via WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name (the SOCKET
> +# is associated with a CRT file descriptor)
Long line.
> +#
> +# @info: the WSAPROTOCOL_INFOW structure (encoded in base64)
> +#
> +# @fdname: file descriptor name
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 8.0
> +#
> +# Notes: If @fdname already exists, the file descriptor assigned to
> +# it will be closed and replaced by the received file
> +# descriptor.
> +#
> +# The 'closefd' command can be used to explicitly close the
> +# file descriptor when it is no longer needed.
> +#
> +# Example:
> +#
> +# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", fdname": "skclient" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 'if': 'CONFIG_WIN32' }
> +
> ##
> # @closefd:
> #
> diff --git a/monitor/fds.c b/monitor/fds.c
> index 7daf1064e1..9ed4197358 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -61,46 +61,55 @@ struct MonFdset {
> static QemuMutex mon_fdsets_lock;
> static QLIST_HEAD(, MonFdset) mon_fdsets;
>
> -void qmp_getfd(const char *fdname, Error **errp)
> +static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp)
> {
> - Monitor *cur_mon = monitor_cur();
> mon_fd_t *monfd;
> - int fd, tmp_fd;
> -
> - fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
> - if (fd == -1) {
> - error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
> - return;
> - }
>
> if (qemu_isdigit(fdname[0])) {
> close(fd);
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> "a name not starting with a digit");
> - return;
> + return false;
> }
>
> /* See close() call below. */
> - qemu_mutex_lock(&cur_mon->mon_lock);
> - QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> + qemu_mutex_lock(&mon->mon_lock);
> + QLIST_FOREACH(monfd, &mon->fds, next) {
> + int tmp_fd;
> +
> if (strcmp(monfd->name, fdname) != 0) {
> continue;
> }
>
> tmp_fd = monfd->fd;
> monfd->fd = fd;
> - qemu_mutex_unlock(&cur_mon->mon_lock);
> + qemu_mutex_unlock(&mon->mon_lock);
> /* Make sure close() is outside critical section */
> close(tmp_fd);
> - return;
> + return true;
> }
Have you considered factoring out the loop searching ->fds?
>
> monfd = g_new0(mon_fd_t, 1);
> monfd->name = g_strdup(fdname);
> monfd->fd = fd;
>
> - QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> - qemu_mutex_unlock(&cur_mon->mon_lock);
> + QLIST_INSERT_HEAD(&mon->fds, monfd, next);
> + qemu_mutex_unlock(&mon->mon_lock);
> + return true;
> +}
> +
> +void qmp_getfd(const char *fdname, Error **errp)
> +{
> + Monitor *cur_mon = monitor_cur();
> + int fd;
> +
> + fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
> + if (fd == -1) {
> + error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
> + return;
> + }
> +
> + monitor_add_fd(cur_mon, fd, fdname, errp);
> }
>
> void qmp_closefd(const char *fdname, Error **errp)
> @@ -214,6 +223,41 @@ error:
> return NULL;
> }
With the doc comment tidied up:
Acked-by: Markus Armbruster <armbru@redhat.com>
The remainder is greek to me :)
>
> +#ifdef WIN32
> +void qmp_get_win32_socket(const char *infos, const char *fdname, Error **errp)
> +{
> + g_autofree WSAPROTOCOL_INFOW *info = NULL;
> + gsize len;
> + SOCKET sk;
> + int fd;
> +
> + info = (void *)g_base64_decode(infos, &len);
> + if (len != sizeof(*info)) {
> + error_setg(errp, "Invalid WSAPROTOCOL_INFOW value");
> + return;
> + }
> +
> + sk = WSASocketW(FROM_PROTOCOL_INFO,
> + FROM_PROTOCOL_INFO,
> + FROM_PROTOCOL_INFO,
> + info, 0, 0);
> + if (sk == INVALID_SOCKET) {
> + error_setg_win32(errp, WSAGetLastError(), "Couldn't import socket");
> + return;
> + }
> +
> + fd = _open_osfhandle(sk, _O_BINARY);
> + if (fd < 0) {
> + error_setg_errno(errp, errno, "Failed to associate a FD with the SOCKET");
> + closesocket(sk);
> + return;
> + }
> +
> + monitor_add_fd(monitor_cur(), fd, fdname, errp);
> +}
> +#endif
> +
> +
> void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> {
> MonFdset *mon_fdset;
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX
2023-03-06 12:27 ` [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX marcandre.lureau
@ 2023-03-06 16:01 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2023-03-06 16:01 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
Regarding the subject line:
1. We use "qmp:" for qmp-only patches, "hmp:" for hmp-only patches, and
"monitor:" for both. "qmp hmp:" would work there, too.
2. We're not implementing anything, we're restricting the existing
implementation to hosts where it is actually useful.
Suggest "monitor: Restrict command getfd to POSIX hosts"
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Currently, the function will simply fail if ancillary fds are not
> provided, for ex on unsupported platforms.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi/misc.json | 2 +-
> monitor/fds.c | 2 ++
> monitor/hmp-cmds.c | 2 ++
> hmp-commands.hx | 2 ++
> 4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 031c94050c..96c053e305 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -273,7 +273,7 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> +{ 'command': 'getfd', 'data': {'fdname': 'str'}, 'if': 'CONFIG_POSIX' }
>
> ##
> # @get-win32-socket:
This changes the failure from
{"error": {"class": "GenericError", "desc": "No file descriptor supplied via SCM_RIGHTS"}}
to
{"error": {"class": "CommandNotFound", "desc": "The command getfd has not been found"}}
when CONFIG_POSIX is off. I think this is fine. But I'd like the
commit message to document it.
> diff --git a/monitor/fds.c b/monitor/fds.c
> index 9ed4197358..d86c2c674c 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -98,6 +98,7 @@ static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **err
> return true;
> }
>
> +#ifdef CONFIG_POSIX
> void qmp_getfd(const char *fdname, Error **errp)
> {
> Monitor *cur_mon = monitor_cur();
> @@ -111,6 +112,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>
> monitor_add_fd(cur_mon, fd, fdname, errp);
> }
> +#endif
>
> void qmp_closefd(const char *fdname, Error **errp)
> {
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 34bd8c67d7..6c559b48c8 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -192,6 +192,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, err);
> }
>
> +#ifdef CONFIG_POSIX
> void hmp_getfd(Monitor *mon, const QDict *qdict)
> {
> const char *fdname = qdict_get_str(qdict, "fdname");
> @@ -200,6 +201,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict)
> qmp_getfd(fdname, &err);
> hmp_handle_error(mon, err);
> }
> +#endif
>
> void hmp_closefd(Monitor *mon, const QDict *qdict)
> {
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index b87c250e23..bb85ee1d26 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1486,6 +1486,7 @@ SRST
> Inject an MCE on the given CPU (x86 only).
> ERST
>
> +#ifdef CONFIG_POSIX
> {
> .name = "getfd",
> .args_type = "fdname:s",
> @@ -1501,6 +1502,7 @@ SRST
> mechanism on unix sockets, it is stored using the name *fdname* for
> later use by other monitor commands.
> ERST
> +#endif
>
> {
> .name = "closefd",
With the commit message brushed up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible
2023-03-06 12:27 ` [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible marcandre.lureau
@ 2023-03-06 16:02 ` Markus Armbruster
2023-03-06 18:26 ` Marc-André Lureau
2023-03-06 16:05 ` Peter Maydell
1 sibling, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2023-03-06 16:02 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
Does anything in the series depend on this patch?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible
2023-03-06 12:27 ` [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible marcandre.lureau
2023-03-06 16:02 ` Markus Armbruster
@ 2023-03-06 16:05 ` Peter Maydell
2023-03-06 18:29 ` Marc-André Lureau
1 sibling, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2023-03-06 16:05 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Make the resulting code even prettier, if possible.
This seems to be a bit short on rationale. This is generated
code, so in general nobody is going to be reading it, and
running clang-format on it every time we generate code feels
like it would be a bit of a waste of cycles...
thanks
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible
2023-03-06 16:02 ` Markus Armbruster
@ 2023-03-06 18:26 ` Marc-André Lureau
0 siblings, 0 replies; 28+ messages in thread
From: Marc-André Lureau @ 2023-03-06 18:26 UTC (permalink / raw
To: Markus Armbruster
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
Hi
On Mon, Mar 6, 2023 at 8:03 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Does anything in the series depend on this patch?
>
Indeed, it's no longer directly relevant for this series. It's a
left-over from various iterations. Nevertheless, feedback welcome.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible
2023-03-06 16:05 ` Peter Maydell
@ 2023-03-06 18:29 ` Marc-André Lureau
2023-03-06 18:39 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Marc-André Lureau @ 2023-03-06 18:29 UTC (permalink / raw
To: Peter Maydell
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
Hi
On Mon, Mar 6, 2023 at 8:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Make the resulting code even prettier, if possible.
>
> This seems to be a bit short on rationale. This is generated
> code, so in general nobody is going to be reading it, and
> running clang-format on it every time we generate code feels
> like it would be a bit of a waste of cycles...
With this reasoning, why do we care about indentation of generated code at all?
I think it still makes sense, because you have many reasons to read
through it eventually, and making it a bit more friendly helps.
Whether it is a waste of time or not, hmm. Indeed, my experience with
clang-format has teached me that it is not the most CPU-friendly
sometimes...
Perhaps the solution is only to enable formatting when debugging is
enabled, for example?
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible
2023-03-06 18:29 ` Marc-André Lureau
@ 2023-03-06 18:39 ` Peter Maydell
2023-03-07 8:51 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2023-03-06 18:39 UTC (permalink / raw
To: Marc-André Lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Markus Armbruster, Eric Blake, Dr. David Alan Gilbert
On Mon, 6 Mar 2023 at 18:29, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Mon, Mar 6, 2023 at 8:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Make the resulting code even prettier, if possible.
> >
> > This seems to be a bit short on rationale. This is generated
> > code, so in general nobody is going to be reading it, and
> > running clang-format on it every time we generate code feels
> > like it would be a bit of a waste of cycles...
>
> With this reasoning, why do we care about indentation of generated code at all?
>
> I think it still makes sense, because you have many reasons to read
> through it eventually, and making it a bit more friendly helps.
Yeah, sometimes you have to read through it, so not printing
it all one one long line is helpful. But it's a tradeoff --
"make it basically kinda readable by tracking indent level" is
easy and quick; running the whole output through a pretty-printer
is more expensive and doesn't improve the output by very much
over what we already have. (If I'm wrong about that last part,
it would be useful for the commit message to give an example
of currently unreadable output that clang-format makes more usable.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible
2023-03-06 18:39 ` Peter Maydell
@ 2023-03-07 8:51 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2023-03-07 8:51 UTC (permalink / raw
To: Peter Maydell
Cc: Marc-André Lureau, qemu-devel, Thomas Huth, Gerd Hoffmann,
Alex Bennée, Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 6 Mar 2023 at 18:29, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>>
>> Hi
>>
>> On Mon, Mar 6, 2023 at 8:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> >
>> > On Mon, 6 Mar 2023 at 12:33, <marcandre.lureau@redhat.com> wrote:
>> > >
>> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > >
>> > > Make the resulting code even prettier, if possible.
>> >
>> > This seems to be a bit short on rationale. This is generated
>> > code, so in general nobody is going to be reading it, and
>> > running clang-format on it every time we generate code feels
>> > like it would be a bit of a waste of cycles...
>>
>> With this reasoning, why do we care about indentation of generated code at all?
>>
>> I think it still makes sense, because you have many reasons to read
>> through it eventually, and making it a bit more friendly helps.
>
> Yeah, sometimes you have to read through it, so not printing
I have to read it frequently enough to make ugly code painful.
> it all one one long line is helpful. But it's a tradeoff --
> "make it basically kinda readable by tracking indent level" is
> easy and quick; running the whole output through a pretty-printer
> is more expensive and doesn't improve the output by very much
> over what we already have. (If I'm wrong about that last part,
> it would be useful for the commit message to give an example
> of currently unreadable output that clang-format makes more usable.)
Tracking indent level is certainly quick, but it can take a bit of
effort. In review of v3, I asked for more effort, and Marc-André
floated the idea of leaving the job to readily available clang-format
instead:
ok, I improved the indentation a bit.
However, I think it would be simpler, and better, if we piped the
generated code to clang-format (when available). I made a simple patch
for that too.
My reply was
Piping through indent or clang-format may well give us neater results
for less effort.
We might want to dumb down generator code then.
Message-ID: <87356yq9rs.fsf@pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg06311.html
v4 no longer contains the indentation issue that triggered this
exchange. Let's treat this patch as if it was separate, so it doesn't
delay the remainder of the series.
You're certainly right to ask for an assessment of costs and benefits.
Costs:
* Yet another dependency, albeit optional
* Running the indenter (not sure it's noticable, but numbers wanted)
Benefits:
* Result is maybe tidier (examples wanted)
* Not in this patch: we could dumb down the code generator some (the
dependency becomes de facto mandatory for serious QAPI developers
then)
We may choose to shelve this patch until the next time decent formatting
takes us effort.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 08/11] qmp: add 'get-win32-socket'
2023-03-06 15:47 ` Markus Armbruster
@ 2023-03-07 12:31 ` Marc-André Lureau
0 siblings, 0 replies; 28+ messages in thread
From: Marc-André Lureau @ 2023-03-07 12:31 UTC (permalink / raw
To: Markus Armbruster
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Daniel P. Berrangé,
Eric Blake, Dr. David Alan Gilbert
Hi
On Mon, Mar 6, 2023 at 7:49 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > A process with enough capabilities can duplicate a socket to QEMU. Add a
> > QMP command to import it and add it to the monitor fd list, so it can be
> > later used by other commands.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > qapi/misc.json | 30 ++++++++++++++++++++
> > monitor/fds.c | 76 +++++++++++++++++++++++++++++++++++++++-----------
> > 2 files changed, 90 insertions(+), 16 deletions(-)
> >
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index f0217cfba0..031c94050c 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -275,6 +275,36 @@
> > ##
> > { 'command': 'getfd', 'data': {'fdname': 'str'} }
> >
> > +##
> > +# @get-win32-socket:
> > +#
> > +# Add a socket that was duplicated to QEMU process with WSADuplicateSocketW()
> > +# via WSASocket() & WSAPROTOCOL_INFOW structure and assign it a name (the SOCKET
> > +# is associated with a CRT file descriptor)
>
> Long line.
>
Ok.. see related .editorconfig patch to make everyone's life easier.
> > +#
> > +# @info: the WSAPROTOCOL_INFOW structure (encoded in base64)
> > +#
> > +# @fdname: file descriptor name
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 8.0
> > +#
> > +# Notes: If @fdname already exists, the file descriptor assigned to
> > +# it will be closed and replaced by the received file
> > +# descriptor.
> > +#
> > +# The 'closefd' command can be used to explicitly close the
> > +# file descriptor when it is no longer needed.
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", fdname": "skclient" } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 'if': 'CONFIG_WIN32' }
> > +
> > ##
> > # @closefd:
> > #
> > diff --git a/monitor/fds.c b/monitor/fds.c
> > index 7daf1064e1..9ed4197358 100644
> > --- a/monitor/fds.c
> > +++ b/monitor/fds.c
> > @@ -61,46 +61,55 @@ struct MonFdset {
> > static QemuMutex mon_fdsets_lock;
> > static QLIST_HEAD(, MonFdset) mon_fdsets;
> >
> > -void qmp_getfd(const char *fdname, Error **errp)
> > +static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **errp)
> > {
> > - Monitor *cur_mon = monitor_cur();
> > mon_fd_t *monfd;
> > - int fd, tmp_fd;
> > -
> > - fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
> > - if (fd == -1) {
> > - error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
> > - return;
> > - }
> >
> > if (qemu_isdigit(fdname[0])) {
> > close(fd);
> > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> > "a name not starting with a digit");
> > - return;
> > + return false;
> > }
> >
> > /* See close() call below. */
> > - qemu_mutex_lock(&cur_mon->mon_lock);
> > - QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> > + qemu_mutex_lock(&mon->mon_lock);
> > + QLIST_FOREACH(monfd, &mon->fds, next) {
> > + int tmp_fd;
> > +
> > if (strcmp(monfd->name, fdname) != 0) {
> > continue;
> > }
> >
> > tmp_fd = monfd->fd;
> > monfd->fd = fd;
> > - qemu_mutex_unlock(&cur_mon->mon_lock);
> > + qemu_mutex_unlock(&mon->mon_lock);
> > /* Make sure close() is outside critical section */
> > close(tmp_fd);
> > - return;
> > + return true;
> > }
>
> Have you considered factoring out the loop searching ->fds?
Not really, it doesn't look trivial at first sight because they do
different things.
>
> >
> > monfd = g_new0(mon_fd_t, 1);
> > monfd->name = g_strdup(fdname);
> > monfd->fd = fd;
> >
> > - QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> > - qemu_mutex_unlock(&cur_mon->mon_lock);
> > + QLIST_INSERT_HEAD(&mon->fds, monfd, next);
> > + qemu_mutex_unlock(&mon->mon_lock);
> > + return true;
> > +}
> > +
> > +void qmp_getfd(const char *fdname, Error **errp)
> > +{
> > + Monitor *cur_mon = monitor_cur();
> > + int fd;
> > +
> > + fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
> > + if (fd == -1) {
> > + error_setg(errp, "No file descriptor supplied via SCM_RIGHTS");
> > + return;
> > + }
> > +
> > + monitor_add_fd(cur_mon, fd, fdname, errp);
> > }
> >
> > void qmp_closefd(const char *fdname, Error **errp)
> > @@ -214,6 +223,41 @@ error:
> > return NULL;
> > }
>
> With the doc comment tidied up:
> Acked-by: Markus Armbruster <armbru@redhat.com>
>
> The remainder is greek to me :)
ok, thanks
>
> >
> > +#ifdef WIN32
> > +void qmp_get_win32_socket(const char *infos, const char *fdname, Error **errp)
> > +{
> > + g_autofree WSAPROTOCOL_INFOW *info = NULL;
> > + gsize len;
> > + SOCKET sk;
> > + int fd;
> > +
> > + info = (void *)g_base64_decode(infos, &len);
> > + if (len != sizeof(*info)) {
> > + error_setg(errp, "Invalid WSAPROTOCOL_INFOW value");
> > + return;
> > + }
> > +
> > + sk = WSASocketW(FROM_PROTOCOL_INFO,
> > + FROM_PROTOCOL_INFO,
> > + FROM_PROTOCOL_INFO,
> > + info, 0, 0);
> > + if (sk == INVALID_SOCKET) {
> > + error_setg_win32(errp, WSAGetLastError(), "Couldn't import socket");
> > + return;
> > + }
> > +
> > + fd = _open_osfhandle(sk, _O_BINARY);
> > + if (fd < 0) {
> > + error_setg_errno(errp, errno, "Failed to associate a FD with the SOCKET");
> > + closesocket(sk);
> > + return;
> > + }
> > +
> > + monitor_add_fd(monitor_cur(), fd, fdname, errp);
> > +}
> > +#endif
> > +
> > +
> > void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> > {
> > MonFdset *mon_fdset;
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32
2023-03-06 12:27 ` [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32 marcandre.lureau
@ 2023-03-07 14:50 ` Daniel P. Berrangé
2023-03-08 6:53 ` Marc-André Lureau
0 siblings, 1 reply; 28+ messages in thread
From: Daniel P. Berrangé @ 2023-03-07 14:50 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Markus Armbruster,
Eric Blake, Dr. David Alan Gilbert
On Mon, Mar 06, 2023 at 04:27:44PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Manually implement a socketpair() function, using UNIX sockets and
> simple peer credential checking.
>
> QEMU doesn't make much use of socketpair, beside vhost-user which is not
> available for win32 at this point. However, I intend to use it for
> writing some new portable tests.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/qemu/sockets.h | 2 -
> util/oslib-win32.c | 110 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 110 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 2b0698a7c9..d935fd80da 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -15,7 +15,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
> bool fd_is_socket(int fd);
> int qemu_socket(int domain, int type, int protocol);
>
> -#ifndef WIN32
> /**
> * qemu_socketpair:
> * @domain: specifies a communication domain, such as PF_UNIX
> @@ -30,7 +29,6 @@ int qemu_socket(int domain, int type, int protocol);
> * Return 0 on success.
> */
> int qemu_socketpair(int domain, int type, int protocol, int sv[2]);
> -#endif
>
> int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
> /*
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 29a667ae3d..16f8a67f7e 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
> return qemu_socket_select(sockfd, NULL, 0, errp);
> }
>
> +int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> +{
> + struct sockaddr_un addr = {
> + 0,
> + };
> + socklen_t socklen;
> + int listener = -1;
> + int client = -1;
> + int server = -1;
> + g_autofree char *path = NULL;
> + int tmpfd;
> + u_long arg;
> + int ret = -1;
> +
> + g_return_val_if_fail(sv != NULL, -1);
> +
> + addr.sun_family = AF_UNIX;
> + socklen = sizeof(addr);
> +
> + tmpfd = g_file_open_tmp(NULL, &path, NULL);
> + if (tmpfd == -1 || !path) {
> + errno = EACCES;
> + goto out;
> + }
> +
> + close(tmpfd);
> +
> + if (strlen(path) >= sizeof(addr.sun_path)) {
> + errno = EINVAL;
> + goto out;
> + }
> +
> + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> +
> + listener = socket(domain, type, protocol);
> + if (listener == -1) {
> + goto out;
> + }
> +
> + if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
> + errno = EACCES;
> + goto out;
> + }
> + g_clear_pointer(&path, g_free);
> +
> + if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
> + goto out;
> + }
> +
> + if (listen(listener, 1) == -1) {
> + goto out;
> + }
> +
> + client = socket(domain, type, protocol);
> + if (client == -1) {
> + goto out;
> + }
> +
> + arg = 1;
> + if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> + goto out;
> + }
> +
> + if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
> + WSAGetLastError() != WSAEWOULDBLOCK) {
> + goto out;
> + }
> +
> + server = accept(listener, NULL, NULL);
> + if (server == -1) {
> + goto out;
> + }
In theory at this point 'client' if connect() returned WSAEWOULDBLOCK,
then at this point it should be fully connected. I wonder if that is
actually guaranteed though, or should we do something to validate
there's no race condition ?
> +
> + arg = 0;
> + if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> + goto out;
> + }
> +
> + arg = 0;
> + if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
> + goto out;
> + }
Maybe this will force a synchronization point ?
Alteratively select() + getsockopt(SO_ERROR) is what we used to
do to check for connect() completion (logic removed now but can
be seen in b2587932582333197c88bf663785b19f441989d7)
> +
> + if (arg != GetCurrentProcessId()) {
> + errno = EPERM;
> + goto out;
> + }
> +
> + sv[0] = server;
> + server = -1;
> + sv[1] = client;
> + client = -1;
> + ret = 0;
> +
> +out:
> + if (listener != -1) {
> + close(listener);
> + }
> + if (client != -1) {
> + close(client);
> + }
> + if (server != -1) {
> + close(server);
> + }
> + if (path) {
> + DeleteFile(path);
> + }
> + return ret;
> +}
> +
> #undef connect
> int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
> socklen_t addrlen)
> --
> 2.39.2
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 09/11] libqtest: make qtest_qmp_add_client work on win32
2023-03-06 12:27 ` [PATCH v4 09/11] libqtest: make qtest_qmp_add_client work on win32 marcandre.lureau
@ 2023-03-07 14:54 ` Daniel P. Berrangé
0 siblings, 0 replies; 28+ messages in thread
From: Daniel P. Berrangé @ 2023-03-07 14:54 UTC (permalink / raw
To: marcandre.lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Markus Armbruster,
Eric Blake, Dr. David Alan Gilbert
On Mon, Mar 06, 2023 at 04:27:49PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Use the "get-win32-socket" function to pass an opened socket to QEMU,
> instead of using "getfd", which relies on socket ancillary FD message
> passing.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/qtest/libqtest.h | 5 ++---
> tests/qtest/libqtest.c | 18 ++++++++++++++++--
> 2 files changed, 18 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32
2023-03-07 14:50 ` Daniel P. Berrangé
@ 2023-03-08 6:53 ` Marc-André Lureau
2023-03-08 9:27 ` Daniel P. Berrangé
0 siblings, 1 reply; 28+ messages in thread
From: Marc-André Lureau @ 2023-03-08 6:53 UTC (permalink / raw
To: Daniel P. Berrangé
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Markus Armbruster,
Eric Blake, Dr. David Alan Gilbert
Hi
On Tue, Mar 7, 2023 at 6:50 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Mar 06, 2023 at 04:27:44PM +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Manually implement a socketpair() function, using UNIX sockets and
> > simple peer credential checking.
> >
> > QEMU doesn't make much use of socketpair, beside vhost-user which is not
> > available for win32 at this point. However, I intend to use it for
> > writing some new portable tests.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > include/qemu/sockets.h | 2 -
> > util/oslib-win32.c | 110 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 110 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > index 2b0698a7c9..d935fd80da 100644
> > --- a/include/qemu/sockets.h
> > +++ b/include/qemu/sockets.h
> > @@ -15,7 +15,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
> > bool fd_is_socket(int fd);
> > int qemu_socket(int domain, int type, int protocol);
> >
> > -#ifndef WIN32
> > /**
> > * qemu_socketpair:
> > * @domain: specifies a communication domain, such as PF_UNIX
> > @@ -30,7 +29,6 @@ int qemu_socket(int domain, int type, int protocol);
> > * Return 0 on success.
> > */
> > int qemu_socketpair(int domain, int type, int protocol, int sv[2]);
> > -#endif
> >
> > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
> > /*
> > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > index 29a667ae3d..16f8a67f7e 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
> > return qemu_socket_select(sockfd, NULL, 0, errp);
> > }
> >
> > +int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> > +{
> > + struct sockaddr_un addr = {
> > + 0,
> > + };
> > + socklen_t socklen;
> > + int listener = -1;
> > + int client = -1;
> > + int server = -1;
> > + g_autofree char *path = NULL;
> > + int tmpfd;
> > + u_long arg;
> > + int ret = -1;
> > +
> > + g_return_val_if_fail(sv != NULL, -1);
> > +
> > + addr.sun_family = AF_UNIX;
> > + socklen = sizeof(addr);
> > +
> > + tmpfd = g_file_open_tmp(NULL, &path, NULL);
> > + if (tmpfd == -1 || !path) {
> > + errno = EACCES;
> > + goto out;
> > + }
> > +
> > + close(tmpfd);
> > +
> > + if (strlen(path) >= sizeof(addr.sun_path)) {
> > + errno = EINVAL;
> > + goto out;
> > + }
> > +
> > + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> > +
> > + listener = socket(domain, type, protocol);
> > + if (listener == -1) {
> > + goto out;
> > + }
> > +
> > + if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
> > + errno = EACCES;
> > + goto out;
> > + }
> > + g_clear_pointer(&path, g_free);
> > +
> > + if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
> > + goto out;
> > + }
> > +
> > + if (listen(listener, 1) == -1) {
> > + goto out;
> > + }
> > +
> > + client = socket(domain, type, protocol);
> > + if (client == -1) {
> > + goto out;
> > + }
> > +
> > + arg = 1;
> > + if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > + goto out;
> > + }
> > +
> > + if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
> > + WSAGetLastError() != WSAEWOULDBLOCK) {
> > + goto out;
> > + }
> > +
> > + server = accept(listener, NULL, NULL);
> > + if (server == -1) {
> > + goto out;
> > + }
>
> In theory at this point 'client' if connect() returned WSAEWOULDBLOCK,
> then at this point it should be fully connected. I wonder if that is
> actually guaranteed though, or should we do something to validate
> there's no race condition ?
>
> > +
> > + arg = 0;
> > + if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > + goto out;
> > + }
> > +
> > + arg = 0;
> > + if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
> > + goto out;
> > + }
>
> Maybe this will force a synchronization point ?
yeah, I guess switching back to sync and getting the peer pid,
I assume the unix socket pair to be ready at this point.
>
> Alteratively select() + getsockopt(SO_ERROR) is what we used to
> do to check for connect() completion (logic removed now but can
> be seen in b2587932582333197c88bf663785b19f441989d7)
>
That's hopefully not necessary.
thanks
>
>
> > +
> > + if (arg != GetCurrentProcessId()) {
> > + errno = EPERM;
> > + goto out;
> > + }
> > +
> > + sv[0] = server;
> > + server = -1;
> > + sv[1] = client;
> > + client = -1;
> > + ret = 0;
> > +
> > +out:
> > + if (listener != -1) {
> > + close(listener);
> > + }
> > + if (client != -1) {
> > + close(client);
> > + }
> > + if (server != -1) {
> > + close(server);
> > + }
> > + if (path) {
> > + DeleteFile(path);
> > + }
> > + return ret;
> > +}
> > +
> > #undef connect
> > int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
> > socklen_t addrlen)
> > --
> > 2.39.2
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32
2023-03-08 6:53 ` Marc-André Lureau
@ 2023-03-08 9:27 ` Daniel P. Berrangé
0 siblings, 0 replies; 28+ messages in thread
From: Daniel P. Berrangé @ 2023-03-08 9:27 UTC (permalink / raw
To: Marc-André Lureau
Cc: qemu-devel, Thomas Huth, Gerd Hoffmann, Alex Bennée,
Michael Roth, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Beraldo Leal,
Wainer dos Santos Moschetta, Stefan Weil, Markus Armbruster,
Eric Blake, Dr. David Alan Gilbert
On Wed, Mar 08, 2023 at 10:53:13AM +0400, Marc-André Lureau wrote:
> Hi
>
> On Tue, Mar 7, 2023 at 6:50 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, Mar 06, 2023 at 04:27:44PM +0400, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Manually implement a socketpair() function, using UNIX sockets and
> > > simple peer credential checking.
> > >
> > > QEMU doesn't make much use of socketpair, beside vhost-user which is not
> > > available for win32 at this point. However, I intend to use it for
> > > writing some new portable tests.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > include/qemu/sockets.h | 2 -
> > > util/oslib-win32.c | 110 +++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 110 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > > index 2b0698a7c9..d935fd80da 100644
> > > --- a/include/qemu/sockets.h
> > > +++ b/include/qemu/sockets.h
> > > @@ -15,7 +15,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
> > > bool fd_is_socket(int fd);
> > > int qemu_socket(int domain, int type, int protocol);
> > >
> > > -#ifndef WIN32
> > > /**
> > > * qemu_socketpair:
> > > * @domain: specifies a communication domain, such as PF_UNIX
> > > @@ -30,7 +29,6 @@ int qemu_socket(int domain, int type, int protocol);
> > > * Return 0 on success.
> > > */
> > > int qemu_socketpair(int domain, int type, int protocol, int sv[2]);
> > > -#endif
> > >
> > > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
> > > /*
> > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> > > index 29a667ae3d..16f8a67f7e 100644
> > > --- a/util/oslib-win32.c
> > > +++ b/util/oslib-win32.c
> > > @@ -310,6 +310,116 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
> > > return qemu_socket_select(sockfd, NULL, 0, errp);
> > > }
> > >
> > > +int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> > > +{
> > > + struct sockaddr_un addr = {
> > > + 0,
> > > + };
> > > + socklen_t socklen;
> > > + int listener = -1;
> > > + int client = -1;
> > > + int server = -1;
> > > + g_autofree char *path = NULL;
> > > + int tmpfd;
> > > + u_long arg;
> > > + int ret = -1;
> > > +
> > > + g_return_val_if_fail(sv != NULL, -1);
> > > +
> > > + addr.sun_family = AF_UNIX;
> > > + socklen = sizeof(addr);
> > > +
> > > + tmpfd = g_file_open_tmp(NULL, &path, NULL);
> > > + if (tmpfd == -1 || !path) {
> > > + errno = EACCES;
> > > + goto out;
> > > + }
> > > +
> > > + close(tmpfd);
> > > +
> > > + if (strlen(path) >= sizeof(addr.sun_path)) {
> > > + errno = EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1);
> > > +
> > > + listener = socket(domain, type, protocol);
> > > + if (listener == -1) {
> > > + goto out;
> > > + }
> > > +
> > > + if (DeleteFile(path) == 0 && GetLastError() != ERROR_FILE_NOT_FOUND) {
> > > + errno = EACCES;
> > > + goto out;
> > > + }
> > > + g_clear_pointer(&path, g_free);
> > > +
> > > + if (bind(listener, (struct sockaddr *)&addr, socklen) == -1) {
> > > + goto out;
> > > + }
> > > +
> > > + if (listen(listener, 1) == -1) {
> > > + goto out;
> > > + }
> > > +
> > > + client = socket(domain, type, protocol);
> > > + if (client == -1) {
> > > + goto out;
> > > + }
> > > +
> > > + arg = 1;
> > > + if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > > + goto out;
> > > + }
> > > +
> > > + if (connect(client, (struct sockaddr *)&addr, socklen) == -1 &&
> > > + WSAGetLastError() != WSAEWOULDBLOCK) {
> > > + goto out;
> > > + }
> > > +
> > > + server = accept(listener, NULL, NULL);
> > > + if (server == -1) {
> > > + goto out;
> > > + }
> >
> > In theory at this point 'client' if connect() returned WSAEWOULDBLOCK,
> > then at this point it should be fully connected. I wonder if that is
> > actually guaranteed though, or should we do something to validate
> > there's no race condition ?
> >
> > > +
> > > + arg = 0;
> > > + if (ioctlsocket(client, FIONBIO, &arg) != NO_ERROR) {
> > > + goto out;
> > > + }
> > > +
> > > + arg = 0;
> > > + if (ioctlsocket(client, SIO_AF_UNIX_GETPEERPID, &arg) != NO_ERROR) {
> > > + goto out;
> > > + }
> >
> > Maybe this will force a synchronization point ?
>
> yeah, I guess switching back to sync and getting the peer pid,
>
> I assume the unix socket pair to be ready at this point.
Ok, lets hope its ok, but remember this if we see any non-deterministic
failures
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-03-08 9:28 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-06 12:27 [PATCH v4 00/11] QMP command to import win32 sockets marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 01/11] tests: fix path separator, use g_build_filename() marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 02/11] char: do not double-close fd when failing to add client marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 03/11] tests/docker: fix a win32 error due to portability marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 04/11] osdep: implement qemu_socketpair() for win32 marcandre.lureau
2023-03-07 14:50 ` Daniel P. Berrangé
2023-03-08 6:53 ` Marc-André Lureau
2023-03-08 9:27 ` Daniel P. Berrangé
2023-03-06 12:27 ` [PATCH v4 05/11] qmp: 'add_client' actually expects sockets marcandre.lureau
2023-03-06 15:02 ` Markus Armbruster
2023-03-06 12:27 ` [PATCH v4 06/11] monitor: release the lock before calling close() marcandre.lureau
2023-03-06 15:29 ` Markus Armbruster
2023-03-06 15:44 ` Markus Armbruster
2023-03-06 12:27 ` [PATCH v4 07/11] qapi/gen: run C code through clang-format, if possible marcandre.lureau
2023-03-06 16:02 ` Markus Armbruster
2023-03-06 18:26 ` Marc-André Lureau
2023-03-06 16:05 ` Peter Maydell
2023-03-06 18:29 ` Marc-André Lureau
2023-03-06 18:39 ` Peter Maydell
2023-03-07 8:51 ` Markus Armbruster
2023-03-06 12:27 ` [PATCH v4 08/11] qmp: add 'get-win32-socket' marcandre.lureau
2023-03-06 15:47 ` Markus Armbruster
2023-03-07 12:31 ` Marc-André Lureau
2023-03-06 12:27 ` [PATCH v4 09/11] libqtest: make qtest_qmp_add_client work on win32 marcandre.lureau
2023-03-07 14:54 ` Daniel P. Berrangé
2023-03-06 12:27 ` [PATCH v4 10/11] qtest: enable vnc-display test " marcandre.lureau
2023-03-06 12:27 ` [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX marcandre.lureau
2023-03-06 16:01 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.