From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8Uhb-0005Mz-Uk for qemu-devel@nongnu.org; Fri, 16 Mar 2012 06:48:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S8UhV-0000eL-IB for qemu-devel@nongnu.org; Fri, 16 Mar 2012 06:48:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24283) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S8UhV-0000eB-9c for qemu-devel@nongnu.org; Fri, 16 Mar 2012 06:48:01 -0400 Message-ID: <4F631A5B.6050206@redhat.com> Date: Fri, 16 Mar 2012 18:47:55 +0800 From: Amos Kong MIME-Version: 1.0 References: <20120306224330.24264.9494.stgit@dhcp-8-167.nay.redhat.com> <20120306224745.24264.19990.stgit@dhcp-8-167.nay.redhat.com> <20120313163907.GB3699@illuin> <4F6057CA.4010302@redhat.com> <20120314145836.GA2894@illuin> In-Reply-To: <20120314145836.GA2894@illuin> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: aliguori@us.ibm.com, kvm@vger.kernel.org, quintela@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, owasserm@redhat.com, laine@redhat.com On 14/03/12 22:58, Michael Roth wrote: > On Wed, Mar 14, 2012 at 04:33:14PM +0800, Amos Kong wrote: >> On 14/03/12 00:39, Michael Roth wrote: >>> On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote: >>>> Introduce tcp_server_start() by moving original code in >>>> tcp_start_incoming_migration(). >>>> >>>> Signed-off-by: Amos Kong >>>> --- >>>> net.c | 28 ++++++++++++++++++++++++++++ >>>> qemu_socket.h | 2 ++ >>>> 2 files changed, 30 insertions(+), 0 deletions(-) >>>> >>>> +int tcp_server_start(const char *str, int *fd) >>>> +{ >>> I would combine this with patch 2, since it provides context for why >>> this function is being added. Would also do the same for 3 and 4. >>> >>> I see client the client implementation you need to pass fd back by >>> reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success, >> >> ret restores 0 or -socket_error() >> success: 0, -EINPROGRESS >> fail : ret< 0&& ret !=-EINTR&& ret != -EWOULDBLOCK >> >> , it should be -EINPROGRESS > > I see, I think I was confued by patch #4 where you do a > > + ret = tcp_client_start(host_port,&s->fd); > + if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) { > + DPRINTF("connect in progress"); > + qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s); > > If ret == EWOULDBLOCK is a failure (or if the call isn't supposed to > return EWOULDBLOCK), we should fail it there rather than passing it on to > tcp_wait_for_connect(). You are right, it should be : if (ret == -EINPROGRESS) { >>> Also, is there any reason we can't re-use >>> qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they >>> serve the same purpose, and already include some of the work from your >>> PATCH #6. >> >> We could not directly use it, there are some difference, >> such as tcp_start_incoming_migration() doesn't set socket no-blocked, >> but net_socket_listen_init() sets socket no-blocked. > > I think adding a common function with blocking/non-blocking flag and > having inet_listen_opts()/socket_listen_opts() call it with a wrapper > would be reasonable. > A lot of code is being introduced here to solve problems that are > already handled in qemu-sockets.c. inet_listen()/inet_connect() > already handles backeted-enclosed ipv6 addrs, getting port numbers when > there's more than one colon, getaddrinfo()-based connections, and most > importantly it's had ipv6 support from day 1. > Not 100% sure it'll work for what you're doing, but qemu-sockets.c was > specifically added for this type of use-case and is heavilly used > currently (vnc, nbd, Chardev users), so I think we should use it unless > there's a good reason not to. There are many special request for migration, which is not implemented in inet_listen_opts()/socket_listen_opts(), but many codes can be reused, I would re-write patches. Thanks, Amos