From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEep0-0004ii-DP for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:21:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEeoz-00036Y-6K for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:21:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33488) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEeoy-00036T-Tb for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:21:29 -0400 Message-ID: <505AFC3D.4030806@redhat.com> Date: Thu, 20 Sep 2012 14:21:33 +0300 From: Orit Wasserman MIME-Version: 1.0 References: <1347562697-15411-1-git-send-email-owasserm@redhat.com> <1347562697-15411-2-git-send-email-owasserm@redhat.com> <50598366.9040902@redhat.com> In-Reply-To: <50598366.9040902@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 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: Amos Kong 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 09/19/2012 11:33 AM, Amos Kong wrote: > 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. > sure > >> 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) >> >