[PATCH v2 0/2] util/qemu-sockets: Introduce configurable TCP keep-alive idle period

[PATCH v2 0/2] util/qemu-sockets: Introduce configurable TCP keep-alive idle period

[Juraj Marcin]

This series extends the work introduced by commit aec21d3175 ("qapi: Add
InetSocketAddress member keep-alive") [1]

First, it allows the use of the keep-alive flag for server-side sockets.
Then it introduces a new keep-alive-idle-period setting, which changes
the TCP_KEEPIDLE socket option on platforms that support it (this option
is found in Linux, BSD, and Win32 documentations).

By default, the value of keep-alive-idle-period is 0, which means no
custom socket option value is set.

This is useful, for example, for live migration. In case there is no
traffic from the destination to the source machine during postcopy, the
destination cannot detect a failed connection due to a lack of
non-acknowledged packets and stays in the postcopy-active state until
paused by the management of the QEMU instance.

[1]: https://lore.kernel.org/all/20190725094937.32454-1-vsementsov@virtuozzo.com/

---
V2:
- moved socket options setting into a common function for both server
  and client sockets (suggested by Vladimir)

Juraj Marcin (2):
  util/qemu-sockets: Add support for keep-alive flag to passive sockets
  utils/qemu-sockets: Introduce keep-alive-idle-period inet socket
    option

 io/dns-resolver.c   |  6 ++++
 meson.build         |  2 ++
 qapi/sockets.json   |  9 +++--
 util/qemu-sockets.c | 81 ++++++++++++++++++++++++++++++++-------------
 4 files changed, 73 insertions(+), 25 deletions(-)

-- 
2.48.1

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

[Juraj Marcin]

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.1

[PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option

[Juraj Marcin]

From: Juraj Marcin <jmarcin@redhat.com>

The default idle period for TCP connection could be even 2 hours.
However, in some cases, the application needs to be aware of a
connection issue much sooner.

This is the case, for example, for postcopy live migration. If there is
no traffic from the migration destination guest (server-side) to the
migration source guest (client-side), the destination keeps waiting for
pages indefinitely and does not switch to the postcopy-paused state.
This can happen, for example, if the destination QEMU instance is
started with '-S' command line option and the machine is not started yet
or if the machine is idle and produces no new page faults for
not-yet-migrated pages.

This patch introduces a new inet socket parameter on platforms where
TCP_KEEPIDLE is defined and passes the configured value to the
TCP_KEEPIDLE socket option when a client-side or server-side socket is
created.

The default value is 0, which means system configuration is used, and no
custom value is set.

Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
---
 io/dns-resolver.c   |  4 ++++
 meson.build         |  2 ++
 qapi/sockets.json   |  5 +++++
 util/qemu-sockets.c | 29 +++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+)

diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index ee7403b65b..03c59673f0 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -128,6 +128,10 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
 #endif
             .has_keep_alive = iaddr->has_keep_alive,
             .keep_alive = iaddr->keep_alive,
+#ifdef HAVE_TCP_KEEPIDLE
+            .has_keep_alive_idle_period = iaddr->has_keep_alive_idle_period,
+            .keep_alive_idle_period = iaddr->keep_alive_idle_period,
+#endif
         };
 
         (*addrs)[i] = newaddr;
diff --git a/meson.build b/meson.build
index 41f68d3806..e3440b09c8 100644
--- a/meson.build
+++ b/meson.build
@@ -2734,6 +2734,8 @@ if linux_io_uring.found()
   config_host_data.set('HAVE_IO_URING_PREP_WRITEV2',
                        cc.has_header_symbol('liburing.h', 'io_uring_prep_writev2'))
 endif
+config_host_data.set('HAVE_TCP_KEEPIDLE',
+                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPIDLE'))
 
 # has_member
 config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
diff --git a/qapi/sockets.json b/qapi/sockets.json
index eb50f64e3a..ddd82b1e66 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -59,6 +59,10 @@
 # @keep-alive: enable keep-alive when connecting to/listening on this socket.
 #     (Since 4.2, not supported for listening sockets until 10.0)
 #
+# @keep-alive-idle-period: time in seconds the connection needs to be idle
+#     before sending a keepalive packet.  Only supported for TCP sockets on
+#     systems where TCP_KEEPIDLE socket option is defined.  (Since 10.0)
+#
 # @mptcp: enable multi-path TCP.  (Since 6.1)
 #
 # Since: 1.3
@@ -71,6 +75,7 @@
     '*ipv4': 'bool',
     '*ipv6': 'bool',
     '*keep-alive': 'bool',
+    '*keep-alive-idle-period': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPIDLE' },
     '*mptcp': { 'type': 'bool', 'if': 'HAVE_IPPROTO_MPTCP' } } }
 
 ##
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 99357a4435..23c8a6cc2b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -217,6 +217,19 @@ static int inet_set_sockopts(int sock, InetSocketAddress *saddr, Error **errp)
                              "Unable to set keep-alive option on socket");
             return -1;
         }
+#ifdef HAVE_TCP_KEEPIDLE
+        if (saddr->has_keep_alive_idle_period &&
+            saddr->keep_alive_idle_period) {
+            int keep_idle = saddr->has_keep_alive_idle_period;
+            ret = setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &keep_idle,
+                             sizeof(keep_idle));
+            if (ret < 0) {
+                error_setg_errno(errp, errno,
+                                 "Unable to set TCP keep-alive idle option on socket");
+                return -1;
+            }
+        }
+#endif
     }
     return 0;
 }
@@ -697,6 +710,22 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
         }
         addr->has_keep_alive = true;
     }
+#ifdef HAVE_TCP_KEEPIDLE
+    begin = strstr(optstr, ",keep-alive-idle-period=");
+    if (begin) {
+        begin += strlen(",keep-alive-idle-period=");
+        if (sscanf(begin, "%" PRIu32 "%n", &addr->keep_alive_idle_period, &pos) != 1 ||
+            (begin[pos] != '\0' && begin[pos] != ',')) {
+            error_setg(errp, "error parsing keep-alive-idle-period argument");
+            return -1;
+        }
+        if (addr->keep_alive_idle_period > INT_MAX) {
+            error_setg(errp, "keep-alive-idle-period value is too large");
+            return -1;
+        }
+        addr->has_keep_alive_idle_period = true;
+    }
+#endif
 #ifdef HAVE_IPPROTO_MPTCP
     begin = strstr(optstr, ",mptcp");
     if (begin) {
-- 
2.48.1

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

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option

[Vladimir Sementsov-Ogievskiy]

On 19.03.25 19:36, Juraj Marcin wrote:
> From: Juraj Marcin<jmarcin@redhat.com>
> 
> The default idle period for TCP connection could be even 2 hours.
> However, in some cases, the application needs to be aware of a
> connection issue much sooner.
> 
> This is the case, for example, for postcopy live migration. If there is
> no traffic from the migration destination guest (server-side) to the
> migration source guest (client-side), the destination keeps waiting for
> pages indefinitely and does not switch to the postcopy-paused state.
> This can happen, for example, if the destination QEMU instance is
> started with '-S' command line option and the machine is not started yet
> or if the machine is idle and produces no new page faults for
> not-yet-migrated pages.
> 
> This patch introduces a new inet socket parameter on platforms where
> TCP_KEEPIDLE is defined and passes the configured value to the
> TCP_KEEPIDLE socket option when a client-side or server-side socket is
> created.
> 
> The default value is 0, which means system configuration is used, and no
> custom value is set.
> 
> Signed-off-by: Juraj Marcin<jmarcin@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

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

[Daniel P. Berrangé]

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

Re: [PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option

[Daniel P. Berrangé]

On Wed, Mar 19, 2025 at 05:36:20PM +0100, Juraj Marcin wrote:
> From: Juraj Marcin <jmarcin@redhat.com>
> 
> The default idle period for TCP connection could be even 2 hours.
> However, in some cases, the application needs to be aware of a
> connection issue much sooner.
> 
> This is the case, for example, for postcopy live migration. If there is
> no traffic from the migration destination guest (server-side) to the
> migration source guest (client-side), the destination keeps waiting for
> pages indefinitely and does not switch to the postcopy-paused state.
> This can happen, for example, if the destination QEMU instance is
> started with '-S' command line option and the machine is not started yet
> or if the machine is idle and produces no new page faults for
> not-yet-migrated pages.
> 
> This patch introduces a new inet socket parameter on platforms where
> TCP_KEEPIDLE is defined and passes the configured value to the
> TCP_KEEPIDLE socket option when a client-side or server-side socket is
> created.
> 
> The default value is 0, which means system configuration is used, and no
> custom value is set.
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---
>  io/dns-resolver.c   |  4 ++++
>  meson.build         |  2 ++
>  qapi/sockets.json   |  5 +++++
>  util/qemu-sockets.c | 29 +++++++++++++++++++++++++++++
>  4 files changed, 40 insertions(+)
> 
> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> index ee7403b65b..03c59673f0 100644
> --- a/io/dns-resolver.c
> +++ b/io/dns-resolver.c
> @@ -128,6 +128,10 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
>  #endif
>              .has_keep_alive = iaddr->has_keep_alive,
>              .keep_alive = iaddr->keep_alive,
> +#ifdef HAVE_TCP_KEEPIDLE
> +            .has_keep_alive_idle_period = iaddr->has_keep_alive_idle_period,
> +            .keep_alive_idle_period = iaddr->keep_alive_idle_period,
> +#endif
>          };
>  
>          (*addrs)[i] = newaddr;

I feel like this code is somewhat fragile. It is probably best to add a
commit that refactors it to do a struct copy, and then override the few
fields that need to be different.

     newaddr->u.inet = iaddr;
     newaddr->u.inet.host = g_strdup(uaddr);
     ...

that way we don't risk forgetting to copy fields as fixed in the previous
commit

> diff --git a/meson.build b/meson.build
> index 41f68d3806..e3440b09c8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2734,6 +2734,8 @@ if linux_io_uring.found()
>    config_host_data.set('HAVE_IO_URING_PREP_WRITEV2',
>                         cc.has_header_symbol('liburing.h', 'io_uring_prep_writev2'))
>  endif
> +config_host_data.set('HAVE_TCP_KEEPIDLE',
> +                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPIDLE'))
>  
>  # has_member
>  config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index eb50f64e3a..ddd82b1e66 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -59,6 +59,10 @@
>  # @keep-alive: enable keep-alive when connecting to/listening on this socket.
>  #     (Since 4.2, not supported for listening sockets until 10.0)
>  #
> +# @keep-alive-idle-period: time in seconds the connection needs to be idle
> +#     before sending a keepalive packet.  Only supported for TCP sockets on
> +#     systems where TCP_KEEPIDLE socket option is defined.  (Since 10.0)

There are three tunables for keepalive on TCP sockets:

   TCP_KEEPCNT (since Linux 2.4)
       The maximum number of keepalive probes TCP should send before
       dropping the connection.  This option should not be used in
       code intended to be portable.

   TCP_KEEPIDLE (since Linux 2.4)
       The time (in seconds) the connection needs to remain idle
       before TCP starts sending keepalive probes, if the socket
       option SO_KEEPALIVE has  been  set  on  this socket.  This
       option should not be used in code intended to be portable.

   TCP_KEEPINTVL (since Linux 2.4)
       The time (in seconds) between individual keepalive probes.
       This option should not be used in code intended to be portable.

Shouldn't we be supporting all of these, rather than just a subset ?

> +#
>  # @mptcp: enable multi-path TCP.  (Since 6.1)
>  #
>  # Since: 1.3
> @@ -71,6 +75,7 @@
>      '*ipv4': 'bool',
>      '*ipv6': 'bool',
>      '*keep-alive': 'bool',
> +    '*keep-alive-idle-period': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPIDLE' },
>      '*mptcp': { 'type': 'bool', 'if': 'HAVE_IPPROTO_MPTCP' } } }
>  
>  ##

> @@ -697,6 +710,22 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
>          }
>          addr->has_keep_alive = true;
>      }
> +#ifdef HAVE_TCP_KEEPIDLE
> +    begin = strstr(optstr, ",keep-alive-idle-period=");

A bit too verbose - make it just 'keep-alive-idle='

> +    if (begin) {
> +        begin += strlen(",keep-alive-idle-period=");
> +        if (sscanf(begin, "%" PRIu32 "%n", &addr->keep_alive_idle_period, &pos) != 1 ||
> +            (begin[pos] != '\0' && begin[pos] != ',')) {
> +            error_setg(errp, "error parsing keep-alive-idle-period argument");
> +            return -1;
> +        }
> +        if (addr->keep_alive_idle_period > INT_MAX) {
> +            error_setg(errp, "keep-alive-idle-period value is too large");
> +            return -1;
> +        }
> +        addr->has_keep_alive_idle_period = true;
> +    }
> +#endif
>  #ifdef HAVE_IPPROTO_MPTCP
>      begin = strstr(optstr, ",mptcp");
>      if (begin) {
> -- 
> 2.48.1
> 

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

Re: [PATCH v2 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option

[Juraj Marcin]

On 2025-03-24 11:12, Daniel P. Berrangé wrote:
> On Wed, Mar 19, 2025 at 05:36:20PM +0100, Juraj Marcin wrote:
> > From: Juraj Marcin <jmarcin@redhat.com>
> > 
> > The default idle period for TCP connection could be even 2 hours.
> > However, in some cases, the application needs to be aware of a
> > connection issue much sooner.
> > 
> > This is the case, for example, for postcopy live migration. If there is
> > no traffic from the migration destination guest (server-side) to the
> > migration source guest (client-side), the destination keeps waiting for
> > pages indefinitely and does not switch to the postcopy-paused state.
> > This can happen, for example, if the destination QEMU instance is
> > started with '-S' command line option and the machine is not started yet
> > or if the machine is idle and produces no new page faults for
> > not-yet-migrated pages.
> > 
> > This patch introduces a new inet socket parameter on platforms where
> > TCP_KEEPIDLE is defined and passes the configured value to the
> > TCP_KEEPIDLE socket option when a client-side or server-side socket is
> > created.
> > 
> > The default value is 0, which means system configuration is used, and no
> > custom value is set.
> > 
> > Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> > ---
> >  io/dns-resolver.c   |  4 ++++
> >  meson.build         |  2 ++
> >  qapi/sockets.json   |  5 +++++
> >  util/qemu-sockets.c | 29 +++++++++++++++++++++++++++++
> >  4 files changed, 40 insertions(+)
> > 
> > diff --git a/io/dns-resolver.c b/io/dns-resolver.c
> > index ee7403b65b..03c59673f0 100644
> > --- a/io/dns-resolver.c
> > +++ b/io/dns-resolver.c
> > @@ -128,6 +128,10 @@ static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
> >  #endif
> >              .has_keep_alive = iaddr->has_keep_alive,
> >              .keep_alive = iaddr->keep_alive,
> > +#ifdef HAVE_TCP_KEEPIDLE
> > +            .has_keep_alive_idle_period = iaddr->has_keep_alive_idle_period,
> > +            .keep_alive_idle_period = iaddr->keep_alive_idle_period,
> > +#endif
> >          };
> >  
> >          (*addrs)[i] = newaddr;
> 
> I feel like this code is somewhat fragile. It is probably best to add a
> commit that refactors it to do a struct copy, and then override the few
> fields that need to be different.
> 
>      newaddr->u.inet = iaddr;
>      newaddr->u.inet.host = g_strdup(uaddr);
>      ...
> 
> that way we don't risk forgetting to copy fields as fixed in the previous
> commit
> 
> > diff --git a/meson.build b/meson.build
> > index 41f68d3806..e3440b09c8 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -2734,6 +2734,8 @@ if linux_io_uring.found()
> >    config_host_data.set('HAVE_IO_URING_PREP_WRITEV2',
> >                         cc.has_header_symbol('liburing.h', 'io_uring_prep_writev2'))
> >  endif
> > +config_host_data.set('HAVE_TCP_KEEPIDLE',
> > +                     cc.has_header_symbol('netinet/tcp.h', 'TCP_KEEPIDLE'))
> >  
> >  # has_member
> >  config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
> > diff --git a/qapi/sockets.json b/qapi/sockets.json
> > index eb50f64e3a..ddd82b1e66 100644
> > --- a/qapi/sockets.json
> > +++ b/qapi/sockets.json
> > @@ -59,6 +59,10 @@
> >  # @keep-alive: enable keep-alive when connecting to/listening on this socket.
> >  #     (Since 4.2, not supported for listening sockets until 10.0)
> >  #
> > +# @keep-alive-idle-period: time in seconds the connection needs to be idle
> > +#     before sending a keepalive packet.  Only supported for TCP sockets on
> > +#     systems where TCP_KEEPIDLE socket option is defined.  (Since 10.0)
> 
> There are three tunables for keepalive on TCP sockets:
> 
>    TCP_KEEPCNT (since Linux 2.4)
>        The maximum number of keepalive probes TCP should send before
>        dropping the connection.  This option should not be used in
>        code intended to be portable.
> 
>    TCP_KEEPIDLE (since Linux 2.4)
>        The time (in seconds) the connection needs to remain idle
>        before TCP starts sending keepalive probes, if the socket
>        option SO_KEEPALIVE has  been  set  on  this socket.  This
>        option should not be used in code intended to be portable.
> 
>    TCP_KEEPINTVL (since Linux 2.4)
>        The time (in seconds) between individual keepalive probes.
>        This option should not be used in code intended to be portable.
> 
> Shouldn't we be supporting all of these, rather than just a subset ?

They were not strictly necessary for the Live Migration use case, but I
can also include them in the next version.

Thank you for your feedback, also for the first patch.

Best regards,

Juraj Marcin

> 
> > +#
> >  # @mptcp: enable multi-path TCP.  (Since 6.1)
> >  #
> >  # Since: 1.3
> > @@ -71,6 +75,7 @@
> >      '*ipv4': 'bool',
> >      '*ipv6': 'bool',
> >      '*keep-alive': 'bool',
> > +    '*keep-alive-idle-period': { 'type': 'uint32', 'if': 'HAVE_TCP_KEEPIDLE' },
> >      '*mptcp': { 'type': 'bool', 'if': 'HAVE_IPPROTO_MPTCP' } } }
> >  
> >  ##
> 
> > @@ -697,6 +710,22 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> >          }
> >          addr->has_keep_alive = true;
> >      }
> > +#ifdef HAVE_TCP_KEEPIDLE
> > +    begin = strstr(optstr, ",keep-alive-idle-period=");
> 
> A bit too verbose - make it just 'keep-alive-idle='
> 
> > +    if (begin) {
> > +        begin += strlen(",keep-alive-idle-period=");
> > +        if (sscanf(begin, "%" PRIu32 "%n", &addr->keep_alive_idle_period, &pos) != 1 ||
> > +            (begin[pos] != '\0' && begin[pos] != ',')) {
> > +            error_setg(errp, "error parsing keep-alive-idle-period argument");
> > +            return -1;
> > +        }
> > +        if (addr->keep_alive_idle_period > INT_MAX) {
> > +            error_setg(errp, "keep-alive-idle-period value is too large");
> > +            return -1;
> > +        }
> > +        addr->has_keep_alive_idle_period = true;
> > +    }
> > +#endif
> >  #ifdef HAVE_IPPROTO_MPTCP
> >      begin = strstr(optstr, ",mptcp");
> >      if (begin) {
> > -- 
> > 2.48.1
> > 
> 
> 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 :|
> 

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

[Juraj Marcin]

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