Re: [Qemu-devel] [PATCH 5/6] qemu-char: Add reconnecting to client sockets

[Paolo Bonzini <pbonzini@redhat.com>]

Il 22/09/2014 01:04, minyard@acm.org ha scritto:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Adds a "reconnect" option to socket backends that gives a reconnect
> timeout.  This only applies to client sockets.  If the other end
> of a socket closes the connection, qemu will attempt to reconnect
> after the given number of seconds.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Comments inline.

> ---
>  qapi-schema.json | 14 +++++----
>  qemu-char.c      | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  qemu-options.hx  | 20 ++++++++-----
>  3 files changed, 106 insertions(+), 17 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 689b548..79f7a07 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2648,14 +2648,18 @@
>  # @nodelay: #optional set TCP_NODELAY socket option (default: false)
>  # @telnet: #optional enable telnet protocol on server
>  #          sockets (default: false)
> +# @reconnect: #optional If not a server socket, if the socket disconnect
> +#          then reconnect after the given number of seconds.  Setting
> +#          to zero disables this function. (default: 0). Since: 2.2.
>  #
>  # Since: 1.4
>  ##
> -{ 'type': 'ChardevSocket', 'data': { 'addr'     : 'SocketAddress',
> -                                     '*server'  : 'bool',
> -                                     '*wait'    : 'bool',
> -                                     '*nodelay' : 'bool',
> -                                     '*telnet'  : 'bool' } }
> +{ 'type': 'ChardevSocket', 'data': { 'addr'       : 'SocketAddress',
> +                                     '*server'    : 'bool',
> +                                     '*wait'      : 'bool',
> +                                     '*nodelay'   : 'bool',
> +                                     '*telnet'    : 'bool',
> +                                     '*reconnect' : 'int' } }
>  
>  ##
>  # @ChardevUdp:
> diff --git a/qemu-char.c b/qemu-char.c
> index 8418e33..eefebd4 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2493,8 +2493,20 @@ typedef struct {
>      SocketAddress *addr;
>      bool is_listen;
>      bool is_telnet;
> +
> +    guint reconnect_timer;
> +    int64_t reconnect_time;
>  } TCPCharDriver;
>  
> +static gboolean socket_reconnect_timeout(gpointer opaque);
> +
> +static void qemu_chr_socket_restart_timer(CharDriverState *chr)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
> +                                               socket_reconnect_timeout, chr);
> +}
> +
>  static gboolean tcp_chr_accept(GIOChannel *chan, GIOCondition cond, void *opaque);
>  
>  #ifndef _WIN32
> @@ -2776,6 +2788,9 @@ static void tcp_chr_disconnect(CharDriverState *chr)
>      SocketAddress_to_str(chr->filename, CHR_MAX_FILENAME_SIZE,
>                           "disconnected:", s->addr, s->is_listen, s->is_telnet);
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +    if (s->reconnect_time) {
> +        qemu_chr_socket_restart_timer(chr);
> +    }
>  }
>  
>  static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> @@ -2956,6 +2971,9 @@ static void tcp_chr_close(CharDriverState *chr)
>      TCPCharDriver *s = chr->opaque;
>      int i;
>  
> +    if (s->reconnect_timer) {
> +        g_source_remove(s->reconnect_timer);

Please add "s->reconnect_timer = 0;" here for easier debugging.

> +    }
>      qapi_free_SocketAddress(s->addr);
>      if (s->fd >= 0) {
>          remove_fd_in_watch(chr);
> @@ -3005,7 +3023,28 @@ static bool qemu_chr_finish_socket_connection(CharDriverState *chr, int fd,
>          tcp_chr_connect(chr);
>      }
>  
> -    return chr;
> +    return true;
> +}
> +
> +static void qemu_chr_socket_connected(int fd, void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    TCPCharDriver *s = chr->opaque;
> +    Error *err = NULL;
> +
> +    if (fd >= 0) {
> +        if (qemu_chr_finish_socket_connection(chr, fd, &err)) {
> +            return;
> +        }
> +        if (err) {
> +            error_report("%s", error_get_pretty(err));
> +            error_free(err);
> +        }
> +        closesocket(fd);
> +    }
> +
> +    s->connected = 0;
> +    qemu_chr_socket_restart_timer(chr);
>  }
>  
>  static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp)
> @@ -3013,6 +3052,11 @@ static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp)
>      TCPCharDriver *s = chr->opaque;
>      int fd;
>  
> +    if (s->reconnect_time) {
> +        fd = socket_connect(s->addr, errp, qemu_chr_socket_connected, chr);
> +        return (fd >= 0);
> +    }

The placement of this condition looks weird...

You're missing a check somewhere that reconnect is not specified
together with listen.  Please add it, and move the condition in the
"else" side of the "if" just below.

>      if (s->is_listen) {
>          fd = socket_listen(s->addr, errp);
>      } else  {
> @@ -3442,6 +3486,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
>      bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
>      bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
> +    int64_t reconnect   = qemu_opt_get_number(opts, "reconnect", 0);
>      const char *path = qemu_opt_get(opts, "path");
>      const char *host = qemu_opt_get(opts, "host");
>      const char *port = qemu_opt_get(opts, "port");
> @@ -4020,6 +4090,7 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
>      bool is_listen      = sock->has_server  ? sock->server  : true;
>      bool is_telnet      = sock->has_telnet  ? sock->telnet  : false;
>      bool is_waitconnect = sock->has_wait    ? sock->wait    : false;
> +    int64_t reconnect   = sock->has_reconnect ? sock->reconnect : 0;
>  
>      chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(TCPCharDriver));
> @@ -4036,6 +4107,8 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
>      s->is_telnet = is_telnet;
>      s->do_nodelay = do_nodelay;
>      qapi_copy_SocketAddress(&s->addr, sock->addr);
> +    s->reconnect_timer = 0;
> +    s->reconnect_time = 0;

Not needed after g_malloc0.
>  
>      chr->opaque = s;
>      chr->chr_write = tcp_chr_write;
> @@ -4057,13 +4130,19 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
>          if (is_telnet) {
>              s->do_telnetopt = 1;
>          }
> +    } else if (reconnect > 0) {
> +        s->reconnect_time = reconnect;
>      }
>  
>      if (!qemu_chr_open_socket_fd(chr, errp)) {
> -        g_free(s);
> -        g_free(chr->filename);
> -        g_free(chr);
> -        return NULL;
> +        if (s->reconnect_time) {
> +            qemu_chr_socket_restart_timer(chr);
> +        } else {
> +            g_free(s);
> +            g_free(chr->filename);
> +            g_free(chr);
> +            return NULL;
> +        }
>      }
>  
>      if (is_listen && is_waitconnect) {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 365b56c..0cb856f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1930,9 +1930,9 @@ ETEXI
>  
>  DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>      "-chardev null,id=id[,mux=on|off]\n"
> -    "-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4][,ipv6][,nodelay]\n"
> -    "         [,server][,nowait][,telnet][,mux=on|off] (tcp)\n"
> -    "-chardev socket,id=id,path=path[,server][,nowait][,telnet],[mux=on|off] (unix)\n"
> +    "-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4][,ipv6][,nodelay][,reconnect=reconnect_time]\n"

s/reconnect_time/seconds/

> +    "         [,server][,nowait][,telnet][,reconnect=reconnect_time][,mux=on|off] (tcp)\n"
> +    "-chardev socket,id=id,path=path[,server][,nowait][,telnet][,reconnect=reconnect_time][,mux=on|off] (unix)\n"

s/reconnect_time/seconds/

>      "-chardev udp,id=id[,host=host],port=port[,localaddr=localaddr]\n"
>      "         [,localport=localport][,ipv4][,ipv6][,mux=on|off]\n"
>      "-chardev msmouse,id=id[,mux=on|off]\n"
> @@ -2004,7 +2004,7 @@ Options to each backend are described below.
>  A void device. This device will not emit any data, and will drop any data it
>  receives. The null backend does not take any options.
>  
> -@item -chardev socket ,id=@var{id} [@var{TCP options} or @var{unix options}] [,server] [,nowait] [,telnet]
> +@item -chardev socket ,id=@var{id} [@var{TCP options} or @var{unix options}] [,server] [,nowait] [,telnet] [,reconnect=reconnect_time]

s/reconnect_time/@var{seconds}/

>  Create a two-way stream socket, which can be either a TCP or a unix socket. A
>  unix socket will be created if @option{path} is specified. Behaviour is
> @@ -2018,6 +2018,10 @@ connect to a listening socket.
>  @option{telnet} specifies that traffic on the socket should interpret telnet
>  escape sequences.
>  
> +@option{reconnect} sets the timeout for reconnecting on non-server sockets when
> +the remote end goes away.  qemu will delay this many seconds and then attempt
> +to reconnect.  Zero disables reconnecting, and is the default.
> +
>  TCP and unix socket options are given below:
>  
>  @table @option
> @@ -2687,14 +2691,16 @@ telnet on port 5555 to access the QEMU port.
>  localhost 5555
>  @end table
>  
> -@item tcp:[@var{host}]:@var{port}[,@var{server}][,nowait][,nodelay]
> +@item tcp:[@var{host}]:@var{port}[,@var{server}][,nowait][,nodelay][,reconnect=reconnect_time]

s/reconnect_time/@var{seconds}/

>  The TCP Net Console has two modes of operation.  It can send the serial
>  I/O to a location or wait for a connection from a location.  By default
>  the TCP Net Console is sent to @var{host} at the @var{port}.  If you use
>  the @var{server} option QEMU will wait for a client socket application
>  to connect to the port before continuing, unless the @code{nowait}
>  option was specified.  The @code{nodelay} option disables the Nagle buffering
> -algorithm.  If @var{host} is omitted, 0.0.0.0 is assumed. Only
> +algorithm.  The @code{reconnect} option only applies if @var{noserver} is
> +set, if the connection goes down it will attempt to reconnect at the
> +given interval.  If @var{host} is omitted, 0.0.0.0 is assumed. Only
>  one TCP connection at a time is accepted. You can use @code{telnet} to
>  connect to the corresponding character device.
>  @table @code
> @@ -2715,7 +2721,7 @@ MAGIC_SYSRQ sequence if you use a telnet that supports sending the break
>  sequence.  Typically in unix telnet you do it with Control-] and then
>  type "send break" followed by pressing the enter key.
>  
> -@item unix:@var{path}[,server][,nowait]
> +@item unix:@var{path}[,server][,nowait][,reconnect=reconnect_time]

s/reconnect_time/@var{seconds}/

Paolo

>  A unix domain socket is used instead of a tcp socket.  The option works the
>  same as if you had specified @code{-serial tcp} except the unix domain socket
>  @var{path} is used for connections.
>