[Qemu-devel] [PATCH v1 0/2] Enable passing pre-opened chardev socket FDs

[Qemu-devel] [PATCH v1 0/2] Enable passing pre-opened chardev socket FDs

[Daniel P. Berrange]

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

-- 
2.14.3

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

[Daniel P. Berrange]

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

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qemu/sockets.h |  1 +
 io/channel-util.c      | 13 -------------
 util/qemu-sockets.c    | 13 +++++++++++++
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 4f7311b52a..5680880f5a 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -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);
diff --git a/io/channel-util.c b/io/channel-util.c
index 0fb4bd0837..423d79845a 100644
--- a/io/channel-util.c
+++ b/io/channel-util.c
@@ -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)
 {
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index af4f01211a..1d23f0b742 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -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

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

[Marc-André Lureau]

Hi

On Thu, Dec 21, 2017 at 2:27 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
> The fd_is_socket() helper method is useful in a few places, so put it in
> the common sockets code.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

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


> ---
>  include/qemu/sockets.h |  1 +
>  io/channel-util.c      | 13 -------------
>  util/qemu-sockets.c    | 13 +++++++++++++
>  3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 4f7311b52a..5680880f5a 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -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);
> diff --git a/io/channel-util.c b/io/channel-util.c
> index 0fb4bd0837..423d79845a 100644
> --- a/io/channel-util.c
> +++ b/io/channel-util.c
> @@ -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)
>  {
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index af4f01211a..1d23f0b742 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -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
>
>



-- 
Marc-André Lureau

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

[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";

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 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(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 6013972f72..c85298589f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -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;
 }
diff --git a/chardev/char.c b/chardev/char.c
index 2ae4f465ec..db940fc40f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -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,
diff --git a/monitor.c b/monitor.c
index d682eee2d8..c7df558535 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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;
 
diff --git a/qapi/common.json b/qapi/common.json
index 6eb01821ef..a15cdc36e9 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -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:
 #
diff --git a/qapi/sockets.json b/qapi/sockets.json
index ac022c6ad0..f3cac02166 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -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' } }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 1d23f0b742..d623a9840c 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -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();
     }
-- 
2.14.3

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

[Marc-André Lureau]

H

On Thu, Dec 21, 2017 at 2:27 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
> 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

> 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

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

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

>
> 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";
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  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(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 6013972f72..c85298589f 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -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;
> +    }
>

That could be shorter ;)

>      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;
>  }
> diff --git a/chardev/char.c b/chardev/char.c
> index 2ae4f465ec..db940fc40f 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -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,
> diff --git a/monitor.c b/monitor.c
> index d682eee2d8..c7df558535 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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;
>
> diff --git a/qapi/common.json b/qapi/common.json
> index 6eb01821ef..a15cdc36e9 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -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:
>  #
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index ac022c6ad0..f3cac02166 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -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' } }
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 1d23f0b742..d623a9840c 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -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();
>      }
> --
> 2.14.3
>
>

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

thanks

-- 
Marc-André Lureau

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

[Eric Blake]

On 12/21/2017 07:48 AM, Marc-André Lureau wrote:

>>       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;
>> +    }
>>
> 
> That could be shorter ;)

Here's one way:

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

>> +++ b/qapi/common.json
>> @@ -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' } }
>> +

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

>>   ##
>>   # @String:
>>   #
>> diff --git a/qapi/sockets.json b/qapi/sockets.json
>> index ac022c6ad0..f3cac02166 100644
>> --- a/qapi/sockets.json
>> +++ b/qapi/sockets.json
>> @@ -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.

>>
>>   ##
>>   # @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' ] }

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

>>
>>   ##
>>   # @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).

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

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

[Daniel P. Berrange]

On Thu, Dec 21, 2017 at 01:27:17PM +0000, Daniel P. Berrange wrote:
> 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

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Markus Armbruster]

QAPI schema review only.

"Daniel P. Berrange" <berrange@redhat.com> writes:

> 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

s/wher/where/

> 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";
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
[...]
> diff --git a/qapi/common.json b/qapi/common.json
> index 6eb01821ef..a15cdc36e9 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -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.

> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'Int',
> +  'data': {
> +    'i': 'int' } }
> +
>  ##
>  # @String:
>  #
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index ac022c6ad0..f3cac02166 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -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
> +#

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.

> +# @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?

>  # 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' } }
[...]

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

[Daniel P. Berrange]

On Thu, Dec 21, 2017 at 05:49:14PM +0100, Markus Armbruster wrote:
> QAPI schema review only.
> 
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > 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
> 
> s/wher/where/
> 
> > 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";
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> [...]
> > diff --git a/qapi/common.json b/qapi/common.json
> > index 6eb01821ef..a15cdc36e9 100644
> > --- a/qapi/common.json
> > +++ b/qapi/common.json
> > @@ -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.
> 
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'Int',
> > +  'data': {
> > +    'i': 'int' } }
> > +
> >  ##
> >  # @String:
> >  #
> > diff --git a/qapi/sockets.json b/qapi/sockets.json
> > index ac022c6ad0..f3cac02166 100644
> > --- a/qapi/sockets.json
> > +++ b/qapi/sockets.json
> > @@ -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
> > +#
> 
> 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.
> 
> > +# @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.

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

Just that the API i used was called fdset.

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

> 
> >  # 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' } }
> [...]

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

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

[Eric Blake]

On 12/21/2017 10:49 AM, Markus Armbruster wrote:
> QAPI schema review only.
> 

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

> 
> One inaccuracy: @fd is *not* a file descriptor, it's the *name* of a
> file descriptor.  Please fix.
> 
>> +# @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.

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

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

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

At this point, more history than anything else.

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