Re: [Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Orit Wasserman <owasserm@redhat.com>]

On 09/13/2012 03:35 PM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> From: Michael S. Tsirkin <mst@redhat.com>
>>
>> refactor address resolution code to fix nonblocking connect
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  qemu-sockets.c |  139 +++++++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 82 insertions(+), 57 deletions(-)
>>
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 361d890..68e4d30 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -209,32 +209,25 @@ listen:
>>      return slisten;
>>  }
>>  
>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>  {
>> -    struct addrinfo ai,*res,*e;
>> +    struct addrinfo ai, *res;
>> +    int rc;
>>      const char *addr;
>>      const char *port;
>> -    char uaddr[INET6_ADDRSTRLEN+1];
>> -    char uport[33];
>> -    int sock,rc;
>> -    bool block;
>>  
>>      memset(&ai,0, sizeof(ai));
>>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>>      ai.ai_family = PF_UNSPEC;
>>      ai.ai_socktype = SOCK_STREAM;
>>  
>> -    if (in_progress) {
>> -        *in_progress = false;
>> -    }
>> -
>>      addr = qemu_opt_get(opts, "host");
>>      port = qemu_opt_get(opts, "port");
>> -    block = qemu_opt_get_bool(opts, "block", 0);
>>      if (addr == NULL || port == NULL) {
>> -        fprintf(stderr, "inet_connect: host and/or port not specified\n");
>> +        fprintf(stderr,
>> +                "inet_parse_connect_opts: host and/or port not specified\n");
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -        return -1;
>> +        return NULL;
>>      }
>>  
>>      if (qemu_opt_get_bool(opts, "ipv4", 0))
>> @@ -247,57 +240,89 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>>          fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>>                  gai_strerror(rc));
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -	return -1;
>> +        return NULL;
>>      }
>> +    return res;
>> +}
>>  
>> -    for (e = res; e != NULL; e = e->ai_next) {
>> -        if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
>> -                            uaddr,INET6_ADDRSTRLEN,uport,32,
>> -                            NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
>> -            fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__);
>> -            continue;
>> -        }
>> -        sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
>> -        if (sock < 0) {
>> -            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
>> -            inet_strfamily(e->ai_family), strerror(errno));
>> -            continue;
>> +#ifdef _WIN32
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
>> +#else
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS)
>> +#endif
>> +
>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>> +                             bool *in_progress, Error **errp)
>> +{
>> +    char uaddr[INET6_ADDRSTRLEN + 1];
>> +    char uport[33];
>> +    int sock, rc;
>> +
>> +    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
>> +                    uaddr, INET6_ADDRSTRLEN, uport, 32,
>> +                    NI_NUMERICHOST | NI_NUMERICSERV)) {
>> +        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
>> +        return -1;
>> +    }
> 
> uaddr[] and uport[] are write-only.  Let's drop getnameinfo().
> 
>> +    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>> +    if (sock < 0) {
>> +        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>> +                inet_strfamily(addr->ai_family), strerror(errno));
>> +        return -1;
>> +    }
>> +    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
>> +    if (!block) {
>> +        socket_set_nonblock(sock);
>> +    }
>> +    /* connect to peer */
>> +    do {
>> +        rc = 0;
>> +        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
>> +            rc = -socket_error();
>>          }
>> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -        if (!block) {
>> -            socket_set_nonblock(sock);
>> +    } while (rc == -EINTR);
>> +
>> +    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> +        if (in_progress) {
>> +            *in_progress = true;
>>          }
>> -        /* connect to peer */
>> -        do {
>> -            rc = 0;
>> -            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
>> -                rc = -socket_error();
>> -            }
>> -        } while (rc == -EINTR);
>> -
>> -  #ifdef _WIN32
>> -        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
>> -                       || rc == -WSAEALREADY)) {
>> -  #else
>> -        if (!block && (rc == -EINPROGRESS)) {
>> -  #endif
>> -            if (in_progress) {
>> -                *in_progress = true;
>> -            }
>> -        } else if (rc < 0) {
>> -            if (NULL == e->ai_next)
>> -                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
>> -                        inet_strfamily(e->ai_family),
>> -                        e->ai_canonname, uaddr, uport, strerror(errno));
>> -            closesocket(sock);
>> -            continue;
>> +    } else if (rc < 0) {
>> +        closesocket(sock);
>> +        return -1;
>> +    }
>> +    return sock;
>> +}
>> +
>> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +{
>> +    struct addrinfo *res, *e;
>> +    int sock = -1;
>> +    bool block = qemu_opt_get_bool(opts, "block", 0);
>> +
>> +    res = inet_parse_connect_opts(opts, errp);
>> +    if (!res) {
>> +        return -1;
>> +    }
>> +
>> +    if (in_progress) {
>> +        *in_progress = false;
>> +    }
>> +
>> +    for (e = res; e != NULL; e = e->ai_next) {
>> +        sock = inet_connect_addr(e, block, in_progress, errp);
>> +        if (in_progress && *in_progress) {
>> +            return sock;
> 
> Doesn't this leak res?
Actually it doesn't after patch 3 wait_for_connect is the one freeing res.
is it OK to leave it as is ?

Orit
> 
>> +        } else if (sock >= 0) {
>> +            break;
>>          }
>> -        freeaddrinfo(res);
>> -        return sock;
>>      }
>> -    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>> +    if (sock < 0) {
>> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>> +    }
>>      freeaddrinfo(res);
>> -    return -1;
>> +    return sock;
>>  }
>>  
>>  int inet_dgram_opts(QemuOpts *opts)
> 
> What about
> 
>     for (e = res; e != NULL; e = e->ai_next) {
>         sock = inet_connect_addr(e, block, in_progress, errp);
>         if (sock >= 0) {
>             break;
>         }
>     }
>     if (sock < 0) {
>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>     }
>     freeaddrinfo(res);
>     return sock;
>