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

[Amos Kong <akong@redhat.com>]

On 14/09/12 02:58, Orit Wasserman wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> refactor address resolution code to fix nonblocking connect
> remove getnameinfo call
>
> 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>

Hi Orit,

Happy new year!

> ---
>   qemu-sockets.c |  144 +++++++++++++++++++++++++++++++------------------------
>   1 files changed, 81 insertions(+), 63 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 361d890..939a453 100644

...

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

trivial comment:

You moved this block to head of inet_connect_addr() in [patch 3/3],
why not do this in this patch?

I know if *in_progress becomes "true", sock is always >= 0, for loop 
will exits.
But moving this block to inet_connect_addr() is clearer.


>       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;
> -        }
> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -        if (!block) {
> -            socket_set_nonblock(sock);
> +        sock = inet_connect_addr(e, block, in_progress);
> +        if (sock >= 0) {
> +            break;
>           }
> -        /* 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;
> -        }
> -        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)
>

-- 
			Amos.