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

[Zhenwei.Pi]

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?
  

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