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

2018-03-12 Thread Victor Toso 
Hi,

On Fri, Mar 09, 2018 at 04:16:43PM +0800, 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.
> 
> So, add cmdline argument '-k' and "--keepalive" for
> usbredirserver to set tcp keepalive idle time,
> interval time(10s), and maximum number of keepalive probes
> count(3). Detecting disconnection costs time is :
>   idle time + 10s * 3
> 
> If setting TCP keepalive fails with errno ENOTSUP, ignore
> the specific error.
> 
> Signed-off-by: zhenwei.pi 
> Tested-by: Uri Lublin 
> Acked-by: Frediano Ziglio 

Thanks, pushed

commit aca0e87db16da1837e07cb52f160f61999745e84
Author: zhenwei.pi 
Date:   Fri Mar 9 16:16:43 2018 +0800

Cheers,
toso

> ---
>  usbredirserver/usbredirserver.c | 45 
> -
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/usbredirserver/usbredirserver.c b/usbredirserver/usbredirserver.c
> index 5575181..849aa05 100644
> --- a/usbredirserver/usbredirserver.c
> +++ b/usbredirserver/usbredirserver.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #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 ] [-v|--verbose <0-5>] "
>  "[[-4|--ipv4 ipaddr]|[-6|--ipv6 ipaddr]] "
> +"[-k|--keepalive seconds] "
>  "\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, , 10);
> @@ -233,6 +237,13 @@ int main(int argc, char *argv[])
>  case '6':
>  ipv6_addr = optarg;
>  break;
> +case 'k':
> +keepalive = strtol(optarg, , 10);
> +if (*endptr != '\0') {
> +fprintf(stderr, "Invalid value for -k: '%s'\n", optarg);
> +usage(1, argv[0]);
> +}
> +break;
>  case '?':
>  case 'h':
>  usage(o == '?', argv[0]);
> @@ -348,6 +359,38 @@ 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, , 
> optlen) == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt SO_KEEPALIVE error.");
> +break;
> +}
> +}
> +optval = keepalive;  /* set default TCP_KEEPIDLE time from 
> cmdline */
> +if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, , 
> optlen) == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt TCP_KEEPIDLE error.");
> +break;
> +}
> +}
> +optval = 10; /* set default TCP_KEEPINTVL time as 10s */
> +if (setsockopt(client_fd, SOL_TCP, TCP_KEEPINTVL, , 
> optlen) == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt TCP_KEEPINTVL error.");
> +break;
> +}
> +}
> +optval = 3;  /* set default TCP_KEEPCNT as 3 */
> +if (setsockopt(client_fd, SOL_TCP, TCP_KEEPCNT, , optlen) 
> == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt TCP_KEEPCNT error.");
> +break;
> +}
> +}
> +}
> +
>  flags = fcntl(client_fd, F_GETFL);
>  if (flags == -1)

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

2018-03-09 Thread zhenwei.pi 
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,
interval time(10s), and maximum number of keepalive probes
count(3). Detecting disconnection costs time is :
idle time + 10s * 3

If setting TCP keepalive fails with errno ENOTSUP, ignore
the specific error.

Signed-off-by: zhenwei.pi 
Tested-by: Uri Lublin 
Acked-by: Frediano Ziglio 
---
 usbredirserver/usbredirserver.c | 45 -
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/usbredirserver/usbredirserver.c b/usbredirserver/usbredirserver.c
index 5575181..849aa05 100644
--- a/usbredirserver/usbredirserver.c
+++ b/usbredirserver/usbredirserver.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #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 ] [-v|--verbose <0-5>] "
 "[[-4|--ipv4 ipaddr]|[-6|--ipv6 ipaddr]] "
+"[-k|--keepalive seconds] "
 "\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, , 10);
@@ -233,6 +237,13 @@ int main(int argc, char *argv[])
 case '6':
 ipv6_addr = optarg;
 break;
+case 'k':
+keepalive = strtol(optarg, , 10);
+if (*endptr != '\0') {
+fprintf(stderr, "Invalid value for -k: '%s'\n", optarg);
+usage(1, argv[0]);
+}
+break;
 case '?':
 case 'h':
 usage(o == '?', argv[0]);
@@ -348,6 +359,38 @@ 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, , 
optlen) == -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt SO_KEEPALIVE error.");
+break;
+}
+}
+optval = keepalive;/* set default TCP_KEEPIDLE time from 
cmdline */
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, , optlen) 
== -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt TCP_KEEPIDLE error.");
+break;
+}
+}
+optval = 10;   /* set default TCP_KEEPINTVL time as 10s */
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPINTVL, , optlen) 
== -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt TCP_KEEPINTVL error.");
+break;
+}
+}
+optval = 3;/* set default TCP_KEEPCNT as 3 */
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPCNT, , optlen) 
== -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt TCP_KEEPCNT error.");
+break;
+}
+}
+}
+
 flags = fcntl(client_fd, F_GETFL);
 if (flags == -1) {
 perror("fcntl F_GETFL");
-- 
2.7.4

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-03-09 Thread zhenwei.pi 
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(30s),
interval time(10s), and maximum number of keepalive probes
count(3). Detecting disconnection costs time is :
30s + 10s * 3 = 60s

If setting TCP keepalive fails with errno ENOTSUP, ignore
the specific error.

Signed-off-by: zhenwei.pi 
---
 usbredirserver/usbredirserver.c | 41 -
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/usbredirserver/usbredirserver.c b/usbredirserver/usbredirserver.c
index 5575181..246171c 100644
--- a/usbredirserver/usbredirserver.c
+++ b/usbredirserver/usbredirserver.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #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", no_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 ] [-v|--verbose <0-5>] "
 "[[-4|--ipv4 ipaddr]|[-6|--ipv6 ipaddr]] "
+"[-k|--keepalive] "
 "\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  = 0;
 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, , 10);
@@ -233,6 +237,9 @@ int main(int argc, char *argv[])
 case '6':
 ipv6_addr = optarg;
 break;
+case 'k':
+keepalive = 1;
+break;
 case '?':
 case 'h':
 usage(o == '?', argv[0]);
@@ -348,6 +355,38 @@ int main(int argc, char *argv[])
 break;
 }
 
+if (keepalive) {
+int optval = 1;
+socklen_t optlen = sizeof(optval);
+if (setsockopt(client_fd, SOL_SOCKET, SO_KEEPALIVE, , 
optlen) == -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt SO_KEEPALIVE error.");
+break;
+}
+}
+optval = 30;   /* set default TCP_KEEPIDLE time as 30s */
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, , optlen) 
== -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt TCP_KEEPIDLE error.");
+break;
+}
+}
+optval = 10;   /* set default TCP_KEEPINTVL time as 10s */
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPINTVL, , optlen) 
== -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt TCP_KEEPINTVL error.");
+break;
+}
+}
+optval = 3;/* set default TCP_KEEPCNT as 3 */
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPCNT, , optlen) 
== -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt TCP_KEEPCNT error.");
+break;
+}
+}
+}
+
 flags = fcntl(client_fd, F_GETFL);
 if (flags == -1) {
 perror("fcntl F_GETFL");
-- 
2.7.4

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-03-08 Thread 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(30s),
> interval time(10s), and maximum number of keepalive probes
> count(3). Detecting disconnection costs time is :
>   30s + 10s * 3 = 60s
> 
> If setting TCP keepalive fails with errno ENOTSUP, ignore
> the specific error.
> 
> Signed-off-by: zhenwei.pi 
> ---
>  usbredirserver/usbredirserver.c | 41
>  -
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/usbredirserver/usbredirserver.c
> b/usbredirserver/usbredirserver.c
> index 5575181..246171c 100644
> --- a/usbredirserver/usbredirserver.c
> +++ b/usbredirserver/usbredirserver.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #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", no_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 ] [-v|--verbose <0-5>] "
>  "[[-4|--ipv4 ipaddr]|[-6|--ipv6 ipaddr]] "
> +"[-k|--keepalive] "
>  "\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  = 0;
>  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, , 10);
> @@ -233,6 +237,9 @@ int main(int argc, char *argv[])
>  case '6':
>  ipv6_addr = optarg;
>  break;
> +case 'k':
> +keepalive = 1;
> +break;
>  case '?':
>  case 'h':
>  usage(o == '?', argv[0]);
> @@ -348,6 +355,38 @@ int main(int argc, char *argv[])
>  break;
>  }
>  
> +if (keepalive) {
> +int optval = 1;
> +socklen_t optlen = sizeof(optval);
> +if (setsockopt(client_fd, SOL_SOCKET, SO_KEEPALIVE, ,
> optlen) == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt SO_KEEPALIVE error.");
> +break;
> +}
> +}
> +optval = 30; /* set default TCP_KEEPIDLE time as 30s */
> +if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, ,
> optlen) == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt TCP_KEEPIDLE error.");
> +break;
> +}
> +}
> +optval = 10; /* set default TCP_KEEPINTVL time as 10s */
> +if (setsockopt(client_fd, SOL_TCP, TCP_KEEPINTVL, ,
> optlen) == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt TCP_KEEPINTVL error.");
> +break;
> +}
> +}
> +optval = 3;  /* set default TCP_KEEPCNT as 3 */
> +if (setsockopt(client_fd, SOL_TCP, TCP_KEEPCNT, , optlen)
> == -1) {
> +if (errno != ENOTSUP) {
> +perror("setsockopt TCP_KEEPCNT error.");
> +break;
> +}
> +}
> +}
> +
>  flags = fcntl(client_fd, F_GETFL);
>  if (flags == -1) {
>  perror("fcntl F_GETFL");

Good for me.
I would have expect a less strong change, kind of keeping the keepalive
argument and use for just TCP_KEEPIDLE and set the other options with
fixed values. I remember you had an observation about different
environment were could be needed to tweak the time.

Otherwise,
  Acked-by: Frediano Ziglio 

Note that the patch was tested by Uri Lublin, I would add (whomever is
going to merge it) at least a
  Tested-by: Uri Lublin 

Frediano