Re: [Qemu-devel] [PATCH 1/4] Use getaddrinfo for migration

[Amos Kong]

On 24/02/12 17:08, Kevin Wolf wrote:
> Am 10.02.2012 07:27, schrieb Amos Kong:
> > This allows us to use ipv4/ipv6 for migration addresses.
> > Once there, it also uses /etc/services names (it came free).
> >
> > Signed-off-by: Juan Quintela<address@hidden>
> > Signed-off-by: Amos Kong<address@hidden>
> > ---
> >   migration-tcp.c |   60 ++++++++-----------------------
> >   net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   qemu_socket.h   |    3 ++
> >   3 files changed, 127 insertions(+), 44 deletions(-)
>
> This patch is making too many changes at once:
>
> 1. Move code around
> 2. Remove duplicated code between client and server
> 3. Replace parse_host_port() with an open-coded version

> 4. Modify the open-coded parse_host_port() to use getaddrinfo instead of
> inet_aton/gethostbyname (Why can't we just change parse_host_port? Are
> there valid reasons to use the old implementation? Which?)

tcp_common_start() needs to use the address list which is got by 
getaddrinfo(),

But I agree with verifying host/port by getaddrinfo() in parse_host_port.


> This makes it rather hard to review.
>
> > diff --git a/migration-tcp.c b/migration-tcp.c
> > index 35a5781..644bb8f 100644
> > --- a/migration-tcp.c
> > +++ b/migration-tcp.c
> > @@ -81,43 +81,27 @@ static void tcp_wait_for_connect(void *opaque)
> >
> >   int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
> >   {
> > -    struct sockaddr_in addr;
> >       int ret;
> > -
> > -    ret = parse_host_port(&addr, host_port);
> > -    if (ret<  0) {
> > -        return ret;
> > -    }
> > +    int fd;
> >
> >       s->get_error = socket_errno;
> >       s->write = socket_write;
> >       s->close = tcp_close;
> >
> > -    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> > -    if (s->fd == -1) {
> > -        DPRINTF("Unable to open socket");
> > -        return -socket_error();
> > -    }
> > -
> > -    socket_set_nonblock(s->fd);
> > -
> > -    do {
> > -        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
> > -        if (ret == -1) {
> > -            ret = -socket_error();
> > -        }
> > -        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> > -            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
> > -            return 0;
> > -        }
> > -    } while (ret == -EINTR);
> > -
> > -    if (ret<  0) {
> > +    ret = tcp_client_start(host_port,&fd);
> > +    s->fd = fd;
> > +    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> > +        DPRINTF("connect in progress");
> > +        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
> > +    } else if (ret<  0) {
> >           DPRINTF("connect failed\n");
> > -        migrate_fd_error(s);
> > +        if (ret != -EINVAL) {
> > +            migrate_fd_error(s);
> > +        }
>
> This looks odd. It is probably needed to keep the old behaviour where a
> failed parse_host_port() wouldn't call migrate_fd_error(). Is this
> really required? Maybe I'm missing something, but the caller can't know
> which failure case we had and if migrate_fd_error() has been called.

getaddrinfo() is used in tcp_common_start(), -EINVAL will be returned
when getaddrinfo() doesn't return 0.


> >           return ret;
> > +    } else {
> > +        migrate_fd_connect(s);
> >       }
> > -    migrate_fd_connect(s);
> >       return 0;
> >   }
> >
> > @@ -157,28 +141,16 @@ out2:
> >
> >   int tcp_start_incoming_migration(const char *host_port)
> >   {
> > -    struct sockaddr_in addr;
> > -    int val;
> > +    int ret;
> >       int s;
> >
> >       DPRINTF("Attempting to start an incoming migration\n");
> >
> > -    if (parse_host_port(&addr, host_port)<  0) {
> > -        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
> > -        return -EINVAL;
> > -    }
> > -
> > -    s = qemu_socket(PF_INET, SOCK_STREAM, 0);
> > -    if (s == -1) {
> > -        return -socket_error();
> > +    ret = tcp_server_start(host_port,&s);
> > +    if (ret<  0) {
> > +        return ret;
> >       }
> >
> > -    val = 1;
> > -    setsockopt(s, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
> > -
> > -    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
> > -        goto err;
> > -    }
> >       if (listen(s, 1) == -1) {
> >           goto err;
> >       }
> > diff --git a/net.c b/net.c
> > index c34474f..f63014c 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -99,6 +99,114 @@ static int get_str_sep(char *buf, int buf_size, const char 
> > **pp, int sep)
> >       return 0;
> >   }
> >
> > +static int tcp_server_bind(int fd, struct addrinfo *rp)
> > +{
> > +    int ret;
> > +    int val = 1;
> > +
> > +    /* allow fast reuse */
> > +    setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
> > +
> > +    ret = bind(fd, rp->ai_addr, rp->ai_addrlen);
> > +
> > +    if (ret == -1) {
> > +        ret = -socket_error();
> > +    }
> > +    return ret;
> > +
> > +}
> > +
> > +static int tcp_client_connect(int fd, struct addrinfo *rp)
> > +{
> > +    int ret;
> > +
> > +    do {
> > +        ret = connect(fd, rp->ai_addr, rp->ai_addrlen);
> > +        if (ret == -1) {
> > +            ret = -socket_error();
> > +        }
> > +    } while (ret == -EINTR);
> > +
> > +    return ret;
> > +}
> > +
> > +
> > +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;
> > +    }
>
> Separating host name and port at the first colon looks wrong for IPv6.

I fixed this in [PATCH 3/4] net: split hostname and service by last colon

> > +    if (server&&  strlen(hostname) == 0) {
> > +        name = NULL;
> > +    } else {
> > +        name = hostname;
> > +    }
> > +
> > +    /* 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));
>
> error_report?

yes

> Kevin

--
                        Amos.