Re: [Qemu-devel] [PATCH v4 for-4.0 1/7] char-socket: Enable "nowait" option on client sockets

[Yongji Xie]

On Fri, 11 Jan 2019 at 00:41, Daniel P. Berrangé <address@hidden> wrote:
>
> On Thu, Jan 10, 2019 at 10:29:20PM +0800, Yongji Xie wrote:
> > On Thu, 10 Jan 2019 at 22:11, Daniel P. Berrangé <address@hidden> wrote:
> > >
> > > On Thu, Jan 10, 2019 at 10:08:54PM +0800, Yongji Xie wrote:
> > > > On Thu, 10 Jan 2019 at 21:24, Daniel P. Berrangé <address@hidden> wrote:
> > > > >
> > > > > On Thu, Jan 10, 2019 at 09:19:41PM +0800, Yongji Xie wrote:
> > > > > > On Thu, 10 Jan 2019 at 20:50, Daniel P. Berrangé <address@hidden> 
> > > > > > wrote:
> > > > > > >
> > > > > > > On Wed, Jan 09, 2019 at 07:27:22PM +0800, address@hidden wrote:
> > > > > > > > From: Xie Yongji <address@hidden>
> > > > > > > >
> > > > > > > > Enable "nowait" option to make QEMU not do a connect
> > > > > > > > on client sockets during initialization of the chardev.
> > > > > > > > Then we can use qemu_chr_fe_wait_connected() to connect
> > > > > > > > when necessary. Now it would be used for unix domain
> > > > > > > > socket of vhost-user-blk device to support reconnect.
> > > > > > > >
> > > > > > > > Signed-off-by: Xie Yongji <address@hidden>
> > > > > > > > Signed-off-by: Zhang Yu <address@hidden>
> > > > > > > > ---
> > > > > > > >  chardev/char-socket.c | 56 
> > > > > > > > +++++++++++++++++++++----------------------
> > > > > > > >  qapi/char.json        |  3 +--
> > > > > > > >  qemu-options.hx       |  9 ++++---
> > > > > > > >  3 files changed, 35 insertions(+), 33 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > > > > > index eaa8e8b68f..f803f4f7d3 100644
> > > > > > > > --- a/chardev/char-socket.c
> > > > > > > > +++ b/chardev/char-socket.c
> > > > > > > > @@ -1072,37 +1072,37 @@ static void 
> > > > > > > > qmp_chardev_open_socket(Chardev *chr,
> > > > > > > >          s->reconnect_time = reconnect;
> > > > > > > >      }
> > > > > > > >
> > > > > > > > -    if (s->reconnect_time) {
> > > > > > > > -        tcp_chr_connect_async(chr);
> > > > > > > > -    } else {
> > > > > > > > -        if (s->is_listen) {
> > > > > > > > -            char *name;
> > > > > > > > -            s->listener = qio_net_listener_new();
> > > > > > > > +    if (s->is_listen) {
> > > > > > > > +        char *name;
> > > > > > > > +        s->listener = qio_net_listener_new();
> > > > > > > >
> > > > > > > > -            name = g_strdup_printf("chardev-tcp-listener-%s", 
> > > > > > > > chr->label);
> > > > > > > > -            qio_net_listener_set_name(s->listener, name);
> > > > > > > > -            g_free(name);
> > > > > > > > +        name = g_strdup_printf("chardev-tcp-listener-%s", 
> > > > > > > > chr->label);
> > > > > > > > +        qio_net_listener_set_name(s->listener, name);
> > > > > > > > +        g_free(name);
> > > > > > > >
> > > > > > > > -            if (qio_net_listener_open_sync(s->listener, 
> > > > > > > > s->addr, errp) < 0) {
> > > > > > > > -                object_unref(OBJECT(s->listener));
> > > > > > > > -                s->listener = NULL;
> > > > > > > > -                goto error;
> > > > > > > > -            }
> > > > > > > > +        if (qio_net_listener_open_sync(s->listener, s->addr, 
> > > > > > > > errp) < 0) {
> > > > > > > > +            object_unref(OBJECT(s->listener));
> > > > > > > > +            s->listener = NULL;
> > > > > > > > +            goto error;
> > > > > > > > +        }
> > > > > > > >
> > > > > > > > -            qapi_free_SocketAddress(s->addr);
> > > > > > > > -            s->addr = 
> > > > > > > > socket_local_address(s->listener->sioc[0]->fd, errp);
> > > > > > > > -            update_disconnected_filename(s);
> > > > > > > > +        qapi_free_SocketAddress(s->addr);
> > > > > > > > +        s->addr = 
> > > > > > > > socket_local_address(s->listener->sioc[0]->fd, errp);
> > > > > > > > +        update_disconnected_filename(s);
> > > > > > > >
> > > > > > > > -            if (is_waitconnect &&
> > > > > > > > -                qemu_chr_wait_connected(chr, errp) < 0) {
> > > > > > > > -                return;
> > > > > > > > -            }
> > > > > > > > -            if (!s->ioc) {
> > > > > > > > -                
> > > > > > > > qio_net_listener_set_client_func_full(s->listener,
> > > > > > > > -                                                      
> > > > > > > > tcp_chr_accept,
> > > > > > > > -                                                      chr, 
> > > > > > > > NULL,
> > > > > > > > -                                                      
> > > > > > > > chr->gcontext);
> > > > > > > > -            }
> > > > > > > > +        if (is_waitconnect &&
> > > > > > > > +            qemu_chr_wait_connected(chr, errp) < 0) {
> > > > > > > > +            return;
> > > > > > > > +        }
> > > > > > > > +        if (!s->ioc) {
> > > > > > > > +            qio_net_listener_set_client_func_full(s->listener,
> > > > > > > > +                                                  
> > > > > > > > tcp_chr_accept,
> > > > > > > > +                                                  chr, NULL,
> > > > > > > > +                                                  
> > > > > > > > chr->gcontext);
> > > > > > > > +        }
> > > > > > > > +    } else if (is_waitconnect) {
> > > > > > > > +        if (s->reconnect_time) {
> > > > > > > > +            tcp_chr_connect_async(chr);
> > > > > > > >          } else if (qemu_chr_wait_connected(chr, errp) < 0) {
> > > > > > > >              goto error;
> > > > > > > >          }
> > > > > > >
> > > > > > > This skips everything when 'is_waitconnect' is false.
> > > > > > >
> > > > > > > This combines with a bug in tests/libqtest.c which adds the 
> > > > > > > 'nowait'
> > > > > > > flag to the -chardevs it cteates. This mistake was previously 
> > > > > > > ignored
> > > > > > > because the chardevs were socket clients, but now we honour it.
> > > > > > >
> > > > > > > We shoul remove 'nowait' from the qtest chardevs, but separately
> > > > > > > from that this code should also still attempt a non-blocking
> > > > > > > connect when is_waitconnect is false.
> > > > > > >
> > > > > >
> > > > > > Do you mean we still need to connect server in background with
> > > > > > "nowait" option? But my purpose is not to connect server until we
> > > > > > manually call qemu_chr_fe_wait_connected() in other place.
> > > > >
> > > > > I don't see a need to delay the connect. We can start a
> > > > > background connect right away. The later code you have
> > > > > merely needs to wait for that background connect  to
> > > > > finish, which qemu_chr_fe_wait_connected still accomplishes.
> > > > > This keeps the chardev code clearer only having 2 distinct
> > > > > code paths to worry about - blocking or non-blocking connect.
> > > > >
> > > >
> > > > Now the problem is that we have a server that only accept one
> > > > connection. And we want to read something from it during device
> > > > initializtion.
> > > >
> > > > If background connect it before we call qemu_chr_fe_wait_connected()
> > > > during device initializtion, qemu_chr_fe_wait_connected() will
> > > > accomplish but we can't read anything. And we have no way to release
> > > > the background connection. So what I want to do in this patch is to
> > > > disable background connect.
> > >
> > > I'm not seeing the problem here. What I proposed results in
> > >
> > >   1. chardev starts connect()
> >
> > This should be asynchronous with "reconnect" option. Another thread
> > may connect before vhost backend?
> >
> > >   2. vhost backend waits for connect() to complete
> >
> > Sorry, I'm not sure I get your point here. Do you mean vhost backend
> > call qemu_chr_fe_wait_connected()? Seems like
> > qemu_chr_fe_wait_connected() will connect directly rather than wait
> > for connect() to complete?
>
> Ahhhh, yes, you are right.
>
> qemu_chr_fe_wait_connected will potentially cause a second connection to
> be established
>
> Looking at it the qemu_chr_fe_wait_connected() I believe it is seriously
> broken even before this patch series.
>
> The intended usage is that a device can all qemu_chr_fe_wait_connected
> to wait for a new connection to be established, and then do I/O on the
> chardev.  This does not in fact work if TLS, websock or telnet modes
> are enabled for the socket, due to a mistake introduced when we previously
> tried to fix this:
>
>   commit 1dc8a6695c731abb7461c637b2512c3670d82be4
>   Author: Marc-André Lureau <address@hidden>
>   Date:   Tue Aug 16 12:33:32 2016 +0400
>
>     char: fix waiting for TLS and telnet connection
>
> That commit fixed the problem where we continued to accept() new sockets
> when TLS/telnet was enabled, because the 's->connected' flag isn't set
> immediately.
>
> Unfortunately what this means is that when qemu_chr_fe_wait_connected
> returns, the chardev is *not* ready to read/write data. The TLS/telnet
> handshake has not been run, and is still pending in the background.
>
> So we'll end up with device backend trying todo I/O on the chardev
> at the same time as it is trying todo the TLS/telnet handshake.
>
> We need to fix qemu_chr_fe_wait_connected so that it does explicit
> synchronization wrt to any ongoing background connection process.
> It must only return once all TLS/telnet/websock handshakes have
> completed.  If we fix that correctly, then I believe it will  also
> solve the problem you're trying to address.
>

Yes, I think this should be the right way to go. To fix it, my thought
is to track the async QIOChannelSocket in SocketChardev. Then we can
easily get the connection progress in qemu_chr_fe_wait_connected(). Do
you have any suggestion?

Thanks,
Yongji