qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] util/qemu-sockets: Introduce configurable TCP keep-alive idle period
@ 2025-03-03 14:33 Juraj Marcin
  2025-03-03 14:33 ` [PATCH 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets Juraj Marcin
  2025-03-03 14:33 ` [PATCH 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option Juraj Marcin
  0 siblings, 2 replies; 6+ messages in thread
From: Juraj Marcin @ 2025-03-03 14:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Juraj Marcin, vsementsov, Daniel P. Berrangé, Paolo Bonzini

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/

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 | 58 ++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 67 insertions(+), 8 deletions(-)

-- 
2.48.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets
  2025-03-03 14:33 [PATCH 0/2] util/qemu-sockets: Introduce configurable TCP keep-alive idle period Juraj Marcin
@ 2025-03-03 14:33 ` Juraj Marcin
  2025-03-14 17:24   ` Vladimir Sementsov-Ogievskiy
  2025-03-03 14:33 ` [PATCH 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option Juraj Marcin
  1 sibling, 1 reply; 6+ messages in thread
From: Juraj Marcin @ 2025-03-03 14:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Juraj Marcin, vsementsov, Daniel P. Berrangé, Paolo Bonzini

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);
+            slisten = -1;
+            goto exit;
+        }
+    }
+exit:
     freeaddrinfo(res);
     return slisten;
 }
-- 
2.48.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option
  2025-03-03 14:33 [PATCH 0/2] util/qemu-sockets: Introduce configurable TCP keep-alive idle period Juraj Marcin
  2025-03-03 14:33 ` [PATCH 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets Juraj Marcin
@ 2025-03-03 14:33 ` Juraj Marcin
  2025-03-14 17:34   ` Vladimir Sementsov-Ogievskiy
  1 sibling, 1 reply; 6+ messages in thread
From: Juraj Marcin @ 2025-03-03 14:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Juraj Marcin, vsementsov, Daniel P. Berrangé, Paolo Bonzini

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 | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 50 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 0ee79c664d..ca726f317f 100644
--- a/meson.build
+++ b/meson.build
@@ -2724,6 +2724,8 @@ config_host_data.set('HAVE_OPTRESET',
                      cc.has_header_symbol('getopt.h', 'optreset'))
 config_host_data.set('HAVE_IPPROTO_MPTCP',
                      cc.has_header_symbol('netinet/in.h', 'IPPROTO_MPTCP'))
+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 c30e4ac2ce..edcda846ef 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -349,6 +349,18 @@ listen_ok:
             slisten = -1;
             goto exit;
         }
+#ifdef HAVE_TCP_KEEPIDLE
+        if (saddr->has_keep_alive_idle_period && saddr->keep_alive_idle_period) {
+            int keepidle = saddr->has_keep_alive_idle_period;
+            ret = setsockopt(slisten, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle, sizeof(keepidle));
+            if (ret < 0) {
+                error_setg_errno(errp, errno, "Unable to set TCP_KEEPIDLE");
+                close(slisten);
+                slisten = -1;
+                goto exit;
+            }
+        }
+#endif
     }
 exit:
     freeaddrinfo(res);
@@ -492,6 +504,17 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
             close(sock);
             return -1;
         }
+#ifdef HAVE_TCP_KEEPIDLE
+        if (saddr->has_keep_alive_idle_period && saddr->keep_alive_idle_period) {
+            int keepidle = saddr->keep_alive_idle_period;
+            ret = setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle, sizeof(keepidle));
+            if (ret < 0) {
+                error_setg_errno(errp, errno, "Unsable to set TCP_KEEPIDLE");
+                close(sock);
+                return -1;
+            }
+        }
+#endif
     }
 
     return sock;
@@ -698,6 +721,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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets
  2025-03-03 14:33 ` [PATCH 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets Juraj Marcin
@ 2025-03-14 17:24   ` Vladimir Sementsov-Ogievskiy
  2025-03-17 14:18     ` Juraj Marcin
  0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-03-14 17:24 UTC (permalink / raw)
  To: Juraj Marcin, qemu-devel
  Cc: vsementsov, Daniel P. Berrangé, Paolo Bonzini

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;
>   }

-- 
Best regards,
Vladimir



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option
  2025-03-03 14:33 ` [PATCH 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option Juraj Marcin
@ 2025-03-14 17:34   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-03-14 17:34 UTC (permalink / raw)
  To: Juraj Marcin, qemu-devel
  Cc: vsementsov, Daniel P. Berrangé, Paolo Bonzini

On 03.03.25 17:33, Juraj Marcin wrote:
> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>   4 files changed, 50 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 0ee79c664d..ca726f317f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2724,6 +2724,8 @@ config_host_data.set('HAVE_OPTRESET',
>                        cc.has_header_symbol('getopt.h', 'optreset'))
>   config_host_data.set('HAVE_IPPROTO_MPTCP',
>                        cc.has_header_symbol('netinet/in.h', 'IPPROTO_MPTCP'))
> +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 c30e4ac2ce..edcda846ef 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -349,6 +349,18 @@ listen_ok:
>               slisten = -1;
>               goto exit;
>           }
> +#ifdef HAVE_TCP_KEEPIDLE
> +        if (saddr->has_keep_alive_idle_period && saddr->keep_alive_idle_period) {
> +            int keepidle = saddr->has_keep_alive_idle_period;
> +            ret = setsockopt(slisten, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle, sizeof(keepidle));
> +            if (ret < 0) {
> +                error_setg_errno(errp, errno, "Unable to set TCP_KEEPIDLE");
> +                close(slisten);
> +                slisten = -1;
> +                goto exit;
> +            }
> +        }
> +#endif
>       }
>   exit:
>       freeaddrinfo(res);
> @@ -492,6 +504,17 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>               close(sock);
>               return -1;
>           }
> +#ifdef HAVE_TCP_KEEPIDLE
> +        if (saddr->has_keep_alive_idle_period && saddr->keep_alive_idle_period) {
> +            int keepidle = saddr->keep_alive_idle_period;
> +            ret = setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &keepidle, sizeof(keepidle));
> +            if (ret < 0) {
> +                error_setg_errno(errp, errno, "Unsable to set TCP_KEEPIDLE");
> +                close(sock);
> +                return -1;
> +            }
> +        }
> +#endif
>       }


Looks like we need some inet_setsockopt(*saddr) {}, which set both options and called from both client and server sockets, to avoid duplication.

>   
>       return sock;
> @@ -698,6 +721,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) {

-- 
Best regards,
Vladimir



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets
  2025-03-14 17:24   ` Vladimir Sementsov-Ogievskiy
@ 2025-03-17 14:18     ` Juraj Marcin
  0 siblings, 0 replies; 6+ messages in thread
From: Juraj Marcin @ 2025-03-17 14:18 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: qemu-devel, Daniel P. Berrangé, Paolo Bonzini

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-03-17 14:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-03 14:33 [PATCH 0/2] util/qemu-sockets: Introduce configurable TCP keep-alive idle period Juraj Marcin
2025-03-03 14:33 ` [PATCH 1/2] util/qemu-sockets: Add support for keep-alive flag to passive sockets Juraj Marcin
2025-03-14 17:24   ` Vladimir Sementsov-Ogievskiy
2025-03-17 14:18     ` Juraj Marcin
2025-03-03 14:33 ` [PATCH 2/2] utils/qemu-sockets: Introduce keep-alive-idle-period inet socket option Juraj Marcin
2025-03-14 17:34   ` Vladimir Sementsov-Ogievskiy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).