Re: [PATCH 3/4] io/channel-socket: implement non-blocking connect

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

On Mon, Jul 20, 2020 at 09:07:14PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Utilize new socket API to make a non-blocking connect for inet sockets.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/io/channel-socket.h | 14 +++++++
>  io/channel-socket.c         | 74 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> index 777ff5954e..82e868bc02 100644
> --- a/include/io/channel-socket.h
> +++ b/include/io/channel-socket.h
> @@ -94,6 +94,20 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
>                                      SocketAddress *addr,
>                                      Error **errp);
>  
> +/**
> + * qio_channel_socket_connect_non_blocking_sync:
> + * @ioc: the socket channel object
> + * @addr: the address to connect to
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Attempt to connect to the address @addr using non-blocking mode of
> + * the socket. Function is synchronous, but being called from
> + * coroutine context will yield during connect operation.
> + */
> +int qio_channel_socket_connect_non_blocking_sync(QIOChannelSocket *ioc,
> +                                                 SocketAddress *addr,
> +                                                 Error **errp);
> +
>  /**
>   * qio_channel_socket_connect_async:
>   * @ioc: the socket channel object
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index e1b4667087..076de7578a 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -22,6 +22,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-visit-sockets.h"
>  #include "qemu/module.h"
> +#include "qemu/sockets.h"
>  #include "io/channel-socket.h"
>  #include "io/channel-watch.h"
>  #include "trace.h"
> @@ -29,6 +30,8 @@
>  
>  #define SOCKET_MAX_FDS 16
>  
> +static int qio_channel_socket_close(QIOChannel *ioc, Error **errp);
> +
>  SocketAddress *
>  qio_channel_socket_get_local_address(QIOChannelSocket *ioc,
>                                       Error **errp)
> @@ -157,6 +160,77 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
>      return 0;
>  }
>  
> +static int qio_channel_inet_connect_non_blocking_sync(QIOChannelSocket *ioc,
> +        InetSocketAddress *addr, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    struct addrinfo *infos, *info;
> +    int sock = -1;
> +
> +    infos = inet_parse_connect_saddr(addr, errp);
> +    if (!infos) {
> +        return -1;
> +    }

This call is blocking since it calls getaddrinfo whose design
offers no ability todo non-blocking DNS lookups. Given this
call, ...

> +
> +    for (info = infos; info != NULL; info = info->ai_next) {
> +        bool in_progress;
> +
> +        error_free(local_err);
> +        local_err = NULL;
> +
> +        sock = inet_connect_addr(addr, info, false, &in_progress, &local_err);
> +        if (sock < 0) {
> +            continue;
> +        }
> +
> +        if (qio_channel_socket_set_fd(ioc, sock, &local_err) < 0) {
> +            close(sock);
> +            continue;
> +        }
> +
> +        if (in_progress) {
> +            if (qemu_in_coroutine()) {
> +                qio_channel_yield(QIO_CHANNEL(ioc), G_IO_OUT);
> +            } else {
> +                qio_channel_wait(QIO_CHANNEL(ioc), G_IO_OUT);
> +            }

...this is offering false assurances of being non-blocking.

If we don't want the current thread to be blocked then we
need to be using the existing qio_channel_socket_connect_async
method or similar. It uses a throw away background thread to
run the connection attempt, and then reports completion back
later, thus avoiding the getaddrinfo design flaw for the callers.

I explicitly didn't want to add an method like the impl in this
patch, because getaddrinfo dooms it and we already had bugs in
the pre-QIOChannel code where QEMU thought it was non-blocking
but wasn't due to getaddrinfo lookups.


IIUC, the main appeal of this method is that the non-blocking
nature is hidden from the caller who can continue to treat it
as a synchronous call and have the coroutine magic happen in
behind the scenes.

IOW, What's needed is a simple way to run the operation in a
thread, and sleep for completion while having the coroutine
yield.

I think this could likely be achieved with QIOTask with an
alternate impl of the qio_task_wait_thread() method that is
friendly to coroutines instead of being based on pthread
condition variable waits.


> +            if (socket_check(sock, &local_err) < 0) {
> +                qio_channel_socket_close(QIO_CHANNEL(ioc), NULL);
> +                continue;
> +            }
> +        }
> +
> +        break;
> +    }
> +
> +    freeaddrinfo(infos);
> +
> +    error_propagate(errp, local_err);
> +    return sock;
> +}

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