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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

ie

    } else if (is_waitconnect) {
        if (s->reconnect_time || !is_waitconnect) {
            tcp_chr_connect_async(chr);
        } else if (qemu_chr_wait_connected(chr, errp) < 0) {
            goto error;
        }
    }


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