Patchwork [v2,1/3] Refactor inet_connect_opts function

login
register
mail settings
Submitter Orit Wasserman
Date Sept. 12, 2012, 11:12 a.m.
Message ID <1347448378-23915-2-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/183320/
State New
Headers show

Comments

Orit Wasserman - Sept. 12, 2012, 11:12 a.m.
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(-)
Markus Armbruster - Sept. 13, 2012, 12:35 p.m.
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?

> +        } 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;
Markus Armbruster - Sept. 13, 2012, 1:14 p.m.
One more...

Orit Wasserman <owasserm@redhat.com> writes:

[...]
> +static int inet_connect_addr(struct addrinfo *addr, bool block,
> +                             bool *in_progress, Error **errp)

Parameter errp is unused.

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

Necessary, because inet_connect_addr() doesn't do it.

Suggest to drop inet_connect_addr() parameter errp.

> +    }
>      freeaddrinfo(res);
> -    return -1;
> +    return sock;
>  }
>  
>  int inet_dgram_opts(QemuOpts *opts)
Orit Wasserman - Sept. 13, 2012, 4:44 p.m.
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().
sure
> 
>> +    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?
right, will fix it.
> 
>> +        } 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;
> 
looks good.
Orit Wasserman - Sept. 13, 2012, 4:44 p.m.
On 09/13/2012 04:14 PM, Markus Armbruster wrote:
> One more...
> 
> Orit Wasserman <owasserm@redhat.com> writes:
> 
> [...]
>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>> +                             bool *in_progress, Error **errp)
> 
> Parameter errp is unused.
> 
>> +{
>> +    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;
>> +    }
>> +    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;
>> +        } 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);
> 
> Necessary, because inet_connect_addr() doesn't do it.
> 
> Suggest to drop inet_connect_addr() parameter errp.
> 
no objections here
>> +    }
>>      freeaddrinfo(res);
>> -    return -1;
>> +    return sock;
>>  }
>>  
>>  int inet_dgram_opts(QemuOpts *opts)
Orit Wasserman - Sept. 13, 2012, 4:52 p.m.
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;
>
Michael S. Tsirkin - Sept. 14, 2012, 7:37 a.m.
On Thu, Sep 13, 2012 at 07:52:35PM +0300, Orit Wasserman wrote:
> >> +    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

We can't avoid breaking bisect sometimes but let's not do this intentionally.

Patch

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