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

[Uri Lublin]

On 03/01/2018 11:08 AM, zhenwei.pi wrote:
> 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.

Hi,

Note that since you only setup TCP_KEEPIDLE, it will take
longer to detect disconnection (interval * probes more,
aka TCP_KEEPINTVL * TCP_KEEPCNT). How much longer depends
on the system configuration.

The patch looks good.
I tested it with usbredirtestclient and iptables.

Uri.

> 
> 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>
> ---
>   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");
>