Re: [PATCH 1/1] channel: add optional tcp keepalive timeout to channels

[Frediano Ziglio <fziglio@xxxxxxxxxx>]

Hi,

With firewall running between spice server and client, if idle time is larger than firewall session timeout, spice sessions freeze and users lose their keyboard and mouse control.

To workaround this issue, I made a patch to add tcp keepalive timeout to spice server. The timeout can be added to qemu config like below.

    <domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>

      <qemu:commandline>

        <qemu:env name='SPICE_KEEPALIVE_TIMEOUT' value='1800'/>

      </qemu:commandline>

I wanted to add this option to spice client, but there was no setsockopt() option of TCP_KEEPIDLE for windows platform, So, I ended up adding it to spice server. Please review the patch and let me know what you think. Thank you.

For Windows see https://msdn.microsoft.com/en-us/library/windows/desktop/ee470551%28v=vs.85%29.aspx, are used SO_KEEPALIVE and SIO_KEEPALIVE_VALS.

---------------------------------------------------------------------------------------------------

[PATCH 1/1] channel: add optional tcp keepalive timeout to channels

Signed-off-by: Sunny Shin <sunny4s.git@xxxxxxxxx>

---

 server/reds.c | 28 ++++++++++++++++++++++++++++

 1 file changed, 28 insertions(+)

diff --git a/server/reds.c b/server/reds.c

index 8b3c3cb..05d0b1d 100644

--- a/server/reds.c

+++ b/server/reds.c

@@ -2254,6 +2254,9 @@ static RedLinkInfo *reds_init_client_connection(int socket)

     RedLinkInfo *link;

     int delay_val = 1;

     int flags;

+    char *keepalive_timeout_str;

+    int keepalive_timeout;

+    int keepalive = 1;

     if ((flags = fcntl(socket, F_GETFL)) == -1) {

         spice_warning("accept failed, %s", strerror(errno));

@@ -2271,6 +2274,31 @@ static RedLinkInfo *reds_init_client_connection(int socket)

         }

     }

+    keepalive_timeout_str = getenv("SPICE_KEEPALIVE_TIMEOUT");

See Djasa comment, environment are not good here.

+    if (keepalive_timeout_str != NULL) {

+        errno = 0;

+        keepalive_timeout = strtol(keepalive_timeout_str, NULL, 10);

+        if (errno != 0) {

+            spice_warning("error parsing SPICE_KEEPALIVE_TIMEOUT: %s", strerror(errno));

+            goto error;

+        }

+

+        spice_debug("keepalive timeout %ds", keepalive_timeout);

+        if (setsockopt(socket, SOL_SOCKET, SO_KEEPALIVE, &keepalive, sizeof(keepalive)) == -1) {

+            if (errno != ENOTSUP) {

+                spice_printerr("setsockopt for keepalive failed, %s", strerror(errno));

+                goto error;

+            }

+        }

I would just issue a warning on error and keep going (not jump to error), we can live without keepalive

+        if (setsockopt(socket, SOL_TCP, TCP_KEEPIDLE,

+                       &keepalive_timeout, sizeof(keepalive_timeout)) == -1) {

+            if (errno != ENOTSUP) {

+                spice_printerr("setsockopt for keepalive timeout failed, %s", strerror(errno));

+                goto error;

+            }

+        }

same here.

+    }

+

     link = spice_new0(RedLinkInfo, 1);

     link->stream = reds_stream_new(socket);

Frediano

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel