Re: [PATCH v2 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets

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

On Wed, Mar 19, 2025 at 05:36:19PM +0100, Juraj Marcin wrote:
> From: Juraj Marcin <jmarcin@redhat.com>
> 
> Commit aec21d3175 (qapi: Add InetSocketAddress member keep-alive)
> introduces the keep-alive flag, which enables the SO_KEEPALIVE socket
> option, but only on client-side sockets. However, this option is also
> useful for server-side sockets, so they can check if a client is still
> reachable or drop the connection otherwise.
> 
> This patch enables the SO_KEEPALIVE socket option on passive server-side
> sockets if the keep-alive flag is enabled. This socket option is then
> inherited by active server-side sockets communicating with connected
> clients.
> 
> This patch also fixes an issue in 'qio_dns_resolver_lookup_sync_inet()'
> where the keep-alive flag is not copied together with other attributes.
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---
>  io/dns-resolver.c   |  2 ++
>  qapi/sockets.json   |  4 ++--
>  util/qemu-sockets.c | 52 +++++++++++++++++++++++++--------------------
>  3 files changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> index 53b0e8407a..ee7403b65b 100644
> --- a/io/dns-resolver.c
> +++ b/io/dns-resolver.c
> @@ -126,6 +126,8 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
>              .has_mptcp = iaddr->has_mptcp,
>              .mptcp = iaddr->mptcp,
>  #endif
> +            .has_keep_alive = iaddr->has_keep_alive,
> +            .keep_alive = iaddr->keep_alive,
>          };
>  
>          (*addrs)[i] = newaddr;

This is a bugfix, so should be a separate commit to facilitate cherry
picking back to stable branches.

> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index 6a95023315..eb50f64e3a 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -56,8 +56,8 @@
>  # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and
>  #     IPv6
>  #
> -# @keep-alive: enable keep-alive when connecting to this socket.  Not
> -#     supported for passive sockets.  (Since 4.2)
> +# @keep-alive: enable keep-alive when connecting to/listening on this socket.
> +#     (Since 4.2, not supported for listening sockets until 10.0)

This needs to be '10.1', since we're past feature freeze for '10.0'

>  #
>  # @mptcp: enable multi-path TCP.  (Since 6.1)
>  #
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 77477c1cd5..99357a4435 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -205,6 +205,22 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e)
>  #endif
>  }
>  
> +static int inet_set_sockopts(int sock, InetSocketAddress *saddr, Error **errp)
> +{
> +    if (saddr->keep_alive) {
> +        int keep_alive = 1;
> +        int ret = setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> +                             &keep_alive, sizeof(keep_alive));
> +
> +        if (ret < 0) {
> +            error_setg_errno(errp, errno,
> +                             "Unable to set keep-alive option on socket");
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static int inet_listen_saddr(InetSocketAddress *saddr,
>                               int port_offset,
>                               int num,
> @@ -220,12 +236,6 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>      int saved_errno = 0;
>      bool socket_created = false;
>  
> -    if (saddr->keep_alive) {
> -        error_setg(errp, "keep-alive option is not supported for passive "
> -                   "sockets");
> -        return -1;
> -    }
> -
>      memset(&ai,0, sizeof(ai));
>      ai.ai_flags = AI_PASSIVE;
>      if (saddr->has_numeric && saddr->numeric) {
> @@ -313,13 +323,18 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>                      goto listen_failed;
>                  }
>              } else {
> -                if (!listen(slisten, num)) {
> +                if (listen(slisten, num)) {
> +                    if (errno != EADDRINUSE) {
> +                        error_setg_errno(errp, errno,
> +                                         "Failed to listen on socket");
> +                        goto listen_failed;
> +                    }

The implicit '} else {' fallthough is a success code path,
but you're failing to set socket opts in that case. We
accept that the socket might already be listening, but
we must still ensure the keepalives are set.

> +                } else {
> +                    if (inet_set_sockopts(slisten, saddr, errp)) {
> +                        goto listen_failed;
> +                    }
>                      goto listen_ok;
>                  }

Stylewise, don't nest the success codepath - this also would
fix the comment above.

> -                if (errno != EADDRINUSE) {
> -                    error_setg_errno(errp, errno, "Failed to listen on socket");
> -                    goto listen_failed;
> -                }
>              }
>              /* Someone else managed to bind to the same port and beat us
>               * to listen on it! Socket semantics does not allow us to
> @@ -474,19 +489,10 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>          error_propagate(errp, local_err);
>          return sock;
>      }
> -
> -    if (saddr->keep_alive) {
> -        int val = 1;
> -        int ret = setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> -                             &val, sizeof(val));
> -
> -        if (ret < 0) {
> -            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> -            close(sock);
> -            return -1;
> -        }
> +    if (inet_set_sockopts(sock, saddr, errp)) {
> +        close(sock);
> +        return -1;
>      }

This is the refactoring of existing code into a separate function. This
can be done in a separate commit, then the new feature of adding it to
the listener socket codepath  can be a followon commit.


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