[Spice-devel] [PATCH usbredirserver] usbredirserver : enable TCP keepalive

[Frediano Ziglio]

> 
> In some bad cases, for example, host OS crashes without
> sending any FIN to usbredirserver, and usbredirserver
> will keep idle connection for a long time.
> 
> We can also set the kernel arguments, it means that other
> processes may be affected.
> 
> Setting a sensible timeout(like 10 minutes) seems good.
> But QEMU is restarted after host OS crashes, it usually
> needs to reconnect usbredirserver within 2 minutes.
> 
> So, add cmdline argument '-k' and "--keepalive" for
> usbredirserver to set tcp keepalive idle time.
> 
> If setting TCP keepalive fails with errno ENOTSUP, ignore
> the specific error.
> 
> Signed-off-by: zhenwei.pi <zhenwei.pi at youruncloud.com>

I think I got completely wrong the problem.
So this patch is trying to use tcp keepalives to detect disconnection
making possible new connections. The problem is that the server
stops accepting connections when it handle one client and return to
accepting new connections when it finishes servicing the client.
This as the usb device usage is exclusive.
I can see that in the current protocol there's no authentication
so all are filtered at tcp/firewall levels.
I suppose being usb redirection that the expected latency is relatively
low.
The disconnection is detected when writing, as the other side is
not replying with acks (or if the machine became available replies
with some ICMP notifications) so the usb server must be stuck reading
from the client.
Would not better instead of having to tweak the time by hand to
detect new connection attempts and then try to detect disconnections
or either stop serving the current client?

Patch looks good otherwise.

> ---
>  usbredirserver/usbredirserver.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/usbredirserver/usbredirserver.c
> b/usbredirserver/usbredirserver.c
> index 5575181..3a7620a 100644
> --- a/usbredirserver/usbredirserver.c
> +++ b/usbredirserver/usbredirserver.c
> @@ -37,6 +37,7 @@
>  #include <netdb.h>
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
> +#include <netinet/tcp.h>
>  #include "usbredirhost.h"
>  
>  
> @@ -52,6 +53,7 @@ static const struct option longopts[] = {
>      { "verbose", required_argument, NULL, 'v' },
>      { "ipv4", required_argument, NULL, '4' },
>      { "ipv6", required_argument, NULL, '6' },
> +    { "keepalive", required_argument, NULL, 'k' },
>      { "help", no_argument, NULL, 'h' },
>      { NULL, 0, NULL, 0 }
>  };
> @@ -98,6 +100,7 @@ static void usage(int exit_code, char *argv0)
>      fprintf(exit_code? stderr:stdout,
>          "Usage: %s [-p|--port <port>] [-v|--verbose <0-5>] "
>          "[[-4|--ipv4 ipaddr]|[-6|--ipv6 ipaddr]] "
> +        "[-k|--keepalive time] "
>          "<busnum-devnum|vendorid:prodid>\n",
>          argv0);
>      exit(exit_code);
> @@ -203,6 +206,7 @@ int main(int argc, char *argv[])
>      int usbvendor  = -1;
>      int usbproduct = -1;
>      int on = 1;
> +    int keepalive  = -1;
>      char *ipv4_addr = NULL, *ipv6_addr = NULL;
>      union {
>          struct sockaddr_in v4;
> @@ -211,7 +215,7 @@ int main(int argc, char *argv[])
>      struct sigaction act;
>      libusb_device_handle *handle = NULL;
>  
> -    while ((o = getopt_long(argc, argv, "hp:v:4:6:", longopts, NULL)) != -1)
> {
> +    while ((o = getopt_long(argc, argv, "hp:v:4:6:k:", longopts, NULL)) !=
> -1) {
>          switch (o) {
>          case 'p':
>              port = strtol(optarg, &endptr, 10);
> @@ -233,6 +237,13 @@ int main(int argc, char *argv[])
>          case '6':
>              ipv6_addr = optarg;
>              break;
> +        case 'k':
> +            keepalive = strtol(optarg, &endptr, 10);
> +            if (*endptr != '\0') {
> +                fprintf(stderr, "Invalid value for --keepalive: '%s'\n",
> optarg);
> +                usage(1, argv[0]);
> +            }
> +            break;
>          case '?':
>          case 'h':
>              usage(o == '?', argv[0]);
> @@ -348,6 +359,24 @@ int main(int argc, char *argv[])
>              break;
>          }
>  
> +        if (keepalive > 0) {
> +            int optval = 1;
> +            socklen_t optlen = sizeof(optval);
> +            if (setsockopt(client_fd, SOL_SOCKET, SO_KEEPALIVE, &optval,
> optlen) == -1) {
> +                if (errno != ENOTSUP) {
> +                    perror("setsockopt SO_KEEPALIVE error.");
> +                    break;
> +                }
> +            }
> +            optval = keepalive;
> +            if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, &optval,
> optlen) == -1) {
> +                if (errno != ENOTSUP) {
> +                    perror("setsockopt TCP_KEEPIDLE error.");
> +                    break;
> +                }
> +            }
> +        }
> +
>          flags = fcntl(client_fd, F_GETFL);
>          if (flags == -1) {
>              perror("fcntl F_GETFL");

Frediano