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

[Eric Blake]

On 10/06/2016 11:40 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 seasy to convert between struct sockaddr_vm

s/seasy/easy/

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

At any rate, it seems like SocketAddress would be a better fit for a
tri-state union between InetSocketAddress, UnixSocketAddress, and
VnetSocketAddress.

> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  qapi-schema.json    |  23 +++++-
>  util/qemu-sockets.c | 222 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 244 insertions(+), 1 deletion(-)
> 

> +##
>  # @SocketAddress
>  #
>  # Captures the address of a socket, which could also be a named file 
> descriptor
> @@ -3027,6 +3047,7 @@
>    'data': {
>      'inet': 'InetSocketAddress',
>      'unix': 'UnixSocketAddress',
> +    'vsock': 'VsockSocketAddress',
>      'fd': 'String' } }

Which is in fact what you did.


> +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);

Isn't the presence of vsock support sufficient to prove that we have
SOCK_NONBLOCK support as part of our socket() call?  In which case,
wouldn't it be better to pass that option up front to atomically get a
non-blocking socket, rather than having to change its state after the fact?


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

Would it be a wise idea to also use %n to ensure that you aren't
ignoring trailing garbage?

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

signature.asc
Description: OpenPGP digital signature