[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 <zhenwei...@youruncloud.com>
Tested-by: Uri Lublin <u...@redhat.com>
Acked-by: Frediano Ziglio <fzig...@redhat.com>
---
 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] "
 "<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, , 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 <zhenwei...@youruncloud.com>
---
 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] "
 "<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  = 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] usbredirserver : enable TCP keepalive

2018-03-08 Thread 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...@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 
   #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 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, , 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 --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, , 
optlen) == -1) {
+    if (errno != ENOTSUP) {
+    perror("setsockopt SO_KEEPALIVE error.");
+    break;
+    }
+    }
+    optval = keepalive;
+    if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, , optlen) 
== -1) {
+    if (errno != ENOTSUP) {
+    perror("setsockopt TCP_KEEPIDLE error.");
+    break;
+    }
+    }
+    }
+
   flags = fcntl(client_fd, F_GETFL);
   if (flags == -1) {
   perror("fcntl F_GETFL");



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


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

2018-03-05 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.

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

Signed-off-by: zhenwei.pi <zhenwei...@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.


This patch is a method of "try to detect disconnections"!
"client_fd" means the connection of the server and client, and the server
sets TCP keepalive for the connection. So the server will send TCP KEEPALIVE
message to client. If client does not response or send RESET, the server will
release the idle connection.
A new connection (or reconnection) could be accepted by the server.


---
  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 
  #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 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, , 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 --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, ,
optlen) == -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt SO_KEEPALIVE error.");
+break;
+}
+}
+optval = keepalive;
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, ,
optlen) == -1) {
+if (errno != ENOTSUP) {
+perror("setsock

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

2018-03-01 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.

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

Signed-off-by: zhenwei.pi <zhenwei...@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 
 #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 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, , 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 --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, , 
optlen) == -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt SO_KEEPALIVE error.");
+break;
+}
+}
+optval = keepalive;
+if (setsockopt(client_fd, SOL_TCP, TCP_KEEPIDLE, , optlen) 
== -1) {
+if (errno != ENOTSUP) {
+perror("setsockopt TCP_KEEPIDLE 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