From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEWaH-0007v8-9T for qemu-devel@nongnu.org; Wed, 19 Sep 2012 22:33:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEWaG-0007XQ-7J for qemu-devel@nongnu.org; Wed, 19 Sep 2012 22:33:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16871) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEWaF-0007XA-VL for qemu-devel@nongnu.org; Wed, 19 Sep 2012 22:33:44 -0400 Message-ID: <505A807F.1020501@redhat.com> Date: Thu, 20 Sep 2012 10:33:35 +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> <50598366.9040902@redhat.com> In-Reply-To: <50598366.9040902@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 19/09/12 16:33, 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 >> --- a/qemu-sockets.c >> +++ b/qemu-sockets.c >> @@ -209,95 +209,113 @@ listen: >> return slisten; >> } >> >> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >> +#ifdef _WIN32 >> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >> + ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY) ^^ Brackets are only be used for first 'rc' >> +#else >> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >> + ((rc) == -EINPROGRESS) >> +#endif > >> + >> +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; In this error path, in_progress is not assigned, but it's used in tcp_start_outgoing_migration() Let also set the default value of in_progress to 'false' in tcp_start_outgoing_migration() >> + } >> + > >> + 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; >> } ... -- Amos.