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

[Frediano Ziglio]

> 
> On 03/08/2018 03:30 AM, Uri Lublin wrote:
> 
> > 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.
> 
> We can also add more cmdline arguments for TCP_KEEPINTVL and TCP_KEEPCNT,
> but usbredirserver seems a little difficult to setup.
> Setting default value (like TCP_KEEPINTVL as 10s, TCP_KEEPCNT as 3) or using
> the system configuration, which is better?
>   

Setting fixed (compiled in) values if -k is specified seems a good idea
to me.

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

Frediano