From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34894) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEFjL-00068W-GX for qemu-devel@nongnu.org; Wed, 19 Sep 2012 04:34:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEFjH-0005aF-Az for qemu-devel@nongnu.org; Wed, 19 Sep 2012 04:33:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5065) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEFjH-0005a4-1y for qemu-devel@nongnu.org; Wed, 19 Sep 2012 04:33:55 -0400 Message-ID: <50598366.9040902@redhat.com> Date: Wed, 19 Sep 2012 16:33:42 +0800 From: Amos Kong MIME-Version: 1.0 References: <1347562697-15411-1-git-send-email-owasserm@redhat.com> <1347562697-15411-2-git-send-email-owasserm@redhat.com> In-Reply-To: <1347562697-15411-2-git-send-email-owasserm@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/3] Refactor inet_connect_opts function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Orit Wasserman Cc: kwolf@redhat.com, aliguori@us.ibm.com, mdroth@linux.vnet.ibm.com, mst@redhat.com, quintela@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, lcapitulino@redhat.com On 14/09/12 02:58, Orit Wasserman wrote: > From: "Michael S. Tsirkin" > > refactor address resolution code to fix nonblocking connect > remove getnameinfo call > > Signed-off-by: Michael S. Tsirkin > Signed-off-by: Amos Kong > Signed-off-by: Orit Wasserman 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.