[Qemu-devel] [PATCH v2 0/5] Convert qemu-socket to use QAPI exclusively

[Qemu-devel] [PATCH v2 0/5] Convert qemu-socket to use QAPI exclusively

[Daniel P. Berrange]

A second posting of

 v1: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04950.html

All the callers of the qemu-sockets module now use the APIs
which take a QAPI SocketAddress.

Thus we now have the fun situation with VNC & Chardevs that
they use QemuOpts to parse the CLI args, then convert to
a SocketAddress, pass it into qemu-sockets which converts
it back into a QemuOpts instance.

This series rips out all usage of QemuOpts from qemu-sockets
so that the code exclusively uses QAPI SocketAddress objects.
Now when parsing CLI args, we just convert from QemuOpts to
SocketAddress and use that directly, and when using the
monitor there's no conversion at all, we have SocketAddress
all the way.

This conversion also fixes a bug in the code where use of
ipv4=off and ipv6=off at the same time, resulted in using
PF_UNSPEC and so was in effective equivalent to using
ipv4=on and ipv6=on.  We now report an explicit error for
the ipv4=off + ipv6=off scenario, since it is an invalid
request to make.

Finally, the VNC code is fixed to honour the distinction
between ipv4/ipv6 being omitted vs set to 'off'.

Changed in v2:

 - Optimization suggested by Paolo
 - Removed accidental debug g_printerr()s
 - Resolved conflicts with QAPI generator changes
   that renamed various struct fields

Daniel P. Berrange (5):
  sockets: remove use of QemuOpts from header file
  sockets: remove use of QemuOpts from socket_listen
  sockets: remove use of QemuOpts from socket_connect
  sockets: remove use of QemuOpts from socket_dgram
  vnc: distiguish between ipv4/ipv6 omitted vs set to off

 include/qemu/sockets.h |  10 --
 ui/vnc.c               |  18 ++-
 util/qemu-sockets.c    | 334 ++++++++++++++++++++++---------------------------
 3 files changed, 159 insertions(+), 203 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 1/5] sockets: remove use of QemuOpts from header file

[Daniel P. Berrange]

There are no callers of the sockets methods which accept
QemuOpts any more. Make all the QemuOpts related functions
static to avoid new callers being added, in preparation
for removal of all QemuOpts usage, in favour of QAPI
SocketAddress.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qemu/sockets.h |  9 ---------
 util/qemu-sockets.c    | 22 +++++++++++-----------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5a183c5..2741b97 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -30,8 +30,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "qapi/error.h"
 #include "qapi-types.h"
 
-extern QemuOptsList socket_optslist;
-
 /* misc helpers */
 int qemu_socket(int domain, int type, int protocol);
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
@@ -56,23 +54,16 @@ int recv_all(int fd, void *buf, int len1, bool single_read);
 typedef void NonBlockingConnectHandler(int fd, Error *errp, void *opaque);
 
 InetSocketAddress *inet_parse(const char *str, Error **errp);
-int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp);
-int inet_connect_opts(QemuOpts *opts, Error **errp,
-                      NonBlockingConnectHandler *callback, void *opaque);
 int inet_connect(const char *str, Error **errp);
 int inet_nonblocking_connect(const char *str,
                              NonBlockingConnectHandler *callback,
                              void *opaque, Error **errp);
 
-int inet_dgram_opts(QemuOpts *opts, Error **errp);
 NetworkAddressFamily inet_netfamily(int family);
 
-int unix_listen_opts(QemuOpts *opts, Error **errp);
 int unix_listen(const char *path, char *ostr, int olen, Error **errp);
-int unix_connect_opts(QemuOpts *opts, Error **errp,
-                      NonBlockingConnectHandler *callback, void *opaque);
 int unix_connect(const char *path, Error **errp);
 int unix_nonblocking_connect(const char *str,
                              NonBlockingConnectHandler *callback,
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 5a31d16..324d3af 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -37,7 +37,7 @@
 #endif
 
 /* used temporarily until all users are converted to QemuOpts */
-QemuOptsList socket_optslist = {
+static QemuOptsList socket_optslist = {
     .name = "socket",
     .head = QTAILQ_HEAD_INITIALIZER(socket_optslist.head),
     .desc = {
@@ -114,7 +114,7 @@ NetworkAddressFamily inet_netfamily(int family)
     return NETWORK_ADDRESS_FAMILY_UNKNOWN;
 }
 
-int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
+static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
 {
     struct addrinfo ai,*res,*e;
     const char *addr;
@@ -392,8 +392,8 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
  * function succeeds, callback will be called when the connection
  * completes, with the file descriptor on success, or -1 on error.
  */
-int inet_connect_opts(QemuOpts *opts, Error **errp,
-                      NonBlockingConnectHandler *callback, void *opaque)
+static int inet_connect_opts(QemuOpts *opts, Error **errp,
+                             NonBlockingConnectHandler *callback, void *opaque)
 {
     Error *local_err = NULL;
     struct addrinfo *res, *e;
@@ -440,7 +440,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
     return sock;
 }
 
-int inet_dgram_opts(QemuOpts *opts, Error **errp)
+static int inet_dgram_opts(QemuOpts *opts, Error **errp)
 {
     struct addrinfo ai, *peer = NULL, *local = NULL;
     const char *addr;
@@ -708,7 +708,7 @@ int inet_nonblocking_connect(const char *str,
 
 #ifndef _WIN32
 
-int unix_listen_opts(QemuOpts *opts, Error **errp)
+static int unix_listen_opts(QemuOpts *opts, Error **errp)
 {
     struct sockaddr_un un;
     const char *path = qemu_opt_get(opts, "path");
@@ -772,8 +772,8 @@ err:
     return -1;
 }
 
-int unix_connect_opts(QemuOpts *opts, Error **errp,
-                      NonBlockingConnectHandler *callback, void *opaque)
+static int unix_connect_opts(QemuOpts *opts, Error **errp,
+                             NonBlockingConnectHandler *callback, void *opaque)
 {
     struct sockaddr_un un;
     const char *path = qemu_opt_get(opts, "path");
@@ -832,15 +832,15 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
 
 #else
 
-int unix_listen_opts(QemuOpts *opts, Error **errp)
+static int unix_listen_opts(QemuOpts *opts, Error **errp)
 {
     error_setg(errp, "unix sockets are not available on windows");
     errno = ENOTSUP;
     return -1;
 }
 
-int unix_connect_opts(QemuOpts *opts, Error **errp,
-                      NonBlockingConnectHandler *callback, void *opaque)
+static int unix_connect_opts(QemuOpts *opts, Error **errp,
+                             NonBlockingConnectHandler *callback, void *opaque)
 {
     error_setg(errp, "unix sockets are not available on windows");
     errno = ENOTSUP;
-- 
2.5.0

[Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen

[Daniel P. Berrange]

The socket_listen method accepts a QAPI SocketAddress object
which it then turns into QemuOpts before calling the
inet_listen_opts/unix_listen_opts helper methods. By
converting the latter to use QAPI SocketAddress directly,
the QemuOpts conversion step can be eliminated

This also fixes the problem where ipv4=off && ipv6=off
would be treated the same as ipv4=on && ipv6=on

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 144 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 87 insertions(+), 57 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 324d3af..d14171a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -114,36 +114,68 @@ NetworkAddressFamily inet_netfamily(int family)
     return NETWORK_ADDRESS_FAMILY_UNKNOWN;
 }
 
-static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
+/*
+ * Matrix we're trying to apply
+ *
+ *  ipv4  ipv6   family
+ *   -     -       PF_UNSPEC
+ *   -     f       PF_INET
+ *   -     t       PF_INET6
+ *   f     -       PF_INET6
+ *   f     f       <error>
+ *   f     t       PF_INET6
+ *   t     -       PF_INET
+ *   t     f       PF_INET
+ *   t     t       PF_INET6
+ */
+static int inet_ai_family_from_address(InetSocketAddress *addr,
+                                       Error **errp)
+{
+    if (addr->has_ipv6 && addr->has_ipv4 &&
+        !addr->ipv6 && !addr->ipv4) {
+        error_setg(errp, "Cannot disable IPv4 and IPv6 at same time");
+        return PF_UNSPEC;
+    }
+    if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
+        return PF_INET6;
+    }
+    if ((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6)) {
+        return PF_INET;
+    }
+    return PF_UNSPEC;
+}
+
+static int inet_listen_saddr(InetSocketAddress *saddr,
+                             int port_offset,
+                             bool update_addr,
+                             Error **errp)
 {
     struct addrinfo ai,*res,*e;
-    const char *addr;
     char port[33];
     char uaddr[INET6_ADDRSTRLEN+1];
     char uport[33];
-    int slisten, rc, to, port_min, port_max, p;
+    int slisten, rc, port_min, port_max, p;
+    Error *err = NULL;
 
     memset(&ai,0, sizeof(ai));
     ai.ai_flags = AI_PASSIVE;
-    ai.ai_family = PF_UNSPEC;
+    ai.ai_family = inet_ai_family_from_address(saddr, &err);
     ai.ai_socktype = SOCK_STREAM;
 
-    if ((qemu_opt_get(opts, "host") == NULL)) {
+    if (err) {
+        error_propagate(errp, err);
+        return -1;
+    }
+
+    if (saddr->host == NULL) {
         error_setg(errp, "host not specified");
         return -1;
     }
-    if (qemu_opt_get(opts, "port") != NULL) {
-        pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port"));
+    if (saddr->port != NULL) {
+        pstrcpy(port, sizeof(port), saddr->port);
     } else {
         port[0] = '\0';
     }
-    addr = qemu_opt_get(opts, "host");
-
-    to = qemu_opt_get_number(opts, "to", 0);
-    if (qemu_opt_get_bool(opts, "ipv4", 0))
-        ai.ai_family = PF_INET;
-    if (qemu_opt_get_bool(opts, "ipv6", 0))
-        ai.ai_family = PF_INET6;
 
     /* lookup */
     if (port_offset) {
@@ -163,11 +195,11 @@ static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
         }
         snprintf(port, sizeof(port), "%d", (int)baseport + port_offset);
     }
-    rc = getaddrinfo(strlen(addr) ? addr : NULL,
+    rc = getaddrinfo(strlen(saddr->host) ? saddr->host : NULL,
                      strlen(port) ? port : NULL, &ai, &res);
     if (rc != 0) {
-        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
-                   gai_strerror(rc));
+        error_setg(errp, "address resolution failed for %s:%s: %s",
+                   saddr->host, port, gai_strerror(rc));
         return -1;
     }
 
@@ -195,7 +227,7 @@ static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
 #endif
 
         port_min = inet_getport(e);
-        port_max = to ? to + port_offset : port_min;
+        port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
             inet_setport(e, p);
             if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) {
@@ -219,13 +251,15 @@ listen:
         freeaddrinfo(res);
         return -1;
     }
-    qemu_opt_set(opts, "host", uaddr, &error_abort);
-    qemu_opt_set_number(opts, "port", inet_getport(e) - port_offset,
-                        &error_abort);
-    qemu_opt_set_bool(opts, "ipv6", e->ai_family == PF_INET6,
-                      &error_abort);
-    qemu_opt_set_bool(opts, "ipv4", e->ai_family != PF_INET6,
-                      &error_abort);
+    if (update_addr) {
+        g_free(saddr->host);
+        saddr->host = g_strdup(uaddr);
+        g_free(saddr->port);
+        saddr->port = g_strdup_printf("%d",
+                                      inet_getport(e) - port_offset);
+        saddr->has_ipv6 = saddr->ipv6 = e->ai_family == PF_INET6;
+        saddr->has_ipv4 = saddr->ipv4 = e->ai_family != PF_INET6;
+    }
     freeaddrinfo(res);
     return slisten;
 }
@@ -617,32 +651,28 @@ static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp)
 {
-    QemuOpts *opts;
     char *optstr;
     int sock = -1;
     InetSocketAddress *addr;
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
-        inet_addr_to_opts(opts, addr);
-        qapi_free_InetSocketAddress(addr);
-        sock = inet_listen_opts(opts, port_offset, errp);
+        sock = inet_listen_saddr(addr, port_offset, true, errp);
         if (sock != -1 && ostr) {
             optstr = strchr(str, ',');
-            if (qemu_opt_get_bool(opts, "ipv6", 0)) {
+            if (addr->ipv6) {
                 snprintf(ostr, olen, "[%s]:%s%s",
-                         qemu_opt_get(opts, "host"),
-                         qemu_opt_get(opts, "port"),
+                         addr->host,
+                         addr->port,
                          optstr ? optstr : "");
             } else {
                 snprintf(ostr, olen, "%s:%s%s",
-                         qemu_opt_get(opts, "host"),
-                         qemu_opt_get(opts, "port"),
+                         addr->host,
+                         addr->port,
                          optstr ? optstr : "");
             }
         }
-        qemu_opts_del(opts);
+        qapi_free_InetSocketAddress(addr);
     }
     return sock;
 }
@@ -708,10 +738,11 @@ int inet_nonblocking_connect(const char *str,
 
 #ifndef _WIN32
 
-static int unix_listen_opts(QemuOpts *opts, Error **errp)
+static int unix_listen_saddr(UnixSocketAddress *saddr,
+                             bool update_addr,
+                             Error **errp)
 {
     struct sockaddr_un un;
-    const char *path = qemu_opt_get(opts, "path");
     int sock, fd;
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
@@ -722,8 +753,8 @@ static int unix_listen_opts(QemuOpts *opts, Error **errp)
 
     memset(&un, 0, sizeof(un));
     un.sun_family = AF_UNIX;
-    if (path && strlen(path)) {
-        snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
+    if (saddr->path && strlen(saddr->path)) {
+        snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path);
     } else {
         const char *tmpdir = getenv("TMPDIR");
         tmpdir = tmpdir ? tmpdir : "/tmp";
@@ -748,7 +779,10 @@ static int unix_listen_opts(QemuOpts *opts, Error **errp)
             goto err;
         }
         close(fd);
-        qemu_opt_set(opts, "path", un.sun_path, &error_abort);
+        if (update_addr) {
+            g_free(saddr->path);
+            saddr->path = g_strdup(un.sun_path);
+        }
     }
 
     if (unlink(un.sun_path) < 0 && errno != ENOENT) {
@@ -832,7 +866,9 @@ static int unix_connect_opts(QemuOpts *opts, Error **errp,
 
 #else
 
-static int unix_listen_opts(QemuOpts *opts, Error **errp)
+static int unix_listen_saddr(UnixSocketAddress *saddr,
+                             bool update_addr,
+                             Error **errp)
 {
     error_setg(errp, "unix sockets are not available on windows");
     errno = ENOTSUP;
@@ -851,11 +887,11 @@ static int unix_connect_opts(QemuOpts *opts, Error **errp,
 /* compatibility wrapper */
 int unix_listen(const char *str, char *ostr, int olen, Error **errp)
 {
-    QemuOpts *opts;
     char *path, *optstr;
     int sock, len;
+    UnixSocketAddress *saddr;
 
-    opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
+    saddr = g_new0(UnixSocketAddress, 1);
 
     optstr = strchr(str, ',');
     if (optstr) {
@@ -863,18 +899,17 @@ int unix_listen(const char *str, char *ostr, int olen, Error **errp)
         if (len) {
             path = g_malloc(len+1);
             snprintf(path, len+1, "%.*s", len, str);
-            qemu_opt_set(opts, "path", path, &error_abort);
-            g_free(path);
+            saddr->path = path;
         }
     } else {
-        qemu_opt_set(opts, "path", str, &error_abort);
+        saddr->path = g_strdup(str);
     }
 
-    sock = unix_listen_opts(opts, errp);
+    sock = unix_listen_saddr(saddr, true, errp);
 
     if (sock != -1 && ostr)
-        snprintf(ostr, olen, "%s%s", qemu_opt_get(opts, "path"), optstr ? optstr : "");
-    qemu_opts_del(opts);
+        snprintf(ostr, olen, "%s%s", saddr->path, optstr ? optstr : "");
+    qapi_free_UnixSocketAddress(saddr);
     return sock;
 }
 
@@ -979,19 +1014,15 @@ int socket_connect(SocketAddress *addr, Error **errp,
 
 int socket_listen(SocketAddress *addr, Error **errp)
 {
-    QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
     switch (addr->type) {
     case SOCKET_ADDRESS_KIND_INET:
-        inet_addr_to_opts(opts, addr->u.inet);
-        fd = inet_listen_opts(opts, 0, errp);
+        fd = inet_listen_saddr(addr->u.inet, 0, false, errp);
         break;
 
     case SOCKET_ADDRESS_KIND_UNIX:
-        qemu_opt_set(opts, "path", addr->u.q_unix->path, &error_abort);
-        fd = unix_listen_opts(opts, errp);
+        fd = unix_listen_saddr(addr->u.q_unix, false, errp);
         break;
 
     case SOCKET_ADDRESS_KIND_FD:
@@ -1001,7 +1032,6 @@ int socket_listen(SocketAddress *addr, Error **errp)
     default:
         abort();
     }
-    qemu_opts_del(opts);
     return fd;
 }
 
-- 
2.5.0

[Qemu-devel] [PATCH v2 3/5] sockets: remove use of QemuOpts from socket_connect

[Daniel P. Berrange]

The socket_connect method accepts a QAPI SocketAddress object
which it then turns into QemuOpts before calling the
inet_connect_opts/unix_connect_opts helper methods. By
converting the latter to use QAPI SocketAddress directly,
the QemuOpts conversion step can be eliminated

This also fixes the problem where ipv4=off && ipv6=off
would be treated the same as ipv4=on && ipv6=on

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 91 +++++++++++++++++++++--------------------------------
 1 file changed, 36 insertions(+), 55 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d14171a..679f27a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -374,38 +374,34 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
     return sock;
 }
 
-static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
+static struct addrinfo *inet_parse_connect_saddr(InetSocketAddress *saddr,
+                                                 Error **errp)
 {
     struct addrinfo ai, *res;
     int rc;
-    const char *addr;
-    const char *port;
+    Error *err = NULL;
 
     memset(&ai, 0, sizeof(ai));
 
     ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
-    ai.ai_family = PF_UNSPEC;
+    ai.ai_family = inet_ai_family_from_address(saddr, &err);
     ai.ai_socktype = SOCK_STREAM;
 
-    addr = qemu_opt_get(opts, "host");
-    port = qemu_opt_get(opts, "port");
-    if (addr == NULL || port == NULL) {
-        error_setg(errp, "host and/or port not specified");
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
 
-    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
-        ai.ai_family = PF_INET;
-    }
-    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
-        ai.ai_family = PF_INET6;
+    if (saddr->host == NULL || saddr->port == NULL) {
+        error_setg(errp, "host and/or port not specified");
+        return NULL;
     }
 
     /* lookup */
-    rc = getaddrinfo(addr, port, &ai, &res);
+    rc = getaddrinfo(saddr->host, saddr->port, &ai, &res);
     if (rc != 0) {
-        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
-                   gai_strerror(rc));
+        error_setg(errp, "address resolution failed for %s:%s: %s",
+                   saddr->host, saddr->port, gai_strerror(rc));
         return NULL;
     }
     return res;
@@ -414,8 +410,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
 /**
  * Create a socket and connect it to an address.
  *
- * @opts: QEMU options, recognized parameters strings "host" and "port",
- *        bools "ipv4" and "ipv6".
+ * @saddr: Inet socket address specification
  * @errp: set on error
  * @callback: callback function for non-blocking connect
  * @opaque: opaque for callback function
@@ -426,8 +421,8 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
  * function succeeds, callback will be called when the connection
  * completes, with the file descriptor on success, or -1 on error.
  */
-static int inet_connect_opts(QemuOpts *opts, Error **errp,
-                             NonBlockingConnectHandler *callback, void *opaque)
+static int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+                              NonBlockingConnectHandler *callback, void *opaque)
 {
     Error *local_err = NULL;
     struct addrinfo *res, *e;
@@ -435,7 +430,7 @@ static int inet_connect_opts(QemuOpts *opts, Error **errp,
     bool in_progress;
     ConnectState *connect_state = NULL;
 
-    res = inet_parse_connect_opts(opts, errp);
+    res = inet_parse_connect_saddr(saddr, errp);
     if (!res) {
         return -1;
     }
@@ -687,17 +682,13 @@ int inet_listen(const char *str, char *ostr, int olen,
  **/
 int inet_connect(const char *str, Error **errp)
 {
-    QemuOpts *opts;
     int sock = -1;
     InetSocketAddress *addr;
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
-        inet_addr_to_opts(opts, addr);
+        sock = inet_connect_saddr(addr, errp, NULL, NULL);
         qapi_free_InetSocketAddress(addr);
-        sock = inet_connect_opts(opts, errp, NULL, NULL);
-        qemu_opts_del(opts);
     }
     return sock;
 }
@@ -719,7 +710,6 @@ int inet_nonblocking_connect(const char *str,
                              NonBlockingConnectHandler *callback,
                              void *opaque, Error **errp)
 {
-    QemuOpts *opts;
     int sock = -1;
     InetSocketAddress *addr;
 
@@ -727,11 +717,8 @@ int inet_nonblocking_connect(const char *str,
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
-        inet_addr_to_opts(opts, addr);
+        sock = inet_connect_saddr(addr, errp, callback, opaque);
         qapi_free_InetSocketAddress(addr);
-        sock = inet_connect_opts(opts, errp, callback, opaque);
-        qemu_opts_del(opts);
     }
     return sock;
 }
@@ -806,15 +793,14 @@ err:
     return -1;
 }
 
-static int unix_connect_opts(QemuOpts *opts, Error **errp,
-                             NonBlockingConnectHandler *callback, void *opaque)
+static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
+                              NonBlockingConnectHandler *callback, void *opaque)
 {
     struct sockaddr_un un;
-    const char *path = qemu_opt_get(opts, "path");
     ConnectState *connect_state = NULL;
     int sock, rc;
 
-    if (path == NULL) {
+    if (saddr->path == NULL) {
         error_setg(errp, "unix connect: no path specified");
         return -1;
     }
@@ -833,7 +819,7 @@ static int unix_connect_opts(QemuOpts *opts, Error **errp,
 
     memset(&un, 0, sizeof(un));
     un.sun_family = AF_UNIX;
-    snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
+    snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path);
 
     /* connect to peer */
     do {
@@ -875,8 +861,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     return -1;
 }
 
-static int unix_connect_opts(QemuOpts *opts, Error **errp,
-                             NonBlockingConnectHandler *callback, void *opaque)
+static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
+                              NonBlockingConnectHandler *callback, void *opaque)
 {
     error_setg(errp, "unix sockets are not available on windows");
     errno = ENOTSUP;
@@ -915,13 +901,13 @@ int unix_listen(const char *str, char *ostr, int olen, Error **errp)
 
 int unix_connect(const char *path, Error **errp)
 {
-    QemuOpts *opts;
+    UnixSocketAddress *saddr;
     int sock;
 
-    opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
-    qemu_opt_set(opts, "path", path, &error_abort);
-    sock = unix_connect_opts(opts, errp, NULL, NULL);
-    qemu_opts_del(opts);
+    saddr = g_new0(UnixSocketAddress, 1);
+    saddr->path = g_strdup(path);
+    sock = unix_connect_saddr(saddr, errp, NULL, NULL);
+    qapi_free_UnixSocketAddress(saddr);
     return sock;
 }
 
@@ -930,15 +916,15 @@ int unix_nonblocking_connect(const char *path,
                              NonBlockingConnectHandler *callback,
                              void *opaque, Error **errp)
 {
-    QemuOpts *opts;
+    UnixSocketAddress *saddr;
     int sock = -1;
 
     g_assert(callback != NULL);
 
-    opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
-    qemu_opt_set(opts, "path", path, &error_abort);
-    sock = unix_connect_opts(opts, errp, callback, opaque);
-    qemu_opts_del(opts);
+    saddr = g_new0(UnixSocketAddress, 1);
+    saddr->path = g_strdup(path);
+    sock = unix_connect_saddr(saddr, errp, callback, opaque);
+    qapi_free_UnixSocketAddress(saddr);
     return sock;
 }
 
@@ -982,19 +968,15 @@ fail:
 int socket_connect(SocketAddress *addr, Error **errp,
                    NonBlockingConnectHandler *callback, void *opaque)
 {
-    QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
     switch (addr->type) {
     case SOCKET_ADDRESS_KIND_INET:
-        inet_addr_to_opts(opts, addr->u.inet);
-        fd = inet_connect_opts(opts, errp, callback, opaque);
+        fd = inet_connect_saddr(addr->u.inet, errp, callback, opaque);
         break;
 
     case SOCKET_ADDRESS_KIND_UNIX:
-        qemu_opt_set(opts, "path", addr->u.q_unix->path, &error_abort);
-        fd = unix_connect_opts(opts, errp, callback, opaque);
+        fd = unix_connect_saddr(addr->u.q_unix, errp, callback, opaque);
         break;
 
     case SOCKET_ADDRESS_KIND_FD:
@@ -1008,7 +990,6 @@ int socket_connect(SocketAddress *addr, Error **errp,
     default:
         abort();
     }
-    qemu_opts_del(opts);
     return fd;
 }
 
-- 
2.5.0

[Qemu-devel] [PATCH v2 4/5] sockets: remove use of QemuOpts from socket_dgram

[Daniel P. Berrange]

The socket_dgram method accepts a QAPI SocketAddress object
which it then turns into QemuOpts before calling the
inet_dgram_opts helper method. By converting the latter to
use QAPI SocketAddress directly, the QemuOpts conversion
step can be eliminated.

This also fixes the problem where ipv4=off && ipv6=off
would be treated the same as ipv4=on && ipv6=on

This removes the very last use of QemuOpts from the
sockets code, so the socket_optslist[] array is also
removed.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qemu/sockets.h |  1 -
 util/qemu-sockets.c    | 99 ++++++++++++--------------------------------------
 2 files changed, 24 insertions(+), 76 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 2741b97..bf7154c 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -26,7 +26,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
 
 #endif /* !_WIN32 */
 
-#include "qemu/option.h"
 #include "qapi/error.h"
 #include "qapi-types.h"
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 679f27a..aeae3b1 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -36,39 +36,6 @@
 # define AI_V4MAPPED 0
 #endif
 
-/* used temporarily until all users are converted to QemuOpts */
-static QemuOptsList socket_optslist = {
-    .name = "socket",
-    .head = QTAILQ_HEAD_INITIALIZER(socket_optslist.head),
-    .desc = {
-        {
-            .name = "path",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "host",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "port",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "localaddr",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "localport",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "to",
-            .type = QEMU_OPT_NUMBER,
-        },{
-            .name = "ipv4",
-            .type = QEMU_OPT_BOOL,
-        },{
-            .name = "ipv6",
-            .type = QEMU_OPT_BOOL,
-        },
-        { /* end if list */ }
-    },
-};
 
 static int inet_getport(struct addrinfo *e)
 {
@@ -469,21 +436,29 @@ static int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
     return sock;
 }
 
-static int inet_dgram_opts(QemuOpts *opts, Error **errp)
+static int inet_dgram_saddr(InetSocketAddress *sraddr,
+                            InetSocketAddress *sladdr,
+                            Error **errp)
 {
     struct addrinfo ai, *peer = NULL, *local = NULL;
     const char *addr;
     const char *port;
     int sock = -1, rc;
+    Error *err = NULL;
 
     /* lookup peer addr */
     memset(&ai,0, sizeof(ai));
     ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
-    ai.ai_family = PF_UNSPEC;
+    ai.ai_family = inet_ai_family_from_address(sraddr, &err);
     ai.ai_socktype = SOCK_DGRAM;
 
-    addr = qemu_opt_get(opts, "host");
-    port = qemu_opt_get(opts, "port");
+    if (err) {
+        error_propagate(errp, err);
+        return -1;
+    }
+
+    addr = sraddr->host;
+    port = sraddr->port;
     if (addr == NULL || strlen(addr) == 0) {
         addr = "localhost";
     }
@@ -492,11 +467,6 @@ static int inet_dgram_opts(QemuOpts *opts, Error **errp)
         return -1;
     }
 
-    if (qemu_opt_get_bool(opts, "ipv4", 0))
-        ai.ai_family = PF_INET;
-    if (qemu_opt_get_bool(opts, "ipv6", 0))
-        ai.ai_family = PF_INET6;
-
     if (0 != (rc = getaddrinfo(addr, port, &ai, &peer))) {
         error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
                    gai_strerror(rc));
@@ -509,13 +479,19 @@ static int inet_dgram_opts(QemuOpts *opts, Error **errp)
     ai.ai_family = peer->ai_family;
     ai.ai_socktype = SOCK_DGRAM;
 
-    addr = qemu_opt_get(opts, "localaddr");
-    port = qemu_opt_get(opts, "localport");
-    if (addr == NULL || strlen(addr) == 0) {
+    if (sladdr) {
+        addr = sladdr->host;
+        port = sladdr->port;
+        if (addr == NULL || strlen(addr) == 0) {
+            addr = NULL;
+        }
+        if (!port || strlen(port) == 0) {
+            port = "0";
+        }
+    } else {
         addr = NULL;
-    }
-    if (!port || strlen(port) == 0)
         port = "0";
+    }
 
     if (0 != (rc = getaddrinfo(addr, port, &ai, &local))) {
         error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
@@ -624,25 +600,6 @@ fail:
     return NULL;
 }
 
-static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
-{
-    bool ipv4 = addr->has_ipv4 && addr->ipv4;
-    bool ipv6 = addr->has_ipv6 && addr->ipv6;
-
-    if (ipv4 || ipv6) {
-        qemu_opt_set_bool(opts, "ipv4", ipv4, &error_abort);
-        qemu_opt_set_bool(opts, "ipv6", ipv6, &error_abort);
-    } else if (addr->has_ipv4 || addr->has_ipv6) {
-        qemu_opt_set_bool(opts, "ipv4", !addr->has_ipv4, &error_abort);
-        qemu_opt_set_bool(opts, "ipv6", !addr->has_ipv6, &error_abort);
-    }
-    if (addr->has_to) {
-        qemu_opt_set_number(opts, "to", addr->to, &error_abort);
-    }
-    qemu_opt_set(opts, "host", addr->host, &error_abort);
-    qemu_opt_set(opts, "port", addr->port, &error_abort);
-}
-
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp)
 {
@@ -1018,25 +975,17 @@ int socket_listen(SocketAddress *addr, Error **errp)
 
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
 {
-    QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
     switch (remote->type) {
     case SOCKET_ADDRESS_KIND_INET:
-        inet_addr_to_opts(opts, remote->u.inet);
-        if (local) {
-            qemu_opt_set(opts, "localaddr", local->u.inet->host, &error_abort);
-            qemu_opt_set(opts, "localport", local->u.inet->port, &error_abort);
-        }
-        fd = inet_dgram_opts(opts, errp);
+        fd = inet_dgram_saddr(remote->u.inet, local ? local->u.inet : NULL, errp);
         break;
 
     default:
         error_setg(errp, "socket type unsupported for datagram");
         fd = -1;
     }
-    qemu_opts_del(opts);
     return fd;
 }
 
-- 
2.5.0

[Qemu-devel] [PATCH v2 5/5] vnc: distiguish between ipv4/ipv6 omitted vs set to off

[Daniel P. Berrange]

The VNC code for interpreting QemuOpts does not currently
distinguish between ipv4/ipv6 being omitted, and being
set to 'off', because historically the 'ipv4' and 'ipv6'
options were just flags which did not accept a value.

The upshot is that if someone runs

  $QEMU -vnc localhost:1,ipv6=off

QEMU still uses PF_UNSPEC and thus may still bind to IPv6,
when it should use PF_INET.

This is another instance of the problem previously fixed
for chardevs in

  commit b77e7c8e99f9ac726c4eaa2fc3461fd886017dc0
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   Mon Oct 12 15:35:16 2015 +0200

    qemu-sockets: fix conversion of ipv4/ipv6 JSON to QemuOpts

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index c9f2fed..4dc7684 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3570,8 +3570,10 @@ void vnc_display_open(const char *id, Error **errp)
 
         const char *websocket = qemu_opt_get(opts, "websocket");
         int to = qemu_opt_get_number(opts, "to", 0);
-        bool has_ipv4 = qemu_opt_get_bool(opts, "ipv4", false);
-        bool has_ipv6 = qemu_opt_get_bool(opts, "ipv6", false);
+        bool has_ipv4 = qemu_opt_get(opts, "ipv4");
+        bool has_ipv6 = qemu_opt_get(opts, "ipv6");
+        bool ipv4 = qemu_opt_get_bool(opts, "ipv4", false);
+        bool ipv6 = qemu_opt_get_bool(opts, "ipv6", false);
 
         saddr = g_new0(SocketAddress, 1);
         if (websocket) {
@@ -3619,8 +3621,10 @@ void vnc_display_open(const char *id, Error **errp)
                 saddr->u.inet->has_to = true;
                 saddr->u.inet->to = to + 5900;
             }
-            saddr->u.inet->ipv4 = saddr->u.inet->has_ipv4 = has_ipv4;
-            saddr->u.inet->ipv6 = saddr->u.inet->has_ipv6 = has_ipv6;
+            saddr->u.inet->ipv4 = ipv4;
+            saddr->u.inet->has_ipv4 = has_ipv4;
+            saddr->u.inet->ipv6 = ipv6;
+            saddr->u.inet->has_ipv6 = has_ipv6;
 
             if (vs->ws_enabled) {
                 wsaddr->type = SOCKET_ADDRESS_KIND_INET;
@@ -3632,8 +3636,10 @@ void vnc_display_open(const char *id, Error **errp)
                     wsaddr->u.inet->has_to = true;
                     wsaddr->u.inet->to = to;
                 }
-                wsaddr->u.inet->ipv4 = wsaddr->u.inet->has_ipv4 = has_ipv4;
-                wsaddr->u.inet->ipv6 = wsaddr->u.inet->has_ipv6 = has_ipv6;
+                wsaddr->u.inet->ipv4 = ipv4;
+                wsaddr->u.inet->has_ipv4 = has_ipv4;
+                wsaddr->u.inet->ipv6 = ipv6;
+                wsaddr->u.inet->has_ipv6 = has_ipv6;
             }
         }
     } else {
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen

[Eric Blake]

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

On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> The socket_listen method accepts a QAPI SocketAddress object
> which it then turns into QemuOpts before calling the
> inet_listen_opts/unix_listen_opts helper methods. By
> converting the latter to use QAPI SocketAddress directly,
> the QemuOpts conversion step can be eliminated
> 
> This also fixes the problem where ipv4=off && ipv6=off
> would be treated the same as ipv4=on && ipv6=on
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 144 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 87 insertions(+), 57 deletions(-)
> 

> +++ b/util/qemu-sockets.c
> @@ -114,36 +114,68 @@ NetworkAddressFamily inet_netfamily(int family)
>      return NETWORK_ADDRESS_FAMILY_UNKNOWN;
>  }
>  
> -static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
> +/*
> + * Matrix we're trying to apply
> + *
> + *  ipv4  ipv6   family
> + *   -     -       PF_UNSPEC
> + *   -     f       PF_INET
> + *   -     t       PF_INET6
> + *   f     -       PF_INET6
> + *   f     f       <error>
> + *   f     t       PF_INET6
> + *   t     -       PF_INET
> + *   t     f       PF_INET

These I understand,

> + *   t     t       PF_INET6

but why is this one PF_INET6 instead of PF_UNSPEC?

> + */
> +static int inet_ai_family_from_address(InetSocketAddress *addr,
> +                                       Error **errp)
> +{
> +    if (addr->has_ipv6 && addr->has_ipv4 &&
> +        !addr->ipv6 && !addr->ipv4) {
> +        error_setg(errp, "Cannot disable IPv4 and IPv6 at same time");
> +        return PF_UNSPEC;
> +    }
> +    if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
> +        return PF_INET6;
> +    }
> +    if ((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6)) {
> +        return PF_INET;
> +    }
> +    return PF_UNSPEC;

This logic matches the matrix as listed, even if I'm not positive that
the matrix is correct.  If we want PF_UNSPEC when both v4 and v6 are
explicitly requested (as in, pick whichever works), then I think it
should be something like:

if (addr->has_ipv6 && addr->has_ipv4 &&
    addr->ipv6 == addr->ipv4) {
    if (!addr->ipv6) {
        error_setg(errp, "cannot disable IPv4 and IPv6 at the hsame time");
    }
    return PF_UNSPEC;
}
if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
    return PF_INET6;
}
assert((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6));
return PF_INET;

> @@ -219,13 +251,15 @@ listen:
>          freeaddrinfo(res);
>          return -1;
>      }
> -    qemu_opt_set(opts, "host", uaddr, &error_abort);
> -    qemu_opt_set_number(opts, "port", inet_getport(e) - port_offset,
> -                        &error_abort);
> -    qemu_opt_set_bool(opts, "ipv6", e->ai_family == PF_INET6,
> -                      &error_abort);
> -    qemu_opt_set_bool(opts, "ipv4", e->ai_family != PF_INET6,
> -                      &error_abort);
> +    if (update_addr) {
> +        g_free(saddr->host);
> +        saddr->host = g_strdup(uaddr);
> +        g_free(saddr->port);
> +        saddr->port = g_strdup_printf("%d",
> +                                      inet_getport(e) - port_offset);
> +        saddr->has_ipv6 = saddr->ipv6 = e->ai_family == PF_INET6;
> +        saddr->has_ipv4 = saddr->ipv4 = e->ai_family != PF_INET6;

Should we handle PF_UNSPEC specifically, maybe by having the has_ipv6
assignment based on e->ai_family != PF_INET?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 3/5] sockets: remove use of QemuOpts from socket_connect

[Eric Blake]

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

On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> The socket_connect method accepts a QAPI SocketAddress object
> which it then turns into QemuOpts before calling the
> inet_connect_opts/unix_connect_opts helper methods. By
> converting the latter to use QAPI SocketAddress directly,
> the QemuOpts conversion step can be eliminated
> 
> This also fixes the problem where ipv4=off && ipv6=off
> would be treated the same as ipv4=on && ipv6=on
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 91 +++++++++++++++++++++--------------------------------
>  1 file changed, 36 insertions(+), 55 deletions(-)
> 

>      ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
> -    ai.ai_family = PF_UNSPEC;
> +    ai.ai_family = inet_ai_family_from_address(saddr, &err);
>      ai.ai_socktype = SOCK_STREAM;
>  

>  
> -    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
> -        ai.ai_family = PF_INET;
> -    }
> -    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
> -        ai.ai_family = PF_INET6;

I'm using the notation you used in 2/5, where - is unspecified, f is
explicitly false, and t is explicitly true.  qemu_opt_get_bool(, 0)
cannot tell the difference between - and f.

The old code treated 4=- and 6=- as PF_UNSPEC; 4=f and 6=f as PF_UNSPEC,
and 4=t and 6=t as PF_INET6.  That doesn't quite jive with the commit
message claiming that 4=off (is that '4=-' or '4=f'?) and 6=off was the
same as 4=on (4=t) and 6=on.

However, I definitely agree with using inet_ai_family_from_address()
rather than the old code.  The code looks correct, but I want to make
sure the commit message matches before giving R-b.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 4/5] sockets: remove use of QemuOpts from socket_dgram

[Eric Blake]

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

On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> The socket_dgram method accepts a QAPI SocketAddress object
> which it then turns into QemuOpts before calling the
> inet_dgram_opts helper method. By converting the latter to
> use QAPI SocketAddress directly, the QemuOpts conversion
> step can be eliminated.
> 
> This also fixes the problem where ipv4=off && ipv6=off
> would be treated the same as ipv4=on && ipv6=on
> 
> This removes the very last use of QemuOpts from the
> sockets code, so the socket_optslist[] array is also
> removed.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qemu/sockets.h |  1 -
>  util/qemu-sockets.c    | 99 ++++++++++++--------------------------------------
>  2 files changed, 24 insertions(+), 76 deletions(-)
> 

I might have split the cleanup into a separate patch, but can understand
if the compiler was warning about unused static items.  Similar
questions about the accuracy of the commit message as in the earlier
patches, but the code cleanup looked sane to me.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 5/5] vnc: distiguish between ipv4/ipv6 omitted vs set to off

[Eric Blake]

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

On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> The VNC code for interpreting QemuOpts does not currently
> distinguish between ipv4/ipv6 being omitted, and being
> set to 'off', because historically the 'ipv4' and 'ipv6'
> options were just flags which did not accept a value.
> 
> The upshot is that if someone runs
> 
>   $QEMU -vnc localhost:1,ipv6=off
> 
> QEMU still uses PF_UNSPEC and thus may still bind to IPv6,
> when it should use PF_INET.
> 
> This is another instance of the problem previously fixed
> for chardevs in
> 
>   commit b77e7c8e99f9ac726c4eaa2fc3461fd886017dc0
>   Author: Paolo Bonzini <pbonzini@redhat.com>
>   Date:   Mon Oct 12 15:35:16 2015 +0200
> 
>     qemu-sockets: fix conversion of ipv4/ipv6 JSON to QemuOpts
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  ui/vnc.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen

[Daniel P. Berrange]

On Tue, Nov 17, 2015 at 03:22:04PM -0700, Eric Blake wrote:
> On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> > The socket_listen method accepts a QAPI SocketAddress object
> > which it then turns into QemuOpts before calling the
> > inet_listen_opts/unix_listen_opts helper methods. By
> > converting the latter to use QAPI SocketAddress directly,
> > the QemuOpts conversion step can be eliminated
> > 
> > This also fixes the problem where ipv4=off && ipv6=off
> > would be treated the same as ipv4=on && ipv6=on
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  util/qemu-sockets.c | 144 +++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 87 insertions(+), 57 deletions(-)
> > 
> 
> > +++ b/util/qemu-sockets.c
> > @@ -114,36 +114,68 @@ NetworkAddressFamily inet_netfamily(int family)
> >      return NETWORK_ADDRESS_FAMILY_UNKNOWN;
> >  }
> >  
> > -static int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
> > +/*
> > + * Matrix we're trying to apply
> > + *
> > + *  ipv4  ipv6   family
> > + *   -     -       PF_UNSPEC
> > + *   -     f       PF_INET
> > + *   -     t       PF_INET6
> > + *   f     -       PF_INET6
> > + *   f     f       <error>
> > + *   f     t       PF_INET6
> > + *   t     -       PF_INET
> > + *   t     f       PF_INET
> 
> These I understand,
> 
> > + *   t     t       PF_INET6
> 
> but why is this one PF_INET6 instead of PF_UNSPEC?

If you use PF_UNSPEC, then the addresses we listen on will be automatically
deteremined by results of the DNS lookup. ie if DNS only returns an IPv4
address, it won't listen on IPv6.  When the user has said ipv6=on, then
they expect to get an error if it was not possible to listen on IPv6. So
we must use PF_INET6 here to ensure that error, otherwise ipv6=on & ipv4=on
would be no different from ipv6=- & ipv4=-.

> 
> > + */
> > +static int inet_ai_family_from_address(InetSocketAddress *addr,
> > +                                       Error **errp)
> > +{
> > +    if (addr->has_ipv6 && addr->has_ipv4 &&
> > +        !addr->ipv6 && !addr->ipv4) {
> > +        error_setg(errp, "Cannot disable IPv4 and IPv6 at same time");
> > +        return PF_UNSPEC;
> > +    }
> > +    if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
> > +        return PF_INET6;
> > +    }
> > +    if ((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6)) {
> > +        return PF_INET;
> > +    }
> > +    return PF_UNSPEC;
> 
> This logic matches the matrix as listed, even if I'm not positive that
> the matrix is correct.  If we want PF_UNSPEC when both v4 and v6 are
> explicitly requested (as in, pick whichever works), then I think it
> should be something like:
> 
> if (addr->has_ipv6 && addr->has_ipv4 &&
>     addr->ipv6 == addr->ipv4) {
>     if (!addr->ipv6) {
>         error_setg(errp, "cannot disable IPv4 and IPv6 at the hsame time");
>     }
>     return PF_UNSPEC;
> }
> if ((addr->has_ipv6 && addr->ipv6) || (addr->has_ipv4 && !addr->ipv4)) {
>     return PF_INET6;
> }
> assert((addr->has_ipv4 && addr->ipv4) || (addr->has_ipv6 && !addr->ipv6));
> return PF_INET;
> 
> > @@ -219,13 +251,15 @@ listen:
> >          freeaddrinfo(res);
> >          return -1;
> >      }
> > -    qemu_opt_set(opts, "host", uaddr, &error_abort);
> > -    qemu_opt_set_number(opts, "port", inet_getport(e) - port_offset,
> > -                        &error_abort);
> > -    qemu_opt_set_bool(opts, "ipv6", e->ai_family == PF_INET6,
> > -                      &error_abort);
> > -    qemu_opt_set_bool(opts, "ipv4", e->ai_family != PF_INET6,
> > -                      &error_abort);
> > +    if (update_addr) {
> > +        g_free(saddr->host);
> > +        saddr->host = g_strdup(uaddr);
> > +        g_free(saddr->port);
> > +        saddr->port = g_strdup_printf("%d",
> > +                                      inet_getport(e) - port_offset);
> > +        saddr->has_ipv6 = saddr->ipv6 = e->ai_family == PF_INET6;
> > +        saddr->has_ipv4 = saddr->ipv4 = e->ai_family != PF_INET6;
> 
> Should we handle PF_UNSPEC specifically, maybe by having the has_ipv6
> assignment based on e->ai_family != PF_INET?

The returne e->ai_family from getaddrinfo will never have a value
of PF_UNSPEC - that's an input only value. So we're OK to assume
we'll have PF_INET6 and PF_INET only.  Well at least until someone
invents IPv7 but I'll let my great grandchildren deal with that
problem ;-)

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/5] sockets: remove use of QemuOpts from socket_connect

[Daniel P. Berrange]

On Tue, Nov 17, 2015 at 03:40:58PM -0700, Eric Blake wrote:
> On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> > The socket_connect method accepts a QAPI SocketAddress object
> > which it then turns into QemuOpts before calling the
> > inet_connect_opts/unix_connect_opts helper methods. By
> > converting the latter to use QAPI SocketAddress directly,
> > the QemuOpts conversion step can be eliminated
> > 
> > This also fixes the problem where ipv4=off && ipv6=off
> > would be treated the same as ipv4=on && ipv6=on
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  util/qemu-sockets.c | 91 +++++++++++++++++++++--------------------------------
> >  1 file changed, 36 insertions(+), 55 deletions(-)
> > 
> 
> >      ai.ai_flags = AI_CANONNAME | AI_V4MAPPED | AI_ADDRCONFIG;
> > -    ai.ai_family = PF_UNSPEC;
> > +    ai.ai_family = inet_ai_family_from_address(saddr, &err);
> >      ai.ai_socktype = SOCK_STREAM;
> >  
> 
> >  
> > -    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
> > -        ai.ai_family = PF_INET;
> > -    }
> > -    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
> > -        ai.ai_family = PF_INET6;
> 
> I'm using the notation you used in 2/5, where - is unspecified, f is
> explicitly false, and t is explicitly true.  qemu_opt_get_bool(, 0)
> cannot tell the difference between - and f.
> 
> The old code treated 4=- and 6=- as PF_UNSPEC; 4=f and 6=f as PF_UNSPEC,
> and 4=t and 6=t as PF_INET6.  That doesn't quite jive with the commit
> message claiming that 4=off (is that '4=-' or '4=f'?) and 6=off was the
> same as 4=on (4=t) and 6=on.

Yeah that commit message is wrong - i'll remove that bit



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen

[Eric Blake]

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

On 11/18/2015 03:08 AM, Daniel P. Berrange wrote:
> On Tue, Nov 17, 2015 at 03:22:04PM -0700, Eric Blake wrote:
>> On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
>>> The socket_listen method accepts a QAPI SocketAddress object
>>> which it then turns into QemuOpts before calling the
>>> inet_listen_opts/unix_listen_opts helper methods. By
>>> converting the latter to use QAPI SocketAddress directly,
>>> the QemuOpts conversion step can be eliminated
>>>
>>> This also fixes the problem where ipv4=off && ipv6=off
>>> would be treated the same as ipv4=on && ipv6=on
>>>

>>> + *  ipv4  ipv6   family
>>> + *   -     -       PF_UNSPEC

This says I have no preference, so pick the one that works.

>>> + *   -     f       PF_INET
>>> + *   -     t       PF_INET6
>>> + *   f     -       PF_INET6
>>> + *   f     f       <error>
>>> + *   f     t       PF_INET6
>>> + *   t     -       PF_INET
>>> + *   t     f       PF_INET
>>
>> These I understand,

Generally to mean "I specifically requested this" or "I specifically
don't want that", where there is no collision.

>>
>>> + *   t     t       PF_INET6
>>
>> but why is this one PF_INET6 instead of PF_UNSPEC?
> 
> If you use PF_UNSPEC, then the addresses we listen on will be automatically
> deteremined by results of the DNS lookup. ie if DNS only returns an IPv4
> address, it won't listen on IPv6.  When the user has said ipv6=on, then
> they expect to get an error if it was not possible to listen on IPv6. So
> we must use PF_INET6 here to ensure that error, otherwise ipv6=on & ipv4=on
> would be no different from ipv6=- & ipv4=-.

But if I'm specifically requesting that both families be used, either
that should be an error (we can't honor two families at once) or it
should be allowed (use the family that makes sense), not a synonym for
ipv6-only.

>>> @@ -219,13 +251,15 @@ listen:
>>>          freeaddrinfo(res);
>>>          return -1;
>>>      }
>>> -    qemu_opt_set(opts, "host", uaddr, &error_abort);
>>> -    qemu_opt_set_number(opts, "port", inet_getport(e) - port_offset,
>>> -                        &error_abort);
>>> -    qemu_opt_set_bool(opts, "ipv6", e->ai_family == PF_INET6,
>>> -                      &error_abort);
>>> -    qemu_opt_set_bool(opts, "ipv4", e->ai_family != PF_INET6,
>>> -                      &error_abort);
>>> +    if (update_addr) {
>>> +        g_free(saddr->host);
>>> +        saddr->host = g_strdup(uaddr);
>>> +        g_free(saddr->port);
>>> +        saddr->port = g_strdup_printf("%d",
>>> +                                      inet_getport(e) - port_offset);
>>> +        saddr->has_ipv6 = saddr->ipv6 = e->ai_family == PF_INET6;
>>> +        saddr->has_ipv4 = saddr->ipv4 = e->ai_family != PF_INET6;
>>
>> Should we handle PF_UNSPEC specifically, maybe by having the has_ipv6
>> assignment based on e->ai_family != PF_INET?
> 
> The returne e->ai_family from getaddrinfo will never have a value
> of PF_UNSPEC - that's an input only value. So we're OK to assume
> we'll have PF_INET6 and PF_INET only.  Well at least until someone
> invents IPv7 but I'll let my great grandchildren deal with that
> problem ;-)

Okay, but maybe worth a comment somewhere.  Or put another way, on
input, we can request one or both families, on output, we want to
guarantee which family was selected.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/5] sockets: remove use of QemuOpts from socket_listen

[Daniel P. Berrange]

On Wed, Nov 18, 2015 at 08:44:28AM -0700, Eric Blake wrote:
> On 11/18/2015 03:08 AM, Daniel P. Berrange wrote:
> > On Tue, Nov 17, 2015 at 03:22:04PM -0700, Eric Blake wrote:
> >> On 11/17/2015 10:00 AM, Daniel P. Berrange wrote:
> >>> The socket_listen method accepts a QAPI SocketAddress object
> >>> which it then turns into QemuOpts before calling the
> >>> inet_listen_opts/unix_listen_opts helper methods. By
> >>> converting the latter to use QAPI SocketAddress directly,
> >>> the QemuOpts conversion step can be eliminated
> >>>
> >>> This also fixes the problem where ipv4=off && ipv6=off
> >>> would be treated the same as ipv4=on && ipv6=on
> >>>
> 
> >>> + *  ipv4  ipv6   family
> >>> + *   -     -       PF_UNSPEC
> 
> This says I have no preference, so pick the one that works.
> 
> >>> + *   -     f       PF_INET
> >>> + *   -     t       PF_INET6
> >>> + *   f     -       PF_INET6
> >>> + *   f     f       <error>
> >>> + *   f     t       PF_INET6
> >>> + *   t     -       PF_INET
> >>> + *   t     f       PF_INET
> >>
> >> These I understand,
> 
> Generally to mean "I specifically requested this" or "I specifically
> don't want that", where there is no collision.
> 
> >>
> >>> + *   t     t       PF_INET6
> >>
> >> but why is this one PF_INET6 instead of PF_UNSPEC?
> > 
> > If you use PF_UNSPEC, then the addresses we listen on will be automatically
> > deteremined by results of the DNS lookup. ie if DNS only returns an IPv4
> > address, it won't listen on IPv6.  When the user has said ipv6=on, then
> > they expect to get an error if it was not possible to listen on IPv6. So
> > we must use PF_INET6 here to ensure that error, otherwise ipv6=on & ipv4=on
> > would be no different from ipv6=- & ipv4=-.
> 
> But if I'm specifically requesting that both families be used, either
> that should be an error (we can't honor two families at once) or it
> should be allowed (use the family that makes sense), not a synonym for
> ipv6-only.

Yes, you are right that this code means  ipv6=t & ipv4=t is
essentially equivalent to ipv6=t & ipv4=-, but that is a
limitation of getaddrinfo().

To address this semantic flaw, when ipv6=t, then we need
better handling of the IPV6_V6ONLY flag to take account
of ipv4= setting, and then actually verify whether ipv4
really was enabled when we asked for it.  This is a
pre-existing bug that my patch is not making worse. I
will have a think about fixing it separately.

And yes, we so badly need a unit test to validate all
this logic, which I also want to look into...

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|