Skip to contentSkip to conversation

Enable passing pre-opened chardev socket FDs

PATCH Superseded Daniel P. Berrange 4 participants 10 messages Dec 21, 2017

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

cover Enable passing pre-opened chardev socket FDs

This fixes a long standing problem that libvirt has with starting up QEMU.

We have to busy-wait retrying connect() on the QMP monitor socket until QEMU finally creates & listens on it, but at same time must be careful to not wait forever if QEMU exits.

This this patch series, libvirt can simply pass in a pre-opened UNIX domain socket file descriptor, which it can immediately connect to with no busy-wait.

Daniel P. Berrange (2):
io: move fd_is_socket() into common sockets code
char: allow passing pre-opened socket file descriptor at startup

 chardev/char-socket.c  | 66 ++++++++++++++++++++++++++++++++++++++++++++------
 chardev/char.c         |  6 +++++
 include/qemu/sockets.h |  1 +
 io/channel-util.c      | 13 ----------
 monitor.c              |  5 ++++
 qapi/common.json       | 11 +++++++++
 qapi/sockets.json      | 14 ++++++++---
 util/qemu-sockets.c    | 49 +++++++++++++++++++++++++++++++++++++
 8 files changed, 142 insertions(+), 23 deletions(-)
Signature · 1 line
2.14.3
#1 lore raw

PATCH 1/2 io: move fd_is_socket() into common sockets code

The fd_is_socket() helper method is useful in a few places, so put it in the common sockets code.

---
 include/qemu/sockets.h |  1 +
 io/channel-util.c      | 13 -------------
 util/qemu-sockets.c    | 13 +++++++++++++
 3 files changed, 14 insertions(+), 13 deletions(-)
include/qemu/sockets.h +1 -0
index 4f7311b52a..5680880f5a 100644
@@ -12,6 +12,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "qapi-types.h"
 
 /* misc helpers */
+bool fd_is_socket(int fd);
 int qemu_socket(int domain, int type, int protocol);
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
 int socket_set_cork(int fd, int v);
io/channel-util.c +0 -13
index 0fb4bd0837..423d79845a 100644
@@ -24,19 +24,6 @@
 #include "io/channel-socket.h"
 
 
-static bool fd_is_socket(int fd)
-{
-    int optval;
-    socklen_t optlen;
-    optlen = sizeof(optval);
-    return qemu_getsockopt(fd,
-                           SOL_SOCKET,
-                           SO_TYPE,
-                           (char *)&optval,
-                           &optlen) == 0;
-}
-
-
 QIOChannel *qio_channel_new_fd(int fd,
                                Error **errp)
 {
util/qemu-sockets.c +13 -0
index af4f01211a..1d23f0b742 100644
@@ -91,6 +91,19 @@ NetworkAddressFamily inet_netfamily(int family)
     return NETWORK_ADDRESS_FAMILY_UNKNOWN;
 }
 
+bool fd_is_socket(int fd)
+{
+    int optval;
+    socklen_t optlen;
+    optlen = sizeof(optval);
+    return qemu_getsockopt(fd,
+                           SOL_SOCKET,
+                           SO_TYPE,
+                           (char *)&optval,
+                           &optlen) == 0;
+}
+
+
 /*
  * Matrix we're trying to apply
  *
Signature · 1 line
2.14.3
Reviewed-by on v1 1/2
#2 lore raw

Re: [Qemu-devel] [PATCH v1 1/2] io: move fd_is_socket() into common sockets code

Hi

Quoted 4 lines · Daniel P. Berrange

The fd_is_socket() helper method is useful in a few places, so put it in the common sockets code.

Quoted 70 lines
---
 include/qemu/sockets.h |  1 +
 io/channel-util.c      | 13 -------------
 util/qemu-sockets.c    | 13 +++++++++++++
 3 files changed, 14 insertions(+), 13 deletions(-)
include/qemu/sockets.h +1 -0
index 4f7311b52a..5680880f5a 100644
@@ -12,6 +12,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "qapi-types.h"
 
 /* misc helpers */
+bool fd_is_socket(int fd);
 int qemu_socket(int domain, int type, int protocol);
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
 int socket_set_cork(int fd, int v);
io/channel-util.c +0 -13
index 0fb4bd0837..423d79845a 100644
@@ -24,19 +24,6 @@
 #include "io/channel-socket.h"
 
 
-static bool fd_is_socket(int fd)
-{
-    int optval;
-    socklen_t optlen;
-    optlen = sizeof(optval);
-    return qemu_getsockopt(fd,
-                           SOL_SOCKET,
-                           SO_TYPE,
-                           (char *)&optval,
-                           &optlen) == 0;
-}
-
-
 QIOChannel *qio_channel_new_fd(int fd,
                                Error **errp)
 {
util/qemu-sockets.c +13 -1
index af4f01211a..1d23f0b742 100644
@@ -91,6 +91,19 @@ NetworkAddressFamily inet_netfamily(int family)
     return NETWORK_ADDRESS_FAMILY_UNKNOWN;
 }
 
+bool fd_is_socket(int fd)
+{
+    int optval;
+    socklen_t optlen;
+    optlen = sizeof(optval);
+    return qemu_getsockopt(fd,
+                           SOL_SOCKET,
+                           SO_TYPE,
+                           (char *)&optval,
+                           &optlen) == 0;
+}
+
+
 /*
  * Matrix we're trying to apply
  *
--

2.14.3

Signature · 1 line
Marc-André Lureau
#3 lore raw

PATCH 2/2 char: allow passing pre-opened socket file descriptor at startup

In reply to Daniel P. Berrange

When starting QEMU management apps will usually setup a monitor socket, and then open it immediately after startup. If not using QEMU's own -daemonize arg, this process can be troublesome to handle correctly. The mgmt app will need to repeatedly call connect() until it succeeds, because it does not know when QEMU has created the listener socket. If can't retry connect() forever though, because an error might have caused QEMU to exit before it even creates the monitor.

The obvious way to fix this kind of problem is to just pass in a pre-opened socket file descriptor for the QEMU monitor to listen on. The management app can now immediately call connect() just once. If connect() fails it knows that QEMU has exited with an error.

The SocketAddress(Legacy) structs allow for FD passing via the monitor, using the 'getfd' command, but only when using QMP JSON syntax. The HMP syntax has no way to initialize the SocketAddress(Legacy) 'fd' variant. So this patch first wires up the 'fd' parameter to refer to a monitor file descriptor, allowing HMP to use

getfd myfd
chardev-add socket,fd=myfd

The SocketAddress 'fd' variant is currently tied to the use of the monitor 'getfd' command, so we have a chicken & egg problem with reusing that at startup wher no monitor connection is available. We could define that the special fd name prefix '/dev/fdset' refers to a FD passed via the CLI, but magic strings feel unpleasant.

Instead we define a SocketAddress 'fdset' variant that takes an fd set number that works in combination with the 'add-fd' command line argument. e.g.

-add-fd fd=3,set=1
-chardev socket,fdset=1,id=mon
-mon chardev=mon,mode=control

Note that we do not wire this up in the legacy chardev syntax, so you cannot use FD passing with '-qmp', you must use the modern '-mon' + '-chardev' pair

An illustrative example of usage is:

#!/usr/bin/perl

use IO::Socket::UNIX;
use Fcntl;

unlink "/tmp/qmp";
my $srv = IO::Socket::UNIX->new(

    Type => SOCK_STREAM(),
    Local => "/tmp/qmp",
    Listen => 1,

);

  my $flags = fcntl $srv, F_GETFD, 0;
  fcntl $srv, F_SETFD, $flags & ~FD_CLOEXEC;

  my $fd = $srv->fileno();

  exec "qemu-system-x86_64", \
      "-add-fd", "fd=$fd,set=1", \
      "-chardev", "socket,fdset=1,server,nowait,id=mon", \
      "-mon", "chardev=mon,mode=control";
---
 chardev/char-socket.c | 66 +++++++++++++++++++++++++++++++++++++++++++++------
 chardev/char.c        |  6 +++++
 monitor.c             |  5 ++++
 qapi/common.json      | 11 +++++++++
 qapi/sockets.json     | 14 ++++++++---
 util/qemu-sockets.c   | 36 ++++++++++++++++++++++++++++
 6 files changed, 128 insertions(+), 10 deletions(-)
chardev/char-socket.c +59 -7
index 6013972f72..c85298589f 100644
@@ -28,6 +28,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
+#include "qemu/cutils.h"
 
 #include "chardev/char-io.h"
 
@@ -372,6 +373,10 @@ static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
         return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
                                is_listen ? ",server" : "");
         break;
+    case SOCKET_ADDRESS_TYPE_FDSET:
+        return g_strdup_printf("%sfdset:%" PRId64 "%s", prefix, addr->u.fdset.i,
+                               is_listen ? ",server" : "");
+        break;
     case SOCKET_ADDRESS_TYPE_VSOCK:
         return g_strdup_printf("%svsock:%s:%s", prefix,
                                addr->u.vsock.cid,
@@ -983,25 +988,62 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     const char *path = qemu_opt_get(opts, "path");
     const char *host = qemu_opt_get(opts, "host");
     const char *port = qemu_opt_get(opts, "port");
+    const char *fd = qemu_opt_get(opts, "fd");
+    const char *fdset = qemu_opt_get(opts, "fdset");
+    int64_t fdseti;
     const char *tls_creds = qemu_opt_get(opts, "tls-creds");
     SocketAddressLegacy *addr;
     ChardevSocket *sock;
+    int num = 0;
+
+    if (path) {
+        num++;
+    }
+    if (fd) {
+        num++;
+    }
+    if (fdset) {
+        num++;
+    }
+    if (host) {
+        num++;
+    }
+    if (num != 1) {
+        error_setg(errp,
+                   "Exactly one of 'path', 'fd', 'fdset' or 'host' required");
+        return;
+    }
 
     backend->type = CHARDEV_BACKEND_KIND_SOCKET;
-    if (!path) {
-        if (!host) {
-            error_setg(errp, "chardev: socket: no host given");
+    if (path) {
+        if (tls_creds) {
+            error_setg(errp, "TLS can only be used over TCP socket");
             return;
         }
+    } else if (host) {
         if (!port) {
             error_setg(errp, "chardev: socket: no port given");
             return;
         }
-    } else {
-        if (tls_creds) {
-            error_setg(errp, "TLS can only be used over TCP socket");
+    } else if (fd) {
+        /* We don't know what host to validate against when in client mode */
+        if (tls_creds && !is_listen) {
+            error_setg(errp, "TLS can not be used with pre-opened client FD");
+            return;
+        }
+    } else if (fdset) {
+        /* We don't know what host to validate against when in client mode */
+        if (tls_creds && !is_listen) {
+            error_setg(errp, "TLS can not be used with pre-opened client FD");
             return;
         }
+        if (qemu_strtoi64(fdset, NULL, 10, &fdseti) < 0) {
+            error_setg_errno(errp, errno,
+                             "Cannot parse fd set number %s", fdset);
+            return;
+        }
+    } else {
+        g_assert_not_reached();
     }
 
     sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
@@ -1027,7 +1069,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX;
         q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
         q_unix->path = g_strdup(path);
-    } else {
+    } else if (host) {
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
         addr->u.inet.data = g_new(InetSocketAddress, 1);
         *addr->u.inet.data = (InetSocketAddress) {
@@ -1040,6 +1082,16 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
             .has_ipv6 = qemu_opt_get(opts, "ipv6"),
             .ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
         };
+    } else if (fd) {
+        addr->type = SOCKET_ADDRESS_LEGACY_KIND_FD;
+        addr->u.fd.data = g_new(String, 1);
+        addr->u.fd.data->str = g_strdup(fd);
+    } else if (fdset) {
+        addr->type = SOCKET_ADDRESS_LEGACY_KIND_FDSET;
+        addr->u.fdset.data = g_new(Int, 1);
+        addr->u.fdset.data->i = fdseti;
+    } else {
+        g_assert_not_reached();
     }
     sock->addr = addr;
 }
chardev/char.c +6 -0
index 2ae4f465ec..db940fc40f 100644
@@ -798,6 +798,12 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "port",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "fd",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "fdset",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "localaddr",
             .type = QEMU_OPT_STRING,
monitor.c +5 -0
index d682eee2d8..c7df558535 100644
@@ -1962,6 +1962,11 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
 
+    if (mon == NULL) {
+        error_setg(errp, "No monitor is available to acquire FD");
+        return -1;
+    }
+
     QLIST_FOREACH(monfd, &mon->fds, next) {
         int fd;
 
qapi/common.json +11 -0
index 6eb01821ef..a15cdc36e9 100644
@@ -74,6 +74,17 @@
 { 'enum': 'OnOffSplit',
   'data': [ 'on', 'off', 'split' ] }
 
+##
+# @Int:
+#
+# A fat type wrapping 'int', to be embedded in lists.
+#
+# Since: 2.12
+##
+{ 'struct': 'Int',
+  'data': {
+    'i': 'int' } }
+
 ##
 # @String:
 #
qapi/sockets.json +11 -3
index ac022c6ad0..f3cac02166 100644
@@ -112,7 +112,8 @@
     'inet': 'InetSocketAddress',
     'unix': 'UnixSocketAddress',
     'vsock': 'VsockSocketAddress',
-    'fd': 'String' } }
+    'fd': 'String',
+    'fdset': 'Int' } }
 
 ##
 # @SocketAddressType:
@@ -123,10 +124,16 @@
 #
 # @unix:  Unix domain socket
 #
+# @vsock: VSOCK socket
+#
+# @fd: socket file descriptor passed over monitor
+#
+# @fdset: socket file descriptor passed via CLI (since 2.12)
+#
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressType',
-  'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
+  'data': [ 'inet', 'unix', 'vsock', 'fd', 'fdset' ] }
 
 ##
 # @SocketAddress:
@@ -144,4 +151,5 @@
   'data': { 'inet': 'InetSocketAddress',
             'unix': 'UnixSocketAddress',
             'vsock': 'VsockSocketAddress',
-            'fd': 'String' } }
+            'fd': 'String',
+            'fdset': 'Int' } }
util/qemu-sockets.c +36 -0
index 1d23f0b742..d623a9840c 100644
@@ -1049,6 +1049,22 @@ int socket_connect(SocketAddress *addr, Error **errp)
         fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);
         break;
 
+    case SOCKET_ADDRESS_TYPE_FDSET:
+        fd = monitor_fdset_get_fd(addr->u.fdset.i, O_RDWR);
+        if (fd < 0) {
+            error_setg_errno(errp, errno,
+                             "Unable to get FD from fdset %" PRId64,
+                             addr->u.fdset.i);
+            return -1;
+        }
+        if (!fd_is_socket(fd)) {
+            error_setg(errp, "Expected a socket FD from fdset %" PRId64,
+                       addr->u.fdset.i);
+            close(fd);
+            return -1;
+        }
+        break;
+
     case SOCKET_ADDRESS_TYPE_VSOCK:
         fd = vsock_connect_saddr(&addr->u.vsock, errp);
         break;
@@ -1076,6 +1092,22 @@ int socket_listen(SocketAddress *addr, Error **errp)
         fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);
         break;
 
+    case SOCKET_ADDRESS_TYPE_FDSET:
+        fd = monitor_fdset_get_fd(addr->u.fdset.i, O_RDWR);
+        if (fd < 0) {
+            error_setg_errno(errp, errno,
+                             "Unable to get FD from fdset %" PRId64,
+                             addr->u.fdset.i);
+            return -1;
+        }
+        if (!fd_is_socket(fd)) {
+            error_setg(errp, "Expected a socket FD from fdset %" PRId64,
+                       addr->u.fdset.i);
+            close(fd);
+            return -1;
+        }
+        break;
+
     case SOCKET_ADDRESS_TYPE_VSOCK:
         fd = vsock_listen_saddr(&addr->u.vsock, errp);
         break;
@@ -1293,6 +1325,10 @@ SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
         addr->type = SOCKET_ADDRESS_TYPE_FD;
         QAPI_CLONE_MEMBERS(String, &addr->u.fd, addr_legacy->u.fd.data);
         break;
+    case SOCKET_ADDRESS_LEGACY_KIND_FDSET:
+        addr->type = SOCKET_ADDRESS_TYPE_FDSET;
+        QAPI_CLONE_MEMBERS(Int, &addr->u.fdset, addr_legacy->u.fdset.data);
+        break;
     default:
         abort();
     }
Signature · 1 line
2.14.3
#4 lore raw

Re: [Qemu-devel] [PATCH v1 2/2] char: allow passing pre-opened socket file descriptor at startup

H

Quoted 5 lines · Daniel P. Berrange

When starting QEMU management apps will usually setup a monitor socket, and then open it immediately after startup. If not using QEMU's own -daemonize arg, this process can be troublesome to handle correctly. The mgmt app will need to repeatedly call connect() until it succeeds, because it does not know when QEMU has created the listener socket. If can't retry connect()

If->It

Quoted 8 lines

forever though, because an error might have caused QEMU to exit before it even creates the monitor.

The obvious way to fix this kind of problem is to just pass in a pre-opened socket file descriptor for the QEMU monitor to listen on. The management app can now immediately call connect() just once. If connect() fails it knows that QEMU has exited with an error.

Quoted 5 lines

The SocketAddress(Legacy) structs allow for FD passing via the monitor, using the 'getfd' command, but only when using QMP JSON syntax. The HMP syntax has no way to initialize the SocketAddress(Legacy) 'fd' variant. So this patch first wires up the 'fd' parameter to refer to a monitor file descriptor, allowing HMP to use

Make it a seperate patch? Do we need that feature in HMP?

Quoted 9 lines

getfd myfd
chardev-add socket,fd=myfd

The SocketAddress 'fd' variant is currently tied to the use of the monitor 'getfd' command, so we have a chicken & egg problem with reusing that at startup wher no monitor connection is available. We could define that the special fd name prefix '/dev/fdset' refers to a FD passed via the CLI, but magic strings feel unpleasant.

Ok

Quoted 88 lines

Instead we define a SocketAddress 'fdset' variant that takes an fd set number that works in combination with the 'add-fd' command line argument. e.g.

-add-fd fd=3,set=1
-chardev socket,fdset=1,id=mon
-mon chardev=mon,mode=control

Note that we do not wire this up in the legacy chardev syntax, so you cannot use FD passing with '-qmp', you must use the modern '-mon' + '-chardev' pair

An illustrative example of usage is:

#!/usr/bin/perl

use IO::Socket::UNIX;
use Fcntl;

unlink "/tmp/qmp";
my $srv = IO::Socket::UNIX->new(

    Type => SOCK_STREAM(),
    Local => "/tmp/qmp",
    Listen => 1,

);

  my $flags = fcntl $srv, F_GETFD, 0;
  fcntl $srv, F_SETFD, $flags & ~FD_CLOEXEC;

  my $fd = $srv->fileno();

  exec "qemu-system-x86_64", \
      "-add-fd", "fd=$fd,set=1", \
      "-chardev", "socket,fdset=1,server,nowait,id=mon", \
      "-mon", "chardev=mon,mode=control";
---
 chardev/char-socket.c | 66 +++++++++++++++++++++++++++++++++++++++++++++------
 chardev/char.c        |  6 +++++
 monitor.c             |  5 ++++
 qapi/common.json      | 11 +++++++++
 qapi/sockets.json     | 14 ++++++++---
 util/qemu-sockets.c   | 36 ++++++++++++++++++++++++++++
 6 files changed, 128 insertions(+), 10 deletions(-)
chardev/char-socket.c +18 -0
index 6013972f72..c85298589f 100644
@@ -28,6 +28,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
+#include "qemu/cutils.h"
 
 #include "chardev/char-io.h"
 
@@ -372,6 +373,10 @@ static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
         return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
                                is_listen ? ",server" : "");
         break;
+    case SOCKET_ADDRESS_TYPE_FDSET:
+        return g_strdup_printf("%sfdset:%" PRId64 "%s", prefix, addr->u.fdset.i,
+                               is_listen ? ",server" : "");
+        break;
     case SOCKET_ADDRESS_TYPE_VSOCK:
         return g_strdup_printf("%svsock:%s:%s", prefix,
                                addr->u.vsock.cid,
@@ -983,25 +988,62 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     const char *path = qemu_opt_get(opts, "path");
     const char *host = qemu_opt_get(opts, "host");
     const char *port = qemu_opt_get(opts, "port");
+    const char *fd = qemu_opt_get(opts, "fd");
+    const char *fdset = qemu_opt_get(opts, "fdset");
+    int64_t fdseti;
     const char *tls_creds = qemu_opt_get(opts, "tls-creds");
     SocketAddressLegacy *addr;
     ChardevSocket *sock;
+    int num = 0;
+
+    if (path) {
+        num++;
+    }
+    if (fd) {
+        num++;
+    }
+    if (fdset) {
+        num++;

chardev/char-socket.c static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, · line 1016

Quoted 10 lines
chardev/char-socket.c +9 -0
+    }
+    if (host) {
+        num++;
+    }
+    if (num != 1) {
+        error_setg(errp,
+                   "Exactly one of 'path', 'fd', 'fdset' or 'host' required");
+        return;
+    }
 

That could be shorter ;)

Quoted 221 lines
chardev/char-socket.c +32 -7
     backend->type = CHARDEV_BACKEND_KIND_SOCKET;
-    if (!path) {
-        if (!host) {
-            error_setg(errp, "chardev: socket: no host given");
+    if (path) {
+        if (tls_creds) {
+            error_setg(errp, "TLS can only be used over TCP socket");
             return;
         }
+    } else if (host) {
         if (!port) {
             error_setg(errp, "chardev: socket: no port given");
             return;
         }
-    } else {
-        if (tls_creds) {
-            error_setg(errp, "TLS can only be used over TCP socket");
+    } else if (fd) {
+        /* We don't know what host to validate against when in client mode */
+        if (tls_creds && !is_listen) {
+            error_setg(errp, "TLS can not be used with pre-opened client FD");
+            return;
+        }
+    } else if (fdset) {
+        /* We don't know what host to validate against when in client mode */
+        if (tls_creds && !is_listen) {
+            error_setg(errp, "TLS can not be used with pre-opened client FD");
             return;
         }
+        if (qemu_strtoi64(fdset, NULL, 10, &fdseti) < 0) {
+            error_setg_errno(errp, errno,
+                             "Cannot parse fd set number %s", fdset);
+            return;
+        }
+    } else {
+        g_assert_not_reached();
     }
 
     sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
@@ -1027,7 +1069,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX;
         q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
         q_unix->path = g_strdup(path);
-    } else {
+    } else if (host) {
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
         addr->u.inet.data = g_new(InetSocketAddress, 1);
         *addr->u.inet.data = (InetSocketAddress) {
@@ -1040,6 +1082,16 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
             .has_ipv6 = qemu_opt_get(opts, "ipv6"),
             .ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
         };
+    } else if (fd) {
+        addr->type = SOCKET_ADDRESS_LEGACY_KIND_FD;
+        addr->u.fd.data = g_new(String, 1);
+        addr->u.fd.data->str = g_strdup(fd);
+    } else if (fdset) {
+        addr->type = SOCKET_ADDRESS_LEGACY_KIND_FDSET;
+        addr->u.fdset.data = g_new(Int, 1);
+        addr->u.fdset.data->i = fdseti;
+    } else {
+        g_assert_not_reached();
     }
     sock->addr = addr;
 }
chardev/char.c +6 -0
index 2ae4f465ec..db940fc40f 100644
@@ -798,6 +798,12 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "port",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "fd",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "fdset",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "localaddr",
             .type = QEMU_OPT_STRING,
monitor.c +5 -0
index d682eee2d8..c7df558535 100644
@@ -1962,6 +1962,11 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
 
+    if (mon == NULL) {
+        error_setg(errp, "No monitor is available to acquire FD");
+        return -1;
+    }
+
     QLIST_FOREACH(monfd, &mon->fds, next) {
         int fd;
 
qapi/common.json +11 -0
index 6eb01821ef..a15cdc36e9 100644
@@ -74,6 +74,17 @@
 { 'enum': 'OnOffSplit',
   'data': [ 'on', 'off', 'split' ] }
 
+##
+# @Int:
+#
+# A fat type wrapping 'int', to be embedded in lists.
+#
+# Since: 2.12
+##
+{ 'struct': 'Int',
+  'data': {
+    'i': 'int' } }
+
 ##
 # @String:
 #
qapi/sockets.json +11 -3
index ac022c6ad0..f3cac02166 100644
@@ -112,7 +112,8 @@
     'inet': 'InetSocketAddress',
     'unix': 'UnixSocketAddress',
     'vsock': 'VsockSocketAddress',
-    'fd': 'String' } }
+    'fd': 'String',
+    'fdset': 'Int' } }
 
 ##
 # @SocketAddressType:
@@ -123,10 +124,16 @@
 #
 # @unix:  Unix domain socket
 #
+# @vsock: VSOCK socket
+#
+# @fd: socket file descriptor passed over monitor
+#
+# @fdset: socket file descriptor passed via CLI (since 2.12)
+#
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressType',
-  'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
+  'data': [ 'inet', 'unix', 'vsock', 'fd', 'fdset' ] }
 
 ##
 # @SocketAddress:
@@ -144,4 +151,5 @@
   'data': { 'inet': 'InetSocketAddress',
             'unix': 'UnixSocketAddress',
             'vsock': 'VsockSocketAddress',
-            'fd': 'String' } }
+            'fd': 'String',
+            'fdset': 'Int' } }
util/qemu-sockets.c +36 -1
index 1d23f0b742..d623a9840c 100644
@@ -1049,6 +1049,22 @@ int socket_connect(SocketAddress *addr, Error **errp)
         fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);
         break;
 
+    case SOCKET_ADDRESS_TYPE_FDSET:
+        fd = monitor_fdset_get_fd(addr->u.fdset.i, O_RDWR);
+        if (fd < 0) {
+            error_setg_errno(errp, errno,
+                             "Unable to get FD from fdset %" PRId64,
+                             addr->u.fdset.i);
+            return -1;
+        }
+        if (!fd_is_socket(fd)) {
+            error_setg(errp, "Expected a socket FD from fdset %" PRId64,
+                       addr->u.fdset.i);
+            close(fd);
+            return -1;
+        }
+        break;
+
     case SOCKET_ADDRESS_TYPE_VSOCK:
         fd = vsock_connect_saddr(&addr->u.vsock, errp);
         break;
@@ -1076,6 +1092,22 @@ int socket_listen(SocketAddress *addr, Error **errp)
         fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);
         break;
 
+    case SOCKET_ADDRESS_TYPE_FDSET:
+        fd = monitor_fdset_get_fd(addr->u.fdset.i, O_RDWR);
+        if (fd < 0) {
+            error_setg_errno(errp, errno,
+                             "Unable to get FD from fdset %" PRId64,
+                             addr->u.fdset.i);
+            return -1;
+        }
+        if (!fd_is_socket(fd)) {
+            error_setg(errp, "Expected a socket FD from fdset %" PRId64,
+                       addr->u.fdset.i);
+            close(fd);
+            return -1;
+        }
+        break;
+
     case SOCKET_ADDRESS_TYPE_VSOCK:
         fd = vsock_listen_saddr(&addr->u.vsock, errp);
         break;
@@ -1293,6 +1325,10 @@ SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
         addr->type = SOCKET_ADDRESS_TYPE_FD;
         QAPI_CLONE_MEMBERS(String, &addr->u.fd, addr_legacy->u.fd.data);
         break;
+    case SOCKET_ADDRESS_LEGACY_KIND_FDSET:
+        addr->type = SOCKET_ADDRESS_TYPE_FDSET;
+        QAPI_CLONE_MEMBERS(Int, &addr->u.fdset, addr_legacy->u.fdset.data);
+        break;
     default:
         abort();
     }
--
Quoted 3 lines

2.14.3

Looks fine, I would prefer the patch to be split, and some tests added, please :)

thanks

Signature · 1 line
Marc-André Lureau
#5 lore raw

Re: [Qemu-devel] [PATCH v1 2/2] char: allow passing pre-opened socket file descriptor at startup

Quoted 22 lines · Marc-André Lureau
Quoted 11 lines
      ChardevSocket *sock;
chardev/char-socket.c +9 -0
+    int num = 0;
+
+    if (path) {
+        num++;
+    }
+    if (fd) {
+        num++;
+    }
+    if (fdset) {

chardev/char-socket.c static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, · line 1015

Quoted 10 lines
chardev/char-socket.c +10 -0
+        num++;
+    }
+    if (host) {
+        num++;
+    }
+    if (num != 1) {
+        error_setg(errp,
+                   "Exactly one of 'path', 'fd', 'fdset' or 'host' required");
+        return;
+    }
Quoted 1 line

That could be shorter ;)

Here's one way:

if (!!path + !!fd + !!fdset ++ !!host) {
    error_setg...
Quoted 5 lines
Quoted 5 lines

+++ b/qapi/common.json

qapi/common.json +0 -0
@@ -74,6 +74,17 @@
  { 'enum': 'OnOffSplit',
    'data': [ 'on', 'off', 'split' ] }
 
Quoted 11 lines

qapi/common.json line 87

Quoted 11 lines
qapi/common.json +11 -0
+##
+# @Int:
+#
+# A fat type wrapping 'int', to be embedded in lists.
+#
+# Since: 2.12
+##
+{ 'struct': 'Int',
+  'data': {
+    'i': 'int' } }
+

The documentation is odd - you need this type because flat unions require a struct rather than a native type for each branch, and not because you are embedding 'int' in lists (that is, we support ['int'] just fine; the documentation for other fat types pre-dates when we supported lists of native types).

Quoted 6 lines
Quoted 6 lines
qapi/common.json +0 -0
  ##
  # @String:
  #
qapi/sockets.json +0 -1
index ac022c6ad0..f3cac02166 100644
--- a/qapi/sockets.json
Quoted 8 lines

qapi/sockets.json line 116

Quoted 8 lines

+++ b/qapi/sockets.json

qapi/sockets.json +2 -1
@@ -112,7 +112,8 @@
      'inet': 'InetSocketAddress',
      'unix': 'UnixSocketAddress',
      'vsock': 'VsockSocketAddress',
-    'fd': 'String' } }
+    'fd': 'String',
+    'fdset': 'Int' } }

This is SocketAddressLegacy, which is an old-style union. Do we really want to be expanding it at this point in time? Old-style unions support:

'fd':'str',
'fdset':'int'

except that we already used the fat 'String', so using the fat 'Int' is done for consistency more than anything else, if we really do want it added to this type.

Missing documentation of when the new branch types were added.

Quoted 10 lines
Quoted 10 lines
qapi/sockets.json +3 -0
  ##
  # @SocketAddressType:
@@ -123,10 +124,16 @@
  #
  # @unix:  Unix domain socket
  #
+# @vsock: VSOCK socket
+#
+# @fd: socket file descriptor passed over monitor
Quoted 8 lines

qapi/sockets.json line 136

Quoted 8 lines
qapi/sockets.json +4 -1
+#
+# @fdset: socket file descriptor passed via CLI (since 2.12)
+#
  # Since: 2.9
  ##
  { 'enum': 'SocketAddressType',
-  'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
+  'data': [ 'inet', 'unix', 'vsock', 'fd', 'fdset' ] }

Hmm, good catch on the missing vsock documentation (SocketAddressType was named SocketAddressFlatType back in 2.9, but 'vsock' did exist back then).

Quoted 10 lines

qapi/sockets.json line 155

Quoted 10 lines
qapi/sockets.json +2 -1
  ##
  # @SocketAddress:
@@ -144,4 +151,5 @@
    'data': { 'inet': 'InetSocketAddress',
              'unix': 'UnixSocketAddress',
              'vsock': 'VsockSocketAddress',
-            'fd': 'String' } }
+            'fd': 'String',
+            'fdset': 'Int' } }

Again, missing documentation on when the branch was added.

But here, you are absolutely correct that we need the fat 'Int' here, as this is a flat union, which requires a struct for each branch (not native scalar types).

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#6 lore raw

Re: [Qemu-devel] [PATCH v1 2/2] char: allow passing pre-opened socket file descriptor at startup

In reply to Daniel P. Berrange
Quoted 28 lines · Daniel P. Berrange

When starting QEMU management apps will usually setup a monitor socket, and then open it immediately after startup. If not using QEMU's own -daemonize arg, this process can be troublesome to handle correctly. The mgmt app will need to repeatedly call connect() until it succeeds, because it does not know when QEMU has created the listener socket. If can't retry connect() forever though, because an error might have caused QEMU to exit before it even creates the monitor.

The obvious way to fix this kind of problem is to just pass in a pre-opened socket file descriptor for the QEMU monitor to listen on. The management app can now immediately call connect() just once. If connect() fails it knows that QEMU has exited with an error.

The SocketAddress(Legacy) structs allow for FD passing via the monitor, using the 'getfd' command, but only when using QMP JSON syntax. The HMP syntax has no way to initialize the SocketAddress(Legacy) 'fd' variant. So this patch first wires up the 'fd' parameter to refer to a monitor file descriptor, allowing HMP to use

getfd myfd
chardev-add socket,fd=myfd

The SocketAddress 'fd' variant is currently tied to the use of the monitor 'getfd' command, so we have a chicken & egg problem with reusing that at startup wher no monitor connection is available. We could define that the special fd name prefix '/dev/fdset' refers to a FD passed via the CLI, but magic strings feel unpleasant.

Quoted 6 lines

Instead we define a SocketAddress 'fdset' variant that takes an fd set number that works in combination with the 'add-fd' command line argument. e.g.

-add-fd fd=3,set=1
-chardev socket,fdset=1,id=mon
-mon chardev=mon,mode=control

Having written this, it occurs to me that using fdset for this is really just adding uncessary complication.

The 'fd' parameter in SocketAddress is required to be a string that refers to a named FD passed over the monitor. I now see, however, that these strings are forbidden to start with a digit. This means we could easily re-use this facility to just directly reference a passed-in file descriptor number, without clashing with named FD strings. eg we could do

-chardev socket,fd=3,id=mon
-mon chardev=mon,mode=control

This would simplify this patch significantly, and give close alignement between the HMP & cli usage.

Regards,
Daniel

Signature · 3 lines
|: 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 :|
#7 lore raw

Re: [Qemu-devel] [PATCH v1 2/2] char: allow passing pre-opened socket file descriptor at startup

In reply to Daniel P. Berrange

QAPI schema review only.

Quoted 19 lines · Daniel P. Berrange

When starting QEMU management apps will usually setup a monitor socket, and then open it immediately after startup. If not using QEMU's own -daemonize arg, this process can be troublesome to handle correctly. The mgmt app will need to repeatedly call connect() until it succeeds, because it does not know when QEMU has created the listener socket. If can't retry connect() forever though, because an error might have caused QEMU to exit before it even creates the monitor.

The obvious way to fix this kind of problem is to just pass in a pre-opened socket file descriptor for the QEMU monitor to listen on. The management app can now immediately call connect() just once. If connect() fails it knows that QEMU has exited with an error.

The SocketAddress(Legacy) structs allow for FD passing via the monitor, using the 'getfd' command, but only when using QMP JSON syntax. The HMP syntax has no way to initialize the SocketAddress(Legacy) 'fd' variant. So this patch first wires up the 'fd' parameter to refer to a monitor file descriptor, allowing HMP to use

Quoted 6 lines

getfd myfd
chardev-add socket,fd=myfd

The SocketAddress 'fd' variant is currently tied to the use of the monitor 'getfd' command, so we have a chicken & egg problem with reusing that at startup wher no monitor connection is available. We could define that the

s/wher/where/

Quoted 38 lines

special fd name prefix '/dev/fdset' refers to a FD passed via the CLI, but magic strings feel unpleasant.

Instead we define a SocketAddress 'fdset' variant that takes an fd set number that works in combination with the 'add-fd' command line argument. e.g.

-add-fd fd=3,set=1
-chardev socket,fdset=1,id=mon
-mon chardev=mon,mode=control

Note that we do not wire this up in the legacy chardev syntax, so you cannot use FD passing with '-qmp', you must use the modern '-mon' + '-chardev' pair

An illustrative example of usage is:

#!/usr/bin/perl

use IO::Socket::UNIX;
use Fcntl;

unlink "/tmp/qmp";
my $srv = IO::Socket::UNIX->new(

    Type => SOCK_STREAM(),
    Local => "/tmp/qmp",
    Listen => 1,

);

  my $flags = fcntl $srv, F_GETFD, 0;
  fcntl $srv, F_SETFD, $flags & ~FD_CLOEXEC;

  my $fd = $srv->fileno();

  exec "qemu-system-x86_64", \
      "-add-fd", "fd=$fd,set=1", \
      "-chardev", "socket,fdset=1,server,nowait,id=mon", \
      "-mon", "chardev=mon,mode=control";

[...]

qapi/common.json line 80

Quoted 12 lines
qapi/common.json +4 -0
index 6eb01821ef..a15cdc36e9 100644
@@ -74,6 +74,17 @@
 { 'enum': 'OnOffSplit',
   'data': [ 'on', 'off', 'split' ] }
 
+##
+# @Int:
+#
+# A fat type wrapping 'int', to be embedded in lists.

I figure you got the "to be embedded in lists" part from @String. That one's occasionally used as list element type, but there are other uses. @Int has only such other uses so far. Let's drop this line from both types.

Quoted 25 lines
qapi/common.json +7 -0
+#
+# Since: 2.12
+##
+{ 'struct': 'Int',
+  'data': {
+    'i': 'int' } }
+
 ##
 # @String:
 #
qapi/sockets.json +2 -1
index ac022c6ad0..f3cac02166 100644
@@ -112,7 +112,8 @@
     'inet': 'InetSocketAddress',
     'unix': 'UnixSocketAddress',
     'vsock': 'VsockSocketAddress',
-    'fd': 'String' } }
+    'fd': 'String',
+    'fdset': 'Int' } }
 
 ##
 # @SocketAddressType:

qapi/sockets.json line 130

Quoted 7 lines
qapi/sockets.json +4 -0
@@ -123,10 +124,16 @@
 #
 # @unix:  Unix domain socket
 #
+# @vsock: VSOCK socket
+#
+# @fd: socket file descriptor passed over monitor
+#

Indepedent doc fix. I'd put it in a separate patch.

One inaccuracy: @fd is *not* a file descriptor, it's the *name* of a file descriptor. Please fix.

qapi/sockets.json line 132

Quoted 2 lines
qapi/sockets.json +2 -0
+# @fdset: socket file descriptor passed via CLI (since 2.12)
+#

I gather we have to ways to pass file descriptors. One way identifies them by name (member @fd), the other by numeric ID (member @fdset).

0. This is disgusting. Is there any way to unify the two, and deprecate the loser (hopefully the numeric one)?

1. What makes the second one a *set*?

2. What ties the second one to the CLI? Accidents of implementation or something deeper?

Quoted 15 lines
qapi/sockets.json +3 -2
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressType',
-  'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
+  'data': [ 'inet', 'unix', 'vsock', 'fd', 'fdset' ] }
 
 ##
 # @SocketAddress:
@@ -144,4 +151,5 @@
   'data': { 'inet': 'InetSocketAddress',
             'unix': 'UnixSocketAddress',
             'vsock': 'VsockSocketAddress',
-            'fd': 'String' } }
+            'fd': 'String',
+            'fdset': 'Int' } }

[...]

#8 lore raw

Re: [Qemu-devel] [PATCH v1 2/2] char: allow passing pre-opened socket file descriptor at startup

Quoted 126 lines · Markus Armbruster

QAPI schema review only.

Quoted 19 lines · Daniel P. Berrange

When starting QEMU management apps will usually setup a monitor socket, and then open it immediately after startup. If not using QEMU's own -daemonize arg, this process can be troublesome to handle correctly. The mgmt app will need to repeatedly call connect() until it succeeds, because it does not know when QEMU has created the listener socket. If can't retry connect() forever though, because an error might have caused QEMU to exit before it even creates the monitor.

The obvious way to fix this kind of problem is to just pass in a pre-opened socket file descriptor for the QEMU monitor to listen on. The management app can now immediately call connect() just once. If connect() fails it knows that QEMU has exited with an error.

The SocketAddress(Legacy) structs allow for FD passing via the monitor, using the 'getfd' command, but only when using QMP JSON syntax. The HMP syntax has no way to initialize the SocketAddress(Legacy) 'fd' variant. So this patch first wires up the 'fd' parameter to refer to a monitor file descriptor, allowing HMP to use

Quoted 6 lines

getfd myfd
chardev-add socket,fd=myfd

The SocketAddress 'fd' variant is currently tied to the use of the monitor 'getfd' command, so we have a chicken & egg problem with reusing that at startup wher no monitor connection is available. We could define that the

s/wher/where/

Quoted 38 lines

special fd name prefix '/dev/fdset' refers to a FD passed via the CLI, but magic strings feel unpleasant.

Instead we define a SocketAddress 'fdset' variant that takes an fd set number that works in combination with the 'add-fd' command line argument. e.g.

-add-fd fd=3,set=1
-chardev socket,fdset=1,id=mon
-mon chardev=mon,mode=control

Note that we do not wire this up in the legacy chardev syntax, so you cannot use FD passing with '-qmp', you must use the modern '-mon' + '-chardev' pair

An illustrative example of usage is:

#!/usr/bin/perl

use IO::Socket::UNIX;
use Fcntl;

unlink "/tmp/qmp";
my $srv = IO::Socket::UNIX->new(

    Type => SOCK_STREAM(),
    Local => "/tmp/qmp",
    Listen => 1,

);

  my $flags = fcntl $srv, F_GETFD, 0;
  fcntl $srv, F_SETFD, $flags & ~FD_CLOEXEC;

  my $fd = $srv->fileno();

  exec "qemu-system-x86_64", \
      "-add-fd", "fd=$fd,set=1", \
      "-chardev", "socket,fdset=1,server,nowait,id=mon", \
      "-mon", "chardev=mon,mode=control";

[...]

qapi/common.json line 80

Quoted 12 lines
qapi/common.json +4 -0
index 6eb01821ef..a15cdc36e9 100644
@@ -74,6 +74,17 @@
 { 'enum': 'OnOffSplit',
   'data': [ 'on', 'off', 'split' ] }
 
+##
+# @Int:
+#
+# A fat type wrapping 'int', to be embedded in lists.

I figure you got the "to be embedded in lists" part from @String. That one's occasionally used as list element type, but there are other uses. @Int has only such other uses so far. Let's drop this line from both types.

Quoted 25 lines
qapi/common.json +7 -0
+#
+# Since: 2.12
+##
+{ 'struct': 'Int',
+  'data': {
+    'i': 'int' } }
+
 ##
 # @String:
 #
qapi/sockets.json +2 -1
index ac022c6ad0..f3cac02166 100644
@@ -112,7 +112,8 @@
     'inet': 'InetSocketAddress',
     'unix': 'UnixSocketAddress',
     'vsock': 'VsockSocketAddress',
-    'fd': 'String' } }
+    'fd': 'String',
+    'fdset': 'Int' } }
 
 ##
 # @SocketAddressType:

qapi/sockets.json line 130

Quoted 7 lines
qapi/sockets.json +4 -0
@@ -123,10 +124,16 @@
 #
 # @unix:  Unix domain socket
 #
+# @vsock: VSOCK socket
+#
+# @fd: socket file descriptor passed over monitor
+#

Indepedent doc fix. I'd put it in a separate patch.

One inaccuracy: @fd is *not* a file descriptor, it's the *name* of a file descriptor. Please fix.

Quoted 8 lines

qapi/sockets.json line 132

Quoted 2 lines
qapi/sockets.json +2 -0
+# @fdset: socket file descriptor passed via CLI (since 2.12)
+#

I gather we have to ways to pass file descriptors. One way identifies them by name (member @fd), the other by numeric ID (member @fdset).

0. This is disgusting. Is there any way to unify the two, and deprecate the loser (hopefully the numeric one)?

Checkout my v2 which takes a different, less disgusting approach.

Quoted 1 line

1. What makes the second one a *set*?

Just that the API i used was called fdset.

Quoted 2 lines

2. What ties the second one to the CLI? Accidents of implementation or something deeper?

Nothing strict - just convention of usage. You could technically (ab)use it from the monitor too. My v2 approach enforces the distinct usage more strictly.

Quoted 7 lines
Quoted 7 lines
qapi/sockets.json +1 -1
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressType',
-  'data': [ 'inet', 'unix', 'vsock', 'fd' ] }
+  'data': [ 'inet', 'unix', 'vsock', 'fd', 'fdset' ] }
 
Quoted 10 lines
Quoted 9 lines
qapi/sockets.json +2 -1
 ##
 # @SocketAddress:
@@ -144,4 +151,5 @@
   'data': { 'inet': 'InetSocketAddress',
             'unix': 'UnixSocketAddress',
             'vsock': 'VsockSocketAddress',
-            'fd': 'String' } }
+            'fd': 'String',
+            'fdset': 'Int' } }

[...]

Regards,
Daniel

Signature · 3 lines
|: 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 :|
#9 lore raw

Re: [Qemu-devel] [PATCH v1 2/2] char: allow passing pre-opened socket file descriptor at startup

In reply to Markus Armbruster
Quoted 13 lines · Markus Armbruster

QAPI schema review only.

qapi/sockets.json line 130

Quoted 8 lines
qapi/sockets.json +4 -0
@@ -123,10 +124,16 @@
  #
  # @unix:  Unix domain socket
  #
+# @vsock: VSOCK socket
+#
+# @fd: socket file descriptor passed over monitor
+#

Indepedent doc fix. I'd put it in a separate patch.

Missed in v2, so we still need that.

Quoted 9 lines

One inaccuracy: @fd is *not* a file descriptor, it's the *name* of a file descriptor. Please fix.

qapi/sockets.json line 132

Quoted 2 lines
qapi/sockets.json +2 -0
+# @fdset: socket file descriptor passed via CLI (since 2.12)
+#

I gather we have to ways to pass file descriptors. One way identifies them by name (member @fd), the other by numeric ID (member @fdset).

Yes, sort of.

Quoted 3 lines

0. This is disgusting. Is there any way to unify the two, and deprecate the loser (hopefully the numeric one)?

v2 went with the numeric one for the CLI, as it is less verbose.

Quoted 2 lines

1. What makes the second one a *set*?

We have two existing mechanisms for fd passing:

a) HMP and QMP have the older 'getfd' command since 0.14, which associates a single fd (via SCM_RIGHTS) with a name (which cannot start with a digit). This does not scale - every command that wants to use an fd like this has to be updated to take an explicit fd argument; and you can't alter permissions on the fd after the fact.

b) QMP added the newer 'add-fd' in 1.2, which allows the association of multiple fds (via SCM_RIGHTS) with a set number (the set is then accessed via the magic prefix /dev/fdset/NNN. This was also exposed via the command line, via '--add-fd', at the same time. It also scales better, in that any command that already takes a filename now operates on fdsets as needed; also, you can associate multiple fds to the set (one that is read-only, another that is read-write), and qemu will alternate between fds according to permissions needed at the time.

New code targetting QMP should use only 'add-fd'; particularly since -addfd is the only version that works via CLI, even if there is only one fd in the set. But it is more verbose, so having CLI shorthand that takes just an fd number is sometimes acceptable.

Quoted 4 lines

2. What ties the second one to the CLI? Accidents of implementation or something deeper?

At this point, more history than anything else.

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