From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34436) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEg1i-0003Iy-O0 for qemu-devel@nongnu.org; Thu, 20 Sep 2012 08:38:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEg1a-0007q7-Qf for qemu-devel@nongnu.org; Thu, 20 Sep 2012 08:38:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20890) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEg1a-0007py-IS for qemu-devel@nongnu.org; Thu, 20 Sep 2012 08:38:34 -0400 From: Markus Armbruster 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> Date: Thu, 20 Sep 2012 14:38:30 +0200 In-Reply-To: <505A807F.1020501@redhat.com> (Amos Kong's message of "Thu, 20 Sep 2012 10:33:35 +0800") Message-ID: <87d31g3m49.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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, quintela@redhat.com, mst@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, Orit Wasserman , pbonzini@redhat.com, lcapitulino@redhat.com Amos Kong writes: > 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() Took me a minute to see what you mean. inet_connect_opts() is called by inet_connect(), which is called by tcp_start_outgoing_migration(), which uses in_progress. The other callers of inet_connect() and inet_connect_opts() aren't affected, because they pass null in_progress. > Let also set the default value of in_progress to 'false' in > tcp_start_outgoing_migration() No, let's make sure the functions that take an in_progress argument always set it. That's easier to use safely. >>> + } >>> + >> >>> + 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. Makes inet_connect_addr() always set in_progress, which I like. [...]