On 2025-03-14 20:24, Vladimir Sementsov-Ogievskiy wrote:
> On 03.03.25 17:33, Juraj Marcin wrote:
> > 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 <jmar...@redhat.com>
> > ---
> > io/dns-resolver.c | 2 ++
> > qapi/sockets.json | 4 ++--
> > util/qemu-sockets.c | 19 +++++++++++++------
> > 3 files changed, 17 insertions(+), 8 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;
> > 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)
> > #
> > # @mptcp: enable multi-path TCP. (Since 6.1)
> > #
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 77477c1cd5..c30e4ac2ce 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -220,12 +220,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) {
> > @@ -344,6 +338,19 @@ listen_failed:
> > return -1;
> > listen_ok:
> > + if (saddr->keep_alive) {
> > + int val = 1;
> > + int ret = setsockopt(slisten, SOL_SOCKET, SO_KEEPALIVE,
> > + &val, sizeof(val));
> > +
> > + if (ret < 0) {
> > + error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> > + close(slisten);
>
> previous code do the trick to not rewrite errno by close() call. I'm not sure
> it's really needed, but consistency counts. So should do the same here. Or
> drop the logic around errno at all in this function if it's not really needed
> (need to check the callers).
>
> Or this way: just goto listen_failed here. And don't add extra "exit:" label.
> And to make final code more readable, move "listen_failed:" block to the and
> of the function and rename to just "fail:" (better to do it in additional
> preparation patch).
Good point, I will update it. Thank you.
>
> > + slisten = -1;
> > + goto exit;
> > + }
> > + }
> > +exit:
> > freeaddrinfo(res);
> > return slisten;
> > }
>
> --
> Best regards,
> Vladimir
>
>
