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

[Daniel P . Berrangé]

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.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|