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

[Kevin Wolf <kwolf@redhat.com>]

Am 02.03.2012 04:33, schrieb 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<quintela@redhat.com>
>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>> ---
>>>   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.

The new parse_host_port() could return an address list. If it's the
right thing to use it here, it's probably right to use it in all other
places as well.

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

This is an implementation detail of tcp_common_start(). What does
-EINVAL mean at the tcp_client_start() level? Should it be documented
with a comment?

Why must migrate_fd_error() not be called when getaddrinfo() failed?

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

Noticed it later. That's why I suggested reordering the patches in my
reply to PATCH 0/4.

Kevin