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;
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..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;
+ }
+ } else {
+ if (inet_set_sockopts(slisten, saddr, errp)) {
+ goto listen_failed;
+ }
goto listen_ok;
}
- 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;
}
-
return sock;
}
--
2.48.1On 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 :|
Hi Daniel,
On 2025-03-24 10:30, Daniel P. Berrangé wrote:
> 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.
Actually, it is not a success code path. Regardless of how 'listen()'
fails, it is still a failure. The only difference is that with
'EADDRINUSE' it does not fail completely, but it continues with
'close()' at the end of the loop and tries the next port. This is the
same behaviour as before:
rc = try_bind(slisten, saddr, e);
if (rc < 0) {
if (errno != EADDRINUSE) {
error_setg_errno(errp, errno, "Failed to bind socket");
/*** don't try next ports, just fail ***/
goto listen_failed;
}
/*** close this one and try the next port ***/
} else {
if (!listen(slisten, num)) {
/*** the only success path ***/
goto listen_ok;
}
if (errno != EADDRINUSE) {
error_setg_errno(errp, errno, "Failed to listen on socket");
/*** don't try next ports, just fail ***/
goto listen_failed;
}
/*** close this one and try the next port ***/
}
/* Someone else managed to bind to the same port and beat us
* to listen on it! Socket semantics does not allow us to
* recover from this situation, so we need to recreate the
* socket to allow bind attempts for subsequent ports:
*/
close(slisten);
slisten = -1;
This loop probably deserves some more refactoring before modifying.
Best regards,
Juraj Marcin
>
> > + } 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 :|
>
On 19.03.25 19:36, 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> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> -- Best regards, Vladimir
© 2016 - 2025 Red Hat, Inc.