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: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()
  2. vhost backend waits for connect() to complete
  3. vhost backend reads from chardev

vs the flow

  1. vhost backend starts connect()
  2. vhost backend waits for connect() to complete
  3. vhost backend reads from chardev

in both cases there's only a single connection established to the
server

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 :|