Re: [Qemu-devel] IPv6 support for TCP migrations

[Nicholas Thomas]

On Wed, 2011-05-04 at 11:13 +0100, Daniel P. Berrange wrote:
> On Wed, May 04, 2011 at 09:39:02AM +0100, address@hidden wrote:
> > Hi,
> > 
> > Currently migration-tcp.c uses the IPv4-only socket functions, making
> > migrations over IPv6 impossible. Following is a tentative patch that 
> > switches
> > it to use inet_connect() and inet_listen(). 
> > 
> > However, the patch loses the non-blocking connect() behaviour seen with the
> > previous code. I'm not sure how much of an issue this is - if connect() 
> > blocks
> > here, does it block execution of the VM? 
> > 
> > If so, I guess we need a non-blocking form of inet_connect(), or some way of
> > replicating the behaviour - it would potentially be needed for my NBD
> > reconnection patches too? I can see that a blocking connect() might not be 
> > an
> > issue while the KVM process is starting up, but could cause problems if we
> > try to reconnect while emulation is ongoing.
> > 
> > Thoughts?
> 
> FWIW, Juan Quintela also posted a set of patches to add IPv6 support for
> migration a few weeks back, but unfortunately they don't appear to have
> been merged yet:
> 
>   http://www.mail-archive.com/address@hidden/msg58954.html
> 
> IIUC, Juan's patches don't have the blocking connect() problem you
> mention.

Hi,

Those patches look closer than mine, yes, although I spotted a problem
in tcp_start_common() of that patch series (Juan CC'd):

> +static int tcp_start_common(const char *str, int *fd, bool server)
> +{
> +    char hostname[512];
> +    const char *service;
> +    const char *name;
> +    struct addrinfo hints;
> +    struct addrinfo *result, *rp;
> +    int s;
> +    int sfd;
> +    int ret = -EINVAL;
> +
> +    *fd = -1;
> +    service = str;
> +    if (get_str_sep(hostname, sizeof(hostname), &service, ':') < 0) {
> +        return -EINVAL;
> +    }
> +    if (server && strlen(hostname) == 0) {
> +        name = NULL;
> +    } else {
> +        name = hostname;
> +    }

I think this will fail when specifying an IPv6 *address* and port, e.g:

migrate -d [::1]:5000

We already have code that does this correctly in qemu-sockets.c - 
inet_parse() - which is primarily what I was trying to get the
benefit of, in my patch. 

> +
> +    /* Obtain address(es) matching host/port */
> +
> +    memset(&hints, 0, sizeof(struct addrinfo));
> +    hints.ai_family = AF_UNSPEC;     /* Allow IPv4 or IPv6 */
> +    hints.ai_socktype = SOCK_STREAM; /* Datagram socket */
> +
> +    if (server) {
> +        hints.ai_flags = AI_PASSIVE;
> +    }
> +
> +    s = getaddrinfo(name, service, &hints, &result);
> +    if (s != 0) {
> +        fprintf(stderr, "qemu: getaddrinfo: %s\n", gai_strerror(s));
> +        return -EINVAL;
> +    }
> +
> +    /* getaddrinfo() returns a list of address structures.
> +       Try each address until we successfully bind/connect).
> +       If socket(2) (or bind/connect(2)) fails, we (close the socket
> +       and) try the next address. */
> +
> +    for (rp = result; rp != NULL; rp = rp->ai_next) {
> +        sfd = qemu_socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol);
> +        if (sfd == -1) {
> +            ret = -errno;
> +            continue;
> +        }
> +        socket_set_nonblock(sfd);
> +        if (server) {
> +            ret = tcp_server_bind(sfd, rp);
> +        } else {
> +            ret = tcp_client_connect(sfd, rp);
> +        }
> +        if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> +            *fd = sfd;
> +            break;                  /* Success */
> +        }
> +        close(sfd);
> +    }
> +
> +    freeaddrinfo(result);
> +    return ret;
> +}