From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60344) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEepx-0005ew-Db for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:22:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEepo-0003L8-Hp for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:22:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9109) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEepo-0003KA-AZ for qemu-devel@nongnu.org; Thu, 20 Sep 2012 07:22:20 -0400 Message-ID: <505AFC70.6040600@redhat.com> Date: Thu, 20 Sep 2012 14:22:24 +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> <505A807F.1020501@redhat.com> In-Reply-To: <505A807F.1020501@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/20/2012 05:33 AM, Amos Kong wrote: > 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' good catch ! > >>> +#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() > ok. >>> + } >>> + >> >>> + 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; >>> } > > ... >