Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

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<address@hidden>
> > > > ---
> > > >   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