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

[Yongji Xie <elohimes@gmail.com>]

On Thu, 10 Jan 2019 at 20:50, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jan 09, 2019 at 07:27:22PM +0800, elohimes@gmail.com wrote:
> > From: Xie Yongji <xieyongji@baidu.com>
> >
> > 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 <xieyongji@baidu.com>
> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > ---
> >  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.

> ie
>
>     } else if (is_waitconnect) {
>         if (s->reconnect_time || !is_waitconnect) {

"!is_waitconnect" should always be false here?

Thanks,
Yongji