Re: [PATCH usbredirserver] usbredirserver : enable TCP keepalive

["Zhenwei.Pi" <zhenwei.pi@xxxxxxxxxxxxxxx>]

> > 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.pi@xxxxxxxxxxxxxxx>
> 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 <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

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