[Qemu-devel] [PATCH v2 0/4] qga: add vsock-listen

[Qemu-devel] [PATCH v2 0/4] qga: add vsock-listen

[Stefan Hajnoczi]

v2:
 * s/seasy/easy/ typo fix in commit description [Eric]
 * Use %n to check for trailing characters in addresses [Eric]
 * Added Mike's R-b

This patch series adds virtio-vsock support to the QEMU guest agent.

  $ qemu-system-x86_64 -device vhost-vsock-pci,guest-cid=3 ...
  (guest)# qemu-ga -m vsock-listen -p 3:1234

You can interact with the qga monitor using the nc-vsock utility:
https://raw.githubusercontent.com/stefanha/linux/dd0d6a2aa62c0fd6cdc9dbd4b3dc4bfd0828c329/nc-vsock.c

  $ nc-vsock 3 1234
  {'execute': 'guest-info'}
  ...

For more information about virtio-vsock, see
http://qemu-project.org/Features/VirtioVsock.

Stefan Hajnoczi (4):
  qga: drop unused sockaddr in accept(2) call
  qga: drop unnecessary GA_CHANNEL_UNIX_LISTEN checks
  sockets: add AF_VSOCK support
  qga: add vsock-listen method

 qapi-schema.json    |  23 +++++-
 qga/channel-posix.c |  36 +++++++--
 qga/channel.h       |   1 +
 qga/main.c          |   6 +-
 util/qemu-sockets.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 282 insertions(+), 11 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/4] qga: drop unused sockaddr in accept(2) call

[Stefan Hajnoczi]

ga_channel_listen_accept() is currently hard-coded to support only
AF_UNIX because the struct sockaddr_un type is used.  This function
should work with any address family.

Drop the sockaddr since the client address is unused and is an optional
argument to accept(2).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-posix.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index bb65d8b..bf32158 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -26,13 +26,10 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
     GAChannel *c = data;
     int ret, client_fd;
     bool accepted = false;
-    struct sockaddr_un addr;
-    socklen_t addrlen = sizeof(addr);
 
     g_assert(channel != NULL);
 
-    client_fd = qemu_accept(g_io_channel_unix_get_fd(channel),
-                            (struct sockaddr *)&addr, &addrlen);
+    client_fd = qemu_accept(g_io_channel_unix_get_fd(channel), NULL, NULL);
     if (client_fd == -1) {
         g_warning("error converting fd to gsocket: %s", strerror(errno));
         goto out;
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/4] qga: drop unnecessary GA_CHANNEL_UNIX_LISTEN checks

[Stefan Hajnoczi]

Throughout the code there are c->listen_channel checks which manage the
listen socket file descriptor (waiting for accept(2), closing the file
descriptor, etc).  These checks are currently preceded by explicit
c->method == GA_CHANNEL_UNIX_LISTEN checks.

Explicit GA_CHANNEL_UNIX_LISTEN checks are not necessary since serial
channel types do not create the listen channel (c->listen_channel).

As more listen channel types are added, explicitly checking all of them
becomes messy.  Rely on c->listen_channel to determine whether or not a
listen socket file descriptor is used.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-posix.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index bf32158..579891d 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -61,7 +61,6 @@ static void ga_channel_listen_add(GAChannel *c, int listen_fd, bool create)
 
 static void ga_channel_listen_close(GAChannel *c)
 {
-    g_assert(c->method == GA_CHANNEL_UNIX_LISTEN);
     g_assert(c->listen_channel);
     g_io_channel_shutdown(c->listen_channel, true, NULL);
     g_io_channel_unref(c->listen_channel);
@@ -77,7 +76,7 @@ static void ga_channel_client_close(GAChannel *c)
     g_io_channel_shutdown(c->client_channel, true, NULL);
     g_io_channel_unref(c->client_channel);
     c->client_channel = NULL;
-    if (c->method == GA_CHANNEL_UNIX_LISTEN && c->listen_channel) {
+    if (c->listen_channel) {
         ga_channel_listen_add(c, 0, false);
     }
 }
@@ -255,8 +254,7 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
 
 void ga_channel_free(GAChannel *c)
 {
-    if (c->method == GA_CHANNEL_UNIX_LISTEN
-        && c->listen_channel) {
+    if (c->listen_channel) {
         ga_channel_listen_close(c);
     }
     if (c->client_channel) {
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/4] sockets: add AF_VSOCK support

[Stefan Hajnoczi]

Add the AF_VSOCK address family so that qemu-ga will be able to use
virtio-vsock.

The AF_VSOCK address family uses <cid, port> address tuples.  The cid is
the unique identifier comparable to an IP address.  AF_VSOCK does not
use name resolution so it's easy to convert between struct sockaddr_vm
and strings.

This patch defines a VsockSocketAddress instead of trying to piggy-back
on InetSocketAddress.  This is cleaner in the long run since it avoids
lots of IPv4 vs IPv6 vs vsock special casing.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
v2:
 * s/seasy/easy/ typo fix in commit description [Eric]
 * Use %n to check for trailing characters in addresses [Eric]
---
 qapi-schema.json    |  23 +++++-
 util/qemu-sockets.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 9e47b47..12aea99 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -988,12 +988,14 @@
 #
 # @unix: unix socket
 #
+# @vsock: vsock family (since 2.8)
+#
 # @unknown: otherwise
 #
 # Since: 2.1
 ##
 { 'enum': 'NetworkAddressFamily',
-  'data': [ 'ipv4', 'ipv6', 'unix', 'unknown' ] }
+  'data': [ 'ipv4', 'ipv6', 'unix', 'vsock', 'unknown' ] }
 
 ##
 # @VncBasicInfo
@@ -3018,6 +3020,24 @@
     'path': 'str' } }
 
 ##
+# @VsockSocketAddress
+#
+# Captures a socket address in the vsock namespace.
+#
+# @cid: unique host identifier
+# @port: port
+#
+# Note that string types are used to allow for possible future hostname or
+# service resolution support.
+#
+# Since 2.8
+##
+{ 'struct': 'VsockSocketAddress',
+  'data': {
+    'cid': 'str',
+    'port': 'str' } }
+
+##
 # @SocketAddress
 #
 # Captures the address of a socket, which could also be a named file descriptor
@@ -3028,6 +3048,7 @@
   'data': {
     'inet': 'InetSocketAddress',
     'unix': 'UnixSocketAddress',
+    'vsock': 'VsockSocketAddress',
     'fd': 'String' } }
 
 ##
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6db48b3..6ef3cc5 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -17,6 +17,10 @@
  */
 #include "qemu/osdep.h"
 
+#ifdef AF_VSOCK
+#include <linux/vm_sockets.h>
+#endif /* AF_VSOCK */
+
 #include "monitor/monitor.h"
 #include "qapi/error.h"
 #include "qemu/sockets.h"
@@ -75,6 +79,9 @@ NetworkAddressFamily inet_netfamily(int family)
     case PF_INET6: return NETWORK_ADDRESS_FAMILY_IPV6;
     case PF_INET:  return NETWORK_ADDRESS_FAMILY_IPV4;
     case PF_UNIX:  return NETWORK_ADDRESS_FAMILY_UNIX;
+#ifdef AF_VSOCK
+    case PF_VSOCK: return NETWORK_ADDRESS_FAMILY_VSOCK;
+#endif /* AF_VSOCK */
     }
     return NETWORK_ADDRESS_FAMILY_UNKNOWN;
 }
@@ -650,6 +657,181 @@ int inet_connect(const char *str, Error **errp)
     return sock;
 }
 
+#ifdef AF_VSOCK
+static bool vsock_parse_vaddr_to_sockaddr(const VsockSocketAddress *vaddr,
+                                          struct sockaddr_vm *svm,
+                                          Error **errp)
+{
+    unsigned long long val;
+
+    memset(svm, 0, sizeof(*svm));
+    svm->svm_family = AF_VSOCK;
+
+    if (parse_uint_full(vaddr->cid, &val, 10) < 0 ||
+        val > UINT32_MAX) {
+        error_setg(errp, "Failed to parse cid '%s'", vaddr->cid);
+        return false;
+    }
+    svm->svm_cid = val;
+
+    if (parse_uint_full(vaddr->port, &val, 10) < 0 ||
+        val > UINT32_MAX) {
+        error_setg(errp, "Failed to parse port '%s'", vaddr->port);
+        return false;
+    }
+    svm->svm_port = val;
+
+    return true;
+}
+
+static int vsock_connect_addr(const struct sockaddr_vm *svm, bool *in_progress,
+                              ConnectState *connect_state, Error **errp)
+{
+    int sock, rc;
+
+    *in_progress = false;
+
+    sock = qemu_socket(AF_VSOCK, SOCK_STREAM, 0);
+    if (sock < 0) {
+        error_setg_errno(errp, errno, "Failed to create socket");
+        return -1;
+    }
+    if (connect_state != NULL) {
+        qemu_set_nonblock(sock);
+    }
+    /* connect to peer */
+    do {
+        rc = 0;
+        if (connect(sock, (const struct sockaddr *)svm, sizeof(*svm)) < 0) {
+            rc = -errno;
+        }
+    } while (rc == -EINTR);
+
+    if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
+        connect_state->fd = sock;
+        qemu_set_fd_handler(sock, NULL, wait_for_connect, connect_state);
+        *in_progress = true;
+    } else if (rc < 0) {
+        error_setg_errno(errp, errno, "Failed to connect socket");
+        closesocket(sock);
+        return -1;
+    }
+    return sock;
+}
+
+static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
+                               NonBlockingConnectHandler *callback,
+                               void *opaque)
+{
+    struct sockaddr_vm svm;
+    int sock = -1;
+    bool in_progress;
+    ConnectState *connect_state = NULL;
+
+    if (!vsock_parse_vaddr_to_sockaddr(vaddr, &svm, errp)) {
+        return -1;
+    }
+
+    if (callback != NULL) {
+        connect_state = g_malloc0(sizeof(*connect_state));
+        connect_state->callback = callback;
+        connect_state->opaque = opaque;
+    }
+
+    sock = vsock_connect_addr(&svm, &in_progress, connect_state, errp);
+    if (sock < 0) {
+        /* do nothing */
+    } else if (in_progress) {
+        /* wait_for_connect() will do the rest */
+        return sock;
+    } else {
+        if (callback) {
+            callback(sock, NULL, opaque);
+        }
+    }
+    g_free(connect_state);
+    return sock;
+}
+
+static int vsock_listen_saddr(VsockSocketAddress *vaddr,
+                              Error **errp)
+{
+    struct sockaddr_vm svm;
+    int slisten;
+
+    if (!vsock_parse_vaddr_to_sockaddr(vaddr, &svm, errp)) {
+        return -1;
+    }
+
+    slisten = qemu_socket(AF_VSOCK, SOCK_STREAM, 0);
+    if (slisten < 0) {
+        error_setg_errno(errp, errno, "Failed to create socket");
+        return -1;
+    }
+
+    if (bind(slisten, (const struct sockaddr *)&svm, sizeof(svm)) != 0) {
+        error_setg_errno(errp, errno, "Failed to bind socket");
+        closesocket(slisten);
+        return -1;
+    }
+
+    if (listen(slisten, 1) != 0) {
+        error_setg_errno(errp, errno, "Failed to listen on socket");
+        closesocket(slisten);
+        return -1;
+    }
+    return slisten;
+}
+
+static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
+{
+    VsockSocketAddress *addr = NULL;
+    char cid[33];
+    char port[33];
+    int n;
+
+    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
+        error_setg(errp, "error parsing address '%s'", str);
+        return NULL;
+    }
+    if (str[n] != '\0') {
+        error_setg(errp, "trailing characters in address '%s'", str);
+        return NULL;
+    }
+
+    addr = g_new0(VsockSocketAddress, 1);
+    addr->cid = g_strdup(cid);
+    addr->port = g_strdup(port);
+    return addr;
+}
+#else
+static void vsock_unsupported(Error **errp)
+{
+    error_setg(errp, "socket family AF_VSOCK unsupported");
+}
+
+static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
+                               NonBlockingConnectHandler *callback,
+                               void *opaque)
+{
+    vsock_unsupported(errp);
+    return -1;
+}
+
+static int vsock_listen_saddr(VsockSocketAddress *vaddr,
+                              Error **errp)
+{
+    vsock_unsupported(errp);
+    return -1;
+}
+
+static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
+{
+    vsock_unsupported(errp);
+    return NULL;
+}
+#endif /* AF_VSOCK */
+
 #ifndef _WIN32
 
 static int unix_listen_saddr(UnixSocketAddress *saddr,
@@ -864,6 +1046,12 @@ SocketAddress *socket_parse(const char *str, Error **errp)
             addr->u.fd.data = g_new(String, 1);
             addr->u.fd.data->str = g_strdup(str + 3);
         }
+    } else if (strstart(str, "vsock:", NULL)) {
+        addr->type = SOCKET_ADDRESS_KIND_VSOCK;
+        addr->u.vsock.data = vsock_parse(str + strlen("vsock:"), errp);
+        if (addr->u.vsock.data == NULL) {
+            goto fail;
+        }
     } else {
         addr->type = SOCKET_ADDRESS_KIND_INET;
         addr->u.inet.data = inet_parse(str, errp);
@@ -900,6 +1088,10 @@ int socket_connect(SocketAddress *addr, Error **errp,
         }
         break;
 
+    case SOCKET_ADDRESS_KIND_VSOCK:
+        fd = vsock_connect_saddr(addr->u.vsock.data, errp, callback, opaque);
+        break;
+
     default:
         abort();
     }
@@ -923,6 +1115,10 @@ int socket_listen(SocketAddress *addr, Error **errp)
         fd = monitor_get_fd(cur_mon, addr->u.fd.data->str, errp);
         break;
 
+    case SOCKET_ADDRESS_KIND_VSOCK:
+        fd = vsock_listen_saddr(addr->u.vsock.data, errp);
+        break;
+
     default:
         abort();
     }
@@ -1022,6 +1218,26 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
 }
 #endif /* WIN32 */
 
+#ifdef AF_VSOCK
+static SocketAddress *
+socket_sockaddr_to_address_vsock(struct sockaddr_storage *sa,
+                                 socklen_t salen,
+                                 Error **errp)
+{
+    SocketAddress *addr;
+    VsockSocketAddress *vaddr;
+    struct sockaddr_vm *svm = (struct sockaddr_vm *)sa;
+
+    addr = g_new0(SocketAddress, 1);
+    addr->type = SOCKET_ADDRESS_KIND_VSOCK;
+    addr->u.vsock.data = vaddr = g_new0(VsockSocketAddress, 1);
+    vaddr->cid = g_strdup_printf("%u", svm->svm_cid);
+    vaddr->port = g_strdup_printf("%u", svm->svm_port);
+
+    return addr;
+}
+#endif /* AF_VSOCK */
+
 SocketAddress *
 socket_sockaddr_to_address(struct sockaddr_storage *sa,
                            socklen_t salen,
@@ -1037,6 +1253,11 @@ socket_sockaddr_to_address(struct sockaddr_storage *sa,
         return socket_sockaddr_to_address_unix(sa, salen, errp);
 #endif /* WIN32 */
 
+#ifdef AF_VSOCK
+    case AF_VSOCK:
+        return socket_sockaddr_to_address_vsock(sa, salen, errp);
+#endif
+
     default:
         error_setg(errp, "socket family %d unsupported",
                    sa->ss_family);
@@ -1103,6 +1324,12 @@ char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
         buf = g_strdup(addr->u.fd.data->str);
         break;
 
+    case SOCKET_ADDRESS_KIND_VSOCK:
+        buf = g_strdup_printf("%s:%s",
+                              addr->u.vsock.data->cid,
+                              addr->u.vsock.data->port);
+        break;
+
     default:
         error_setg(errp, "socket family %d unsupported",
                    addr->type);
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/4] qga: add vsock-listen method

[Stefan Hajnoczi]

Add AF_VSOCK (virtio-vsock) support as an alternative to virtio-serial.

  $ qemu-system-x86_64 -device vhost-vsock-pci,guest-cid=3 ...
  (guest)# qemu-ga -m vsock-listen -p 3:1234

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-posix.c | 25 +++++++++++++++++++++++++
 qga/channel.h       |  1 +
 qga/main.c          |  6 ++++--
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 579891d..71582e0 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -193,6 +193,31 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
         ga_channel_listen_add(c, fd, true);
         break;
     }
+    case GA_CHANNEL_VSOCK_LISTEN: {
+        Error *local_err = NULL;
+        SocketAddress *addr;
+        char *addr_str;
+        int fd;
+
+        addr_str = g_strdup_printf("vsock:%s", path);
+        addr = socket_parse(addr_str, &local_err);
+        g_free(addr_str);
+        if (local_err != NULL) {
+            g_critical("%s", error_get_pretty(local_err));
+            error_free(local_err);
+            return false;
+        }
+
+        fd = socket_listen(addr, &local_err);
+        qapi_free_SocketAddress(addr);
+        if (local_err != NULL) {
+            g_critical("%s", error_get_pretty(local_err));
+            error_free(local_err);
+            return false;
+        }
+        ga_channel_listen_add(c, fd, true);
+        break;
+    }
     default:
         g_critical("error binding/listening to specified socket");
         return false;
diff --git a/qga/channel.h b/qga/channel.h
index ae8cf0f..8fd0c8f 100644
--- a/qga/channel.h
+++ b/qga/channel.h
@@ -19,6 +19,7 @@ typedef enum {
     GA_CHANNEL_VIRTIO_SERIAL,
     GA_CHANNEL_ISA_SERIAL,
     GA_CHANNEL_UNIX_LISTEN,
+    GA_CHANNEL_VSOCK_LISTEN,
 } GAChannelMethod;
 
 typedef gboolean (*GAChannelCallback)(GIOCondition condition, gpointer opaque);
diff --git a/qga/main.c b/qga/main.c
index 0b9d04e..6caf215 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -190,8 +190,8 @@ static void usage(const char *cmd)
 "Usage: %s [-m <method> -p <path>] [<options>]\n"
 "QEMU Guest Agent %s\n"
 "\n"
-"  -m, --method      transport method: one of unix-listen, virtio-serial, or\n"
-"                    isa-serial (virtio-serial is the default)\n"
+"  -m, --method      transport method: one of unix-listen, virtio-serial,\n"
+"                    isa-serial, or vsock-listen (virtio-serial is the default)\n"
 "  -p, --path        device/socket path (the default for virtio-serial is:\n"
 "                    %s,\n"
 "                    the default for isa-serial is:\n"
@@ -659,6 +659,8 @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path)
         channel_method = GA_CHANNEL_ISA_SERIAL;
     } else if (strcmp(method, "unix-listen") == 0) {
         channel_method = GA_CHANNEL_UNIX_LISTEN;
+    } else if (strcmp(method, "vsock-listen") == 0) {
+        channel_method = GA_CHANNEL_VSOCK_LISTEN;
     } else {
         g_critical("unsupported channel method/type: %s", method);
         return false;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 3/4] sockets: add AF_VSOCK support

[Eric Blake]

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

On 10/14/2016 04:00 AM, Stefan Hajnoczi wrote:
> Add the AF_VSOCK address family so that qemu-ga will be able to use
> virtio-vsock.
> 
> The AF_VSOCK address family uses <cid, port> address tuples.  The cid is
> the unique identifier comparable to an IP address.  AF_VSOCK does not
> use name resolution so it's easy to convert between struct sockaddr_vm
> and strings.
> 
> This patch defines a VsockSocketAddress instead of trying to piggy-back
> on InetSocketAddress.  This is cleaner in the long run since it avoids
> lots of IPv4 vs IPv6 vs vsock special casing.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

>  ##
> +# @VsockSocketAddress
> +#
> +# Captures a socket address in the vsock namespace.
> +#
> +# @cid: unique host identifier
> +# @port: port
> +#
> +# Note that string types are used to allow for possible future hostname or
> +# service resolution support.
> +#
> +# Since 2.8
> +##
> +{ 'struct': 'VsockSocketAddress',
> +  'data': {
> +    'cid': 'str',
> +    'port': 'str' } }

It would also be possible to do this now:

{ 'struct': 'VsockSocketAddress',
  'data': { 'cid': 'int', 'port': 'int' } }

then down the road expand to:

{ 'alternate': 'StrOrInt',
  'data': { 's': 'str', 'i': 'int' } }
{ 'struct': 'VsockSocketAddress',
  'data': { 'cid': 'StrOrInt', 'port': 'StrOrInt' } }

although the C code to do type-safe access vsock->cid.u.i or
vsock->cid.u.s based on vsock->cid.type is a bit more verbose than just
accessing vsock->cid as a string and converting every time around.
Where it gets really interesting is that using the alternate would allow
all of these QMP forms:

{ "socket": { "cid": 1, "port": 2 } }
{ "socket": { "cid": "1", "port": "2" } }
{ "socket": { "cid": "host", "port": "service" } }

It MIGHT be worth going the type-safe route, especially if we WANT to
convert the existing SocketAddress uses to use the alternate rather than
always managing things as a string.  But it doesn't have to be in this
patch.

> +static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
> +{
> +    VsockSocketAddress *addr = NULL;
> +    char cid[33];
> +    char port[33];
> +    int n;
> +
> +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {

This says stop at the first comma after the colon...

> +        error_setg(errp, "error parsing address '%s'", str);
> +        return NULL;
> +    }
> +    if (str[n] != '\0') {
> +        error_setg(errp, "trailing characters in address '%s'", str);

...but this rejects a trailing comma.  Is a trailing comma possible base
on how QemuOpts work? If so, do you need to handle it here?

Otherwise looking okay 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 3/4] sockets: add AF_VSOCK support

[Stefan Hajnoczi]

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

On Fri, Oct 14, 2016 at 09:40:40AM -0500, Eric Blake wrote:
> On 10/14/2016 04:00 AM, Stefan Hajnoczi wrote:
> > Add the AF_VSOCK address family so that qemu-ga will be able to use
> > virtio-vsock.
> > 
> > The AF_VSOCK address family uses <cid, port> address tuples.  The cid is
> > the unique identifier comparable to an IP address.  AF_VSOCK does not
> > use name resolution so it's easy to convert between struct sockaddr_vm
> > and strings.
> > 
> > This patch defines a VsockSocketAddress instead of trying to piggy-back
> > on InetSocketAddress.  This is cleaner in the long run since it avoids
> > lots of IPv4 vs IPv6 vs vsock special casing.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> 
> >  ##
> > +# @VsockSocketAddress
> > +#
> > +# Captures a socket address in the vsock namespace.
> > +#
> > +# @cid: unique host identifier
> > +# @port: port
> > +#
> > +# Note that string types are used to allow for possible future hostname or
> > +# service resolution support.
> > +#
> > +# Since 2.8
> > +##
> > +{ 'struct': 'VsockSocketAddress',
> > +  'data': {
> > +    'cid': 'str',
> > +    'port': 'str' } }
> 
> It would also be possible to do this now:
> 
> { 'struct': 'VsockSocketAddress',
>   'data': { 'cid': 'int', 'port': 'int' } }
> 
> then down the road expand to:
> 
> { 'alternate': 'StrOrInt',
>   'data': { 's': 'str', 'i': 'int' } }
> { 'struct': 'VsockSocketAddress',
>   'data': { 'cid': 'StrOrInt', 'port': 'StrOrInt' } }
> 
> although the C code to do type-safe access vsock->cid.u.i or
> vsock->cid.u.s based on vsock->cid.type is a bit more verbose than just
> accessing vsock->cid as a string and converting every time around.
> Where it gets really interesting is that using the alternate would allow
> all of these QMP forms:
> 
> { "socket": { "cid": 1, "port": 2 } }
> { "socket": { "cid": "1", "port": "2" } }
> { "socket": { "cid": "host", "port": "service" } }
> 
> It MIGHT be worth going the type-safe route, especially if we WANT to
> convert the existing SocketAddress uses to use the alternate rather than
> always managing things as a string.  But it doesn't have to be in this
> patch.
> 
> > +static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
> > +{
> > +    VsockSocketAddress *addr = NULL;
> > +    char cid[33];
> > +    char port[33];
> > +    int n;
> > +
> > +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
> 
> This says stop at the first comma after the colon...
> 
> > +        error_setg(errp, "error parsing address '%s'", str);
> > +        return NULL;
> > +    }
> > +    if (str[n] != '\0') {
> > +        error_setg(errp, "trailing characters in address '%s'", str);
> 
> ...but this rejects a trailing comma.  Is a trailing comma possible base
> on how QemuOpts work? If so, do you need to handle it here?

Actually I just wanted to grab characters up until the end of string.
It wasn't clear from the sscanf(3) man page what the best way to do that
was, so I kept the comma which is also used in tcp addresses (because
they support additional comma-separated options).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] sockets: add AF_VSOCK support

[Eric Blake]

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

On 10/16/2016 08:35 AM, Stefan Hajnoczi wrote:

>>> +
>>> +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
>>
>> This says stop at the first comma after the colon...
>>
>>> +        error_setg(errp, "error parsing address '%s'", str);
>>> +        return NULL;
>>> +    }
>>> +    if (str[n] != '\0') {
>>> +        error_setg(errp, "trailing characters in address '%s'", str);
>>
>> ...but this rejects a trailing comma.  Is a trailing comma possible base
>> on how QemuOpts work? If so, do you need to handle it here?
> 
> Actually I just wanted to grab characters up until the end of string.
> It wasn't clear from the sscanf(3) man page what the best way to do that
> was, so I kept the comma which is also used in tcp addresses (because
> they support additional comma-separated options).

%32s instead of %32[^,] should grab up to all 32 remaining characters in
the string; your %n trick then ensures there is no garbage.  I guess
it's still a question of whether we want to always treat a comma as
trailing garbage.

-- 
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/4] sockets: add AF_VSOCK support

[Stefan Hajnoczi]

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

On Mon, Oct 17, 2016 at 09:25:46AM -0500, Eric Blake wrote:
> On 10/16/2016 08:35 AM, Stefan Hajnoczi wrote:
> 
> >>> +
> >>> +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
> >>
> >> This says stop at the first comma after the colon...
> >>
> >>> +        error_setg(errp, "error parsing address '%s'", str);
> >>> +        return NULL;
> >>> +    }
> >>> +    if (str[n] != '\0') {
> >>> +        error_setg(errp, "trailing characters in address '%s'", str);
> >>
> >> ...but this rejects a trailing comma.  Is a trailing comma possible base
> >> on how QemuOpts work? If so, do you need to handle it here?
> > 
> > Actually I just wanted to grab characters up until the end of string.
> > It wasn't clear from the sscanf(3) man page what the best way to do that
> > was, so I kept the comma which is also used in tcp addresses (because
> > they support additional comma-separated options).
> 
> %32s instead of %32[^,] should grab up to all 32 remaining characters in
> the string; your %n trick then ensures there is no garbage.  I guess
> it's still a question of whether we want to always treat a comma as
> trailing garbage.

No comma options are currently accepted.  Treating comma as garbage
seems okay.

Mike: Please change %32[^,] to %32s when merging.  Thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] sockets: add AF_VSOCK support

[Michael Roth]

Quoting Stefan Hajnoczi (2016-10-14 04:00:55)
> Add the AF_VSOCK address family so that qemu-ga will be able to use
> virtio-vsock.
> 
> The AF_VSOCK address family uses <cid, port> address tuples.  The cid is
> the unique identifier comparable to an IP address.  AF_VSOCK does not
> use name resolution so it's easy to convert between struct sockaddr_vm
> and strings.
> 
> This patch defines a VsockSocketAddress instead of trying to piggy-back
> on InetSocketAddress.  This is cleaner in the long run since it avoids
> lots of IPv4 vs IPv6 vs vsock special casing.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v2:
>  * s/seasy/easy/ typo fix in commit description [Eric]
>  * Use %n to check for trailing characters in addresses [Eric]
> ---
>  qapi-schema.json    |  23 +++++-
>  util/qemu-sockets.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 249 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9e47b47..12aea99 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -988,12 +988,14 @@
>  #
>  # @unix: unix socket
>  #
> +# @vsock: vsock family (since 2.8)
> +#
>  # @unknown: otherwise
>  #
>  # Since: 2.1
>  ##
>  { 'enum': 'NetworkAddressFamily',
> -  'data': [ 'ipv4', 'ipv6', 'unix', 'unknown' ] }
> +  'data': [ 'ipv4', 'ipv6', 'unix', 'vsock', 'unknown' ] }
> 
>  ##
>  # @VncBasicInfo
> @@ -3018,6 +3020,24 @@
>      'path': 'str' } }
> 
>  ##
> +# @VsockSocketAddress
> +#
> +# Captures a socket address in the vsock namespace.
> +#
> +# @cid: unique host identifier
> +# @port: port
> +#
> +# Note that string types are used to allow for possible future hostname or
> +# service resolution support.
> +#
> +# Since 2.8
> +##
> +{ 'struct': 'VsockSocketAddress',
> +  'data': {
> +    'cid': 'str',
> +    'port': 'str' } }
> +
> +##
>  # @SocketAddress
>  #
>  # Captures the address of a socket, which could also be a named file descriptor
> @@ -3028,6 +3048,7 @@
>    'data': {
>      'inet': 'InetSocketAddress',
>      'unix': 'UnixSocketAddress',
> +    'vsock': 'VsockSocketAddress',
>      'fd': 'String' } }
> 
>  ##
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 6db48b3..6ef3cc5 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -17,6 +17,10 @@
>   */
>  #include "qemu/osdep.h"
> 
> +#ifdef AF_VSOCK
> +#include <linux/vm_sockets.h>
> +#endif /* AF_VSOCK */

I have this series applied locally but I hit some build issues on Ubuntu
14.04 due to linux/vm_sockets.h not being provided by Ubuntu 14.04's
linux-libc-dev package. It is however included with linux-libc-dev in
16.04. linux-headers package includes it in both cases, but installs
to /usr/src/linux-headers*, which are not part of the default include
path.

Do you think we need a configure check and CONFIG_AF_VSOCK flag instead?

> +
>  #include "monitor/monitor.h"
>  #include "qapi/error.h"
>  #include "qemu/sockets.h"
> @@ -75,6 +79,9 @@ NetworkAddressFamily inet_netfamily(int family)
>      case PF_INET6: return NETWORK_ADDRESS_FAMILY_IPV6;
>      case PF_INET:  return NETWORK_ADDRESS_FAMILY_IPV4;
>      case PF_UNIX:  return NETWORK_ADDRESS_FAMILY_UNIX;
> +#ifdef AF_VSOCK
> +    case PF_VSOCK: return NETWORK_ADDRESS_FAMILY_VSOCK;
> +#endif /* AF_VSOCK */
>      }
>      return NETWORK_ADDRESS_FAMILY_UNKNOWN;
>  }
> @@ -650,6 +657,181 @@ int inet_connect(const char *str, Error **errp)
>      return sock;
>  }
> 
> +#ifdef AF_VSOCK
> +static bool vsock_parse_vaddr_to_sockaddr(const VsockSocketAddress *vaddr,
> +                                          struct sockaddr_vm *svm,
> +                                          Error **errp)
> +{
> +    unsigned long long val;
> +
> +    memset(svm, 0, sizeof(*svm));
> +    svm->svm_family = AF_VSOCK;
> +
> +    if (parse_uint_full(vaddr->cid, &val, 10) < 0 ||
> +        val > UINT32_MAX) {
> +        error_setg(errp, "Failed to parse cid '%s'", vaddr->cid);
> +        return false;
> +    }
> +    svm->svm_cid = val;
> +
> +    if (parse_uint_full(vaddr->port, &val, 10) < 0 ||
> +        val > UINT32_MAX) {
> +        error_setg(errp, "Failed to parse port '%s'", vaddr->port);
> +        return false;
> +    }
> +    svm->svm_port = val;
> +
> +    return true;
> +}
> +
> +static int vsock_connect_addr(const struct sockaddr_vm *svm, bool *in_progress,
> +                              ConnectState *connect_state, Error **errp)
> +{
> +    int sock, rc;
> +
> +    *in_progress = false;
> +
> +    sock = qemu_socket(AF_VSOCK, SOCK_STREAM, 0);
> +    if (sock < 0) {
> +        error_setg_errno(errp, errno, "Failed to create socket");
> +        return -1;
> +    }
> +    if (connect_state != NULL) {
> +        qemu_set_nonblock(sock);
> +    }
> +    /* connect to peer */
> +    do {
> +        rc = 0;
> +        if (connect(sock, (const struct sockaddr *)svm, sizeof(*svm)) < 0) {
> +            rc = -errno;
> +        }
> +    } while (rc == -EINTR);
> +
> +    if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +        connect_state->fd = sock;
> +        qemu_set_fd_handler(sock, NULL, wait_for_connect, connect_state);
> +        *in_progress = true;
> +    } else if (rc < 0) {
> +        error_setg_errno(errp, errno, "Failed to connect socket");
> +        closesocket(sock);
> +        return -1;
> +    }
> +    return sock;
> +}
> +
> +static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
> +                               NonBlockingConnectHandler *callback,
> +                               void *opaque)
> +{
> +    struct sockaddr_vm svm;
> +    int sock = -1;
> +    bool in_progress;
> +    ConnectState *connect_state = NULL;
> +
> +    if (!vsock_parse_vaddr_to_sockaddr(vaddr, &svm, errp)) {
> +        return -1;
> +    }
> +
> +    if (callback != NULL) {
> +        connect_state = g_malloc0(sizeof(*connect_state));
> +        connect_state->callback = callback;
> +        connect_state->opaque = opaque;
> +    }
> +
> +    sock = vsock_connect_addr(&svm, &in_progress, connect_state, errp);
> +    if (sock < 0) {
> +        /* do nothing */
> +    } else if (in_progress) {
> +        /* wait_for_connect() will do the rest */
> +        return sock;
> +    } else {
> +        if (callback) {
> +            callback(sock, NULL, opaque);
> +        }
> +    }
> +    g_free(connect_state);
> +    return sock;
> +}
> +
> +static int vsock_listen_saddr(VsockSocketAddress *vaddr,
> +                              Error **errp)
> +{
> +    struct sockaddr_vm svm;
> +    int slisten;
> +
> +    if (!vsock_parse_vaddr_to_sockaddr(vaddr, &svm, errp)) {
> +        return -1;
> +    }
> +
> +    slisten = qemu_socket(AF_VSOCK, SOCK_STREAM, 0);
> +    if (slisten < 0) {
> +        error_setg_errno(errp, errno, "Failed to create socket");
> +        return -1;
> +    }
> +
> +    if (bind(slisten, (const struct sockaddr *)&svm, sizeof(svm)) != 0) {
> +        error_setg_errno(errp, errno, "Failed to bind socket");
> +        closesocket(slisten);
> +        return -1;
> +    }
> +
> +    if (listen(slisten, 1) != 0) {
> +        error_setg_errno(errp, errno, "Failed to listen on socket");
> +        closesocket(slisten);
> +        return -1;
> +    }
> +    return slisten;
> +}
> +
> +static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
> +{
> +    VsockSocketAddress *addr = NULL;
> +    char cid[33];
> +    char port[33];
> +    int n;
> +
> +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
> +        error_setg(errp, "error parsing address '%s'", str);
> +        return NULL;
> +    }
> +    if (str[n] != '\0') {
> +        error_setg(errp, "trailing characters in address '%s'", str);
> +        return NULL;
> +    }
> +
> +    addr = g_new0(VsockSocketAddress, 1);
> +    addr->cid = g_strdup(cid);
> +    addr->port = g_strdup(port);
> +    return addr;
> +}
> +#else
> +static void vsock_unsupported(Error **errp)
> +{
> +    error_setg(errp, "socket family AF_VSOCK unsupported");
> +}
> +
> +static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
> +                               NonBlockingConnectHandler *callback,
> +                               void *opaque)
> +{
> +    vsock_unsupported(errp);
> +    return -1;
> +}
> +
> +static int vsock_listen_saddr(VsockSocketAddress *vaddr,
> +                              Error **errp)
> +{
> +    vsock_unsupported(errp);
> +    return -1;
> +}
> +
> +static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
> +{
> +    vsock_unsupported(errp);
> +    return NULL;
> +}
> +#endif /* AF_VSOCK */
> +
>  #ifndef _WIN32
> 
>  static int unix_listen_saddr(UnixSocketAddress *saddr,
> @@ -864,6 +1046,12 @@ SocketAddress *socket_parse(const char *str, Error **errp)
>              addr->u.fd.data = g_new(String, 1);
>              addr->u.fd.data->str = g_strdup(str + 3);
>          }
> +    } else if (strstart(str, "vsock:", NULL)) {
> +        addr->type = SOCKET_ADDRESS_KIND_VSOCK;
> +        addr->u.vsock.data = vsock_parse(str + strlen("vsock:"), errp);
> +        if (addr->u.vsock.data == NULL) {
> +            goto fail;
> +        }
>      } else {
>          addr->type = SOCKET_ADDRESS_KIND_INET;
>          addr->u.inet.data = inet_parse(str, errp);
> @@ -900,6 +1088,10 @@ int socket_connect(SocketAddress *addr, Error **errp,
>          }
>          break;
> 
> +    case SOCKET_ADDRESS_KIND_VSOCK:
> +        fd = vsock_connect_saddr(addr->u.vsock.data, errp, callback, opaque);
> +        break;
> +
>      default:
>          abort();
>      }
> @@ -923,6 +1115,10 @@ int socket_listen(SocketAddress *addr, Error **errp)
>          fd = monitor_get_fd(cur_mon, addr->u.fd.data->str, errp);
>          break;
> 
> +    case SOCKET_ADDRESS_KIND_VSOCK:
> +        fd = vsock_listen_saddr(addr->u.vsock.data, errp);
> +        break;
> +
>      default:
>          abort();
>      }
> @@ -1022,6 +1218,26 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>  }
>  #endif /* WIN32 */
> 
> +#ifdef AF_VSOCK
> +static SocketAddress *
> +socket_sockaddr_to_address_vsock(struct sockaddr_storage *sa,
> +                                 socklen_t salen,
> +                                 Error **errp)
> +{
> +    SocketAddress *addr;
> +    VsockSocketAddress *vaddr;
> +    struct sockaddr_vm *svm = (struct sockaddr_vm *)sa;
> +
> +    addr = g_new0(SocketAddress, 1);
> +    addr->type = SOCKET_ADDRESS_KIND_VSOCK;
> +    addr->u.vsock.data = vaddr = g_new0(VsockSocketAddress, 1);
> +    vaddr->cid = g_strdup_printf("%u", svm->svm_cid);
> +    vaddr->port = g_strdup_printf("%u", svm->svm_port);
> +
> +    return addr;
> +}
> +#endif /* AF_VSOCK */
> +
>  SocketAddress *
>  socket_sockaddr_to_address(struct sockaddr_storage *sa,
>                             socklen_t salen,
> @@ -1037,6 +1253,11 @@ socket_sockaddr_to_address(struct sockaddr_storage *sa,
>          return socket_sockaddr_to_address_unix(sa, salen, errp);
>  #endif /* WIN32 */
> 
> +#ifdef AF_VSOCK
> +    case AF_VSOCK:
> +        return socket_sockaddr_to_address_vsock(sa, salen, errp);
> +#endif
> +
>      default:
>          error_setg(errp, "socket family %d unsupported",
>                     sa->ss_family);
> @@ -1103,6 +1324,12 @@ char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
>          buf = g_strdup(addr->u.fd.data->str);
>          break;
> 
> +    case SOCKET_ADDRESS_KIND_VSOCK:
> +        buf = g_strdup_printf("%s:%s",
> +                              addr->u.vsock.data->cid,
> +                              addr->u.vsock.data->port);
> +        break;
> +
>      default:
>          error_setg(errp, "socket family %d unsupported",
>                     addr->type);
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH v2 3/4] sockets: add AF_VSOCK support

[Michael Roth]

Quoting Michael Roth (2016-10-25 18:51:19)
> Quoting Stefan Hajnoczi (2016-10-14 04:00:55)
> > Add the AF_VSOCK address family so that qemu-ga will be able to use
> > virtio-vsock.
> > 
> > The AF_VSOCK address family uses <cid, port> address tuples.  The cid is
> > the unique identifier comparable to an IP address.  AF_VSOCK does not
> > use name resolution so it's easy to convert between struct sockaddr_vm
> > and strings.
> > 
> > This patch defines a VsockSocketAddress instead of trying to piggy-back
> > on InetSocketAddress.  This is cleaner in the long run since it avoids
> > lots of IPv4 vs IPv6 vs vsock special casing.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > v2:
> >  * s/seasy/easy/ typo fix in commit description [Eric]
> >  * Use %n to check for trailing characters in addresses [Eric]
> > ---
> >  qapi-schema.json    |  23 +++++-
> >  util/qemu-sockets.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 249 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 9e47b47..12aea99 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -988,12 +988,14 @@
> >  #
> >  # @unix: unix socket
> >  #
> > +# @vsock: vsock family (since 2.8)
> > +#
> >  # @unknown: otherwise
> >  #
> >  # Since: 2.1
> >  ##
> >  { 'enum': 'NetworkAddressFamily',
> > -  'data': [ 'ipv4', 'ipv6', 'unix', 'unknown' ] }
> > +  'data': [ 'ipv4', 'ipv6', 'unix', 'vsock', 'unknown' ] }
> > 
> >  ##
> >  # @VncBasicInfo
> > @@ -3018,6 +3020,24 @@
> >      'path': 'str' } }
> > 
> >  ##
> > +# @VsockSocketAddress
> > +#
> > +# Captures a socket address in the vsock namespace.
> > +#
> > +# @cid: unique host identifier
> > +# @port: port
> > +#
> > +# Note that string types are used to allow for possible future hostname or
> > +# service resolution support.
> > +#
> > +# Since 2.8
> > +##
> > +{ 'struct': 'VsockSocketAddress',
> > +  'data': {
> > +    'cid': 'str',
> > +    'port': 'str' } }
> > +
> > +##
> >  # @SocketAddress
> >  #
> >  # Captures the address of a socket, which could also be a named file descriptor
> > @@ -3028,6 +3048,7 @@
> >    'data': {
> >      'inet': 'InetSocketAddress',
> >      'unix': 'UnixSocketAddress',
> > +    'vsock': 'VsockSocketAddress',
> >      'fd': 'String' } }
> > 
> >  ##
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 6db48b3..6ef3cc5 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -17,6 +17,10 @@
> >   */
> >  #include "qemu/osdep.h"
> > 
> > +#ifdef AF_VSOCK
> > +#include <linux/vm_sockets.h>
> > +#endif /* AF_VSOCK */
> 
> I have this series applied locally but I hit some build issues on Ubuntu
> 14.04 due to linux/vm_sockets.h not being provided by Ubuntu 14.04's
> linux-libc-dev package. It is however included with linux-libc-dev in
> 16.04. linux-headers package includes it in both cases, but installs
> to /usr/src/linux-headers*, which are not part of the default include
> path.
> 
> Do you think we need a configure check and CONFIG_AF_VSOCK flag instead?

I've applied this series with the above-mentioned configure check
squashed into this patch. Here's the diff:

diff --git a/configure b/configure
index d3dafcb..60f1060 100755
--- a/configure
+++ b/configure
@@ -4605,6 +4605,33 @@ if compile_prog "" "" ; then
     have_rtnetlink=yes
 fi
 
+##########################################
+# check for usable AF_VSOCK environment
+have_af_vsock=no
+cat > $TMPC << EOF
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#if !defined(AF_VSOCK)
+# error missing AF_VSOCK flag
+#endif
+#include <linux/vm_sockets.h>
+int main(void) {
+    int sock, ret;
+    struct sockaddr_vm svm;
+    socklen_t len = sizeof(svm);
+    sock = socket(AF_VSOCK, SOCK_STREAM, 0);
+    ret = getpeername(sock, (struct sockaddr *)&svm, &len);
+    if ((ret == -1) && (errno == ENOTCONN)) {
+        return 0;
+    }
+    return -1;
+}
+EOF
+if compile_prog "" "" ; then
+    have_af_vsock=yes
+fi
+
 #################################################
 # Sparc implicitly links with --relax, which is
 # incompatible with -r, so --no-relax should be
@@ -5580,6 +5607,10 @@ if test "$replication" = "yes" ; then
   echo "CONFIG_REPLICATION=y" >> $config_host_mak
 fi
 
+if test "$have_af_vsock" = "yes" ; then
+  echo "CONFIG_AF_VSOCK=y" >> $config_host_mak
+fi
+
 # Hold two types of flag:
 #   CONFIG_THREAD_SETNAME_BYTHREAD  - we've got a way of setting the name on
 #                                     a thread we have a handle to
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 559f452..e616f48 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -17,9 +17,9 @@
  */
 #include "qemu/osdep.h"
 
-#ifdef AF_VSOCK
+#ifdef CONFIG_AF_VSOCK
 #include <linux/vm_sockets.h>
-#endif /* AF_VSOCK */
+#endif /* CONFIG_AF_VSOCK */
 
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -79,9 +79,9 @@ NetworkAddressFamily inet_netfamily(int family)
     case PF_INET6: return NETWORK_ADDRESS_FAMILY_IPV6;
     case PF_INET:  return NETWORK_ADDRESS_FAMILY_IPV4;
     case PF_UNIX:  return NETWORK_ADDRESS_FAMILY_UNIX;
-#ifdef AF_VSOCK
+#ifdef CONFIG_AF_VSOCK
     case PF_VSOCK: return NETWORK_ADDRESS_FAMILY_VSOCK;
-#endif /* AF_VSOCK */
+#endif /* CONFIG_AF_VSOCK */
     }
     return NETWORK_ADDRESS_FAMILY_UNKNOWN;
 }
@@ -657,7 +657,7 @@ int inet_connect(const char *str, Error **errp)
     return sock;
 }
 
-#ifdef AF_VSOCK
+#ifdef CONFIG_AF_VSOCK
 static bool vsock_parse_vaddr_to_sockaddr(const VsockSocketAddress *vaddr,
                                           struct sockaddr_vm *svm,
                                           Error **errp)
@@ -830,7 +830,7 @@ static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
     vsock_unsupported(errp);
     return NULL;
 }
-#endif /* AF_VSOCK */
+#endif /* CONFIG_AF_VSOCK */
 
 #ifndef _WIN32
 
@@ -1218,7 +1218,7 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
 }
 #endif /* WIN32 */
 
-#ifdef AF_VSOCK
+#ifdef CONFIG_AF_VSOCK
 static SocketAddress *
 socket_sockaddr_to_address_vsock(struct sockaddr_storage *sa,
                                  socklen_t salen,
@@ -1236,7 +1236,7 @@ socket_sockaddr_to_address_vsock(struct sockaddr_storage *sa,
 
     return addr;
 }
-#endif /* AF_VSOCK */
+#endif /* CONFIG_AF_VSOCK */
 
 SocketAddress *
 socket_sockaddr_to_address(struct sockaddr_storage *sa,
@@ -1253,7 +1253,7 @@ socket_sockaddr_to_address(struct sockaddr_storage *sa,
         return socket_sockaddr_to_address_unix(sa, salen, errp);
 #endif /* WIN32 */
 
-#ifdef AF_VSOCK
+#ifdef CONFIG_AF_VSOCK
     case AF_VSOCK:
         return socket_sockaddr_to_address_vsock(sa, salen, errp);
 #endif

> 
> > +
> >  #include "monitor/monitor.h"
> >  #include "qapi/error.h"
> >  #include "qemu/sockets.h"
> > @@ -75,6 +79,9 @@ NetworkAddressFamily inet_netfamily(int family)
> >      case PF_INET6: return NETWORK_ADDRESS_FAMILY_IPV6;
> >      case PF_INET:  return NETWORK_ADDRESS_FAMILY_IPV4;
> >      case PF_UNIX:  return NETWORK_ADDRESS_FAMILY_UNIX;
> > +#ifdef AF_VSOCK
> > +    case PF_VSOCK: return NETWORK_ADDRESS_FAMILY_VSOCK;
> > +#endif /* AF_VSOCK */
> >      }
> >      return NETWORK_ADDRESS_FAMILY_UNKNOWN;
> >  }
> > @@ -650,6 +657,181 @@ int inet_connect(const char *str, Error **errp)
> >      return sock;
> >  }
> > 
> > +#ifdef AF_VSOCK
> > +static bool vsock_parse_vaddr_to_sockaddr(const VsockSocketAddress *vaddr,
> > +                                          struct sockaddr_vm *svm,
> > +                                          Error **errp)
> > +{
> > +    unsigned long long val;
> > +
> > +    memset(svm, 0, sizeof(*svm));
> > +    svm->svm_family = AF_VSOCK;
> > +
> > +    if (parse_uint_full(vaddr->cid, &val, 10) < 0 ||
> > +        val > UINT32_MAX) {
> > +        error_setg(errp, "Failed to parse cid '%s'", vaddr->cid);
> > +        return false;
> > +    }
> > +    svm->svm_cid = val;
> > +
> > +    if (parse_uint_full(vaddr->port, &val, 10) < 0 ||
> > +        val > UINT32_MAX) {
> > +        error_setg(errp, "Failed to parse port '%s'", vaddr->port);
> > +        return false;
> > +    }
> > +    svm->svm_port = val;
> > +
> > +    return true;
> > +}
> > +
> > +static int vsock_connect_addr(const struct sockaddr_vm *svm, bool *in_progress,
> > +                              ConnectState *connect_state, Error **errp)
> > +{
> > +    int sock, rc;
> > +
> > +    *in_progress = false;
> > +
> > +    sock = qemu_socket(AF_VSOCK, SOCK_STREAM, 0);
> > +    if (sock < 0) {
> > +        error_setg_errno(errp, errno, "Failed to create socket");
> > +        return -1;
> > +    }
> > +    if (connect_state != NULL) {
> > +        qemu_set_nonblock(sock);
> > +    }
> > +    /* connect to peer */
> > +    do {
> > +        rc = 0;
> > +        if (connect(sock, (const struct sockaddr *)svm, sizeof(*svm)) < 0) {
> > +            rc = -errno;
> > +        }
> > +    } while (rc == -EINTR);
> > +
> > +    if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> > +        connect_state->fd = sock;
> > +        qemu_set_fd_handler(sock, NULL, wait_for_connect, connect_state);
> > +        *in_progress = true;
> > +    } else if (rc < 0) {
> > +        error_setg_errno(errp, errno, "Failed to connect socket");
> > +        closesocket(sock);
> > +        return -1;
> > +    }
> > +    return sock;
> > +}
> > +
> > +static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
> > +                               NonBlockingConnectHandler *callback,
> > +                               void *opaque)
> > +{
> > +    struct sockaddr_vm svm;
> > +    int sock = -1;
> > +    bool in_progress;
> > +    ConnectState *connect_state = NULL;
> > +
> > +    if (!vsock_parse_vaddr_to_sockaddr(vaddr, &svm, errp)) {
> > +        return -1;
> > +    }
> > +
> > +    if (callback != NULL) {
> > +        connect_state = g_malloc0(sizeof(*connect_state));
> > +        connect_state->callback = callback;
> > +        connect_state->opaque = opaque;
> > +    }
> > +
> > +    sock = vsock_connect_addr(&svm, &in_progress, connect_state, errp);
> > +    if (sock < 0) {
> > +        /* do nothing */
> > +    } else if (in_progress) {
> > +        /* wait_for_connect() will do the rest */
> > +        return sock;
> > +    } else {
> > +        if (callback) {
> > +            callback(sock, NULL, opaque);
> > +        }
> > +    }
> > +    g_free(connect_state);
> > +    return sock;
> > +}
> > +
> > +static int vsock_listen_saddr(VsockSocketAddress *vaddr,
> > +                              Error **errp)
> > +{
> > +    struct sockaddr_vm svm;
> > +    int slisten;
> > +
> > +    if (!vsock_parse_vaddr_to_sockaddr(vaddr, &svm, errp)) {
> > +        return -1;
> > +    }
> > +
> > +    slisten = qemu_socket(AF_VSOCK, SOCK_STREAM, 0);
> > +    if (slisten < 0) {
> > +        error_setg_errno(errp, errno, "Failed to create socket");
> > +        return -1;
> > +    }
> > +
> > +    if (bind(slisten, (const struct sockaddr *)&svm, sizeof(svm)) != 0) {
> > +        error_setg_errno(errp, errno, "Failed to bind socket");
> > +        closesocket(slisten);
> > +        return -1;
> > +    }
> > +
> > +    if (listen(slisten, 1) != 0) {
> > +        error_setg_errno(errp, errno, "Failed to listen on socket");
> > +        closesocket(slisten);
> > +        return -1;
> > +    }
> > +    return slisten;
> > +}
> > +
> > +static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
> > +{
> > +    VsockSocketAddress *addr = NULL;
> > +    char cid[33];
> > +    char port[33];
> > +    int n;
> > +
> > +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
> > +        error_setg(errp, "error parsing address '%s'", str);
> > +        return NULL;
> > +    }
> > +    if (str[n] != '\0') {
> > +        error_setg(errp, "trailing characters in address '%s'", str);
> > +        return NULL;
> > +    }
> > +
> > +    addr = g_new0(VsockSocketAddress, 1);
> > +    addr->cid = g_strdup(cid);
> > +    addr->port = g_strdup(port);
> > +    return addr;
> > +}
> > +#else
> > +static void vsock_unsupported(Error **errp)
> > +{
> > +    error_setg(errp, "socket family AF_VSOCK unsupported");
> > +}
> > +
> > +static int vsock_connect_saddr(VsockSocketAddress *vaddr, Error **errp,
> > +                               NonBlockingConnectHandler *callback,
> > +                               void *opaque)
> > +{
> > +    vsock_unsupported(errp);
> > +    return -1;
> > +}
> > +
> > +static int vsock_listen_saddr(VsockSocketAddress *vaddr,
> > +                              Error **errp)
> > +{
> > +    vsock_unsupported(errp);
> > +    return -1;
> > +}
> > +
> > +static VsockSocketAddress *vsock_parse(const char *str, Error **errp)
> > +{
> > +    vsock_unsupported(errp);
> > +    return NULL;
> > +}
> > +#endif /* AF_VSOCK */
> > +
> >  #ifndef _WIN32
> > 
> >  static int unix_listen_saddr(UnixSocketAddress *saddr,
> > @@ -864,6 +1046,12 @@ SocketAddress *socket_parse(const char *str, Error **errp)
> >              addr->u.fd.data = g_new(String, 1);
> >              addr->u.fd.data->str = g_strdup(str + 3);
> >          }
> > +    } else if (strstart(str, "vsock:", NULL)) {
> > +        addr->type = SOCKET_ADDRESS_KIND_VSOCK;
> > +        addr->u.vsock.data = vsock_parse(str + strlen("vsock:"), errp);
> > +        if (addr->u.vsock.data == NULL) {
> > +            goto fail;
> > +        }
> >      } else {
> >          addr->type = SOCKET_ADDRESS_KIND_INET;
> >          addr->u.inet.data = inet_parse(str, errp);
> > @@ -900,6 +1088,10 @@ int socket_connect(SocketAddress *addr, Error **errp,
> >          }
> >          break;
> > 
> > +    case SOCKET_ADDRESS_KIND_VSOCK:
> > +        fd = vsock_connect_saddr(addr->u.vsock.data, errp, callback, opaque);
> > +        break;
> > +
> >      default:
> >          abort();
> >      }
> > @@ -923,6 +1115,10 @@ int socket_listen(SocketAddress *addr, Error **errp)
> >          fd = monitor_get_fd(cur_mon, addr->u.fd.data->str, errp);
> >          break;
> > 
> > +    case SOCKET_ADDRESS_KIND_VSOCK:
> > +        fd = vsock_listen_saddr(addr->u.vsock.data, errp);
> > +        break;
> > +
> >      default:
> >          abort();
> >      }
> > @@ -1022,6 +1218,26 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
> >  }
> >  #endif /* WIN32 */
> > 
> > +#ifdef AF_VSOCK
> > +static SocketAddress *
> > +socket_sockaddr_to_address_vsock(struct sockaddr_storage *sa,
> > +                                 socklen_t salen,
> > +                                 Error **errp)
> > +{
> > +    SocketAddress *addr;
> > +    VsockSocketAddress *vaddr;
> > +    struct sockaddr_vm *svm = (struct sockaddr_vm *)sa;
> > +
> > +    addr = g_new0(SocketAddress, 1);
> > +    addr->type = SOCKET_ADDRESS_KIND_VSOCK;
> > +    addr->u.vsock.data = vaddr = g_new0(VsockSocketAddress, 1);
> > +    vaddr->cid = g_strdup_printf("%u", svm->svm_cid);
> > +    vaddr->port = g_strdup_printf("%u", svm->svm_port);
> > +
> > +    return addr;
> > +}
> > +#endif /* AF_VSOCK */
> > +
> >  SocketAddress *
> >  socket_sockaddr_to_address(struct sockaddr_storage *sa,
> >                             socklen_t salen,
> > @@ -1037,6 +1253,11 @@ socket_sockaddr_to_address(struct sockaddr_storage *sa,
> >          return socket_sockaddr_to_address_unix(sa, salen, errp);
> >  #endif /* WIN32 */
> > 
> > +#ifdef AF_VSOCK
> > +    case AF_VSOCK:
> > +        return socket_sockaddr_to_address_vsock(sa, salen, errp);
> > +#endif
> > +
> >      default:
> >          error_setg(errp, "socket family %d unsupported",
> >                     sa->ss_family);
> > @@ -1103,6 +1324,12 @@ char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
> >          buf = g_strdup(addr->u.fd.data->str);
> >          break;
> > 
> > +    case SOCKET_ADDRESS_KIND_VSOCK:
> > +        buf = g_strdup_printf("%s:%s",
> > +                              addr->u.vsock.data->cid,
> > +                              addr->u.vsock.data->port);
> > +        break;
> > +
> >      default:
> >          error_setg(errp, "socket family %d unsupported",
> >                     addr->type);
> > -- 
> > 2.7.4
> > 
> 
> 

Re: [Qemu-devel] [PATCH v2 3/4] sockets: add AF_VSOCK support

[Kashyap Chamarthy]

[Reviving a more than 1-year old thread.  I randomly noticed this (refer
below) while browsing through AF_VSOCK changes in my 'qemu-devel'
maildir.]

On Tue, Oct 18, 2016 at 11:21:23AM +0100, Stefan Hajnoczi wrote:
> On Mon, Oct 17, 2016 at 09:25:46AM -0500, Eric Blake wrote:
> > On 10/16/2016 08:35 AM, Stefan Hajnoczi wrote:
> > 
> > >>> +
> > >>> +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
> > >>
> > >> This says stop at the first comma after the colon...
> > >>
> > >>> +        error_setg(errp, "error parsing address '%s'", str);
> > >>> +        return NULL;
> > >>> +    }
> > >>> +    if (str[n] != '\0') {
> > >>> +        error_setg(errp, "trailing characters in address '%s'", str);
> > >>
> > >> ...but this rejects a trailing comma.  Is a trailing comma possible base
> > >> on how QemuOpts work? If so, do you need to handle it here?
> > > 
> > > Actually I just wanted to grab characters up until the end of string.
> > > It wasn't clear from the sscanf(3) man page what the best way to do that
> > > was, so I kept the comma which is also used in tcp addresses (because
> > > they support additional comma-separated options).
> > 
> > %32s instead of %32[^,] should grab up to all 32 remaining characters in
> > the string; your %n trick then ensures there is no garbage.  I guess
> > it's still a question of whether we want to always treat a comma as
> > trailing garbage.
> 
> No comma options are currently accepted.  Treating comma as garbage
> seems okay.
> 
> Mike: Please change %32[^,] to %32s when merging.  Thanks!

The above change didn't take place while merging at that time as part of
v2.7.0-1720-g6a02c80.

I still see %32[^,] in all the sscanf(3) calls in
util/util/qemu-sockets.c

Is it still important to change?

-- 
/kashyap

Re: [Qemu-devel] [PATCH v2 3/4] sockets: add AF_VSOCK support

[Stefan Hajnoczi]

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

On Sun, Nov 05, 2017 at 07:59:59PM +0100, Kashyap Chamarthy wrote:
> [Reviving a more than 1-year old thread.  I randomly noticed this (refer
> below) while browsing through AF_VSOCK changes in my 'qemu-devel'
> maildir.]
> 
> On Tue, Oct 18, 2016 at 11:21:23AM +0100, Stefan Hajnoczi wrote:
> > On Mon, Oct 17, 2016 at 09:25:46AM -0500, Eric Blake wrote:
> > > On 10/16/2016 08:35 AM, Stefan Hajnoczi wrote:
> > > 
> > > >>> +
> > > >>> +    if (sscanf(str, "%32[^:]:%32[^,]%n", cid, port, &n) != 2) {
> > > >>
> > > >> This says stop at the first comma after the colon...
> > > >>
> > > >>> +        error_setg(errp, "error parsing address '%s'", str);
> > > >>> +        return NULL;
> > > >>> +    }
> > > >>> +    if (str[n] != '\0') {
> > > >>> +        error_setg(errp, "trailing characters in address '%s'", str);
> > > >>
> > > >> ...but this rejects a trailing comma.  Is a trailing comma possible base
> > > >> on how QemuOpts work? If so, do you need to handle it here?
> > > > 
> > > > Actually I just wanted to grab characters up until the end of string.
> > > > It wasn't clear from the sscanf(3) man page what the best way to do that
> > > > was, so I kept the comma which is also used in tcp addresses (because
> > > > they support additional comma-separated options).
> > > 
> > > %32s instead of %32[^,] should grab up to all 32 remaining characters in
> > > the string; your %n trick then ensures there is no garbage.  I guess
> > > it's still a question of whether we want to always treat a comma as
> > > trailing garbage.
> > 
> > No comma options are currently accepted.  Treating comma as garbage
> > seems okay.
> > 
> > Mike: Please change %32[^,] to %32s when merging.  Thanks!
> 
> The above change didn't take place while merging at that time as part of
> v2.7.0-1720-g6a02c80.
> 
> I still see %32[^,] in all the sscanf(3) calls in
> util/util/qemu-sockets.c
> 
> Is it still important to change?

Thanks for noticing.  I don't think it's necessary to make this change
now.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]