| Message ID | 20250303143312.640909-2-jmarcin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | util/qemu-sockets: Introduce configurable TCP keep-alive idle period | expand |
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 <jmarcin@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). > + slisten = -1; > + goto exit; > + } > + } > +exit: > freeaddrinfo(res); > return slisten; > }
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 <jmarcin@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 > >
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); + slisten = -1; + goto exit; + } + } +exit: freeaddrinfo(res); return slisten; }
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 | 19 +++++++++++++------ 3 files changed, 17 insertions(+), 8 deletions(-)