[Qemu-devel] [PATCH] qemu-char: Allow a chardev to reconnect if disconnected

[Qemu-devel] [PATCH] qemu-char: Allow a chardev to reconnect if disconnected

[arei.gonglei]

From: Huangweidong <weidong.huang@huawei.com>

Allow a socket chardev reconnect if the connection drops while in use.

Signed-off-by: Huangweidong <weidong.huang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
This patch is modified according to corey's patch. Some changes below:
1. IMO it's unnecessary that chardev reconnect if it fails to connect at startup.
Qemu exit in this scene. In this way the patch does not change interface of chardev.
It would be much more simple.
2. I set the reconnect timer one second, just like pty.

 include/sysemu/char.h |  2 ++
 qemu-char.c           | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index b81a6ff..f646ac8 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -19,6 +19,7 @@
 #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
 #define CHR_EVENT_CLOSED  5 /* connection closed */
 
+#define CHR_SOCK_RECONNECT_TIME 1 /* reconnection time (second) */
 
 #define CHR_IOCTL_SERIAL_SET_PARAMS   1
 typedef struct {
@@ -82,6 +83,7 @@ struct CharDriverState {
     guint fd_in_tag;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
+    QEMUTimer *recon_timer;
 };
 
 /**
diff --git a/qemu-char.c b/qemu-char.c
index 54ed244..a87a345 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -96,9 +96,17 @@ void qemu_chr_be_event(CharDriverState *s, int event)
     /* Keep track if the char device is open */
     switch (event) {
         case CHR_EVENT_OPENED:
+            if (s->recon_timer) {
+                timer_del(s->recon_timer);
+            }
             s->be_open = 1;
             break;
         case CHR_EVENT_CLOSED:
+            if (s->recon_timer) {
+                timer_mod(s->recon_timer,
+                (get_clock() +
+                 (CHR_SOCK_RECONNECT_TIME * get_ticks_per_sec())));
+            }
             s->be_open = 0;
             break;
     }
@@ -2619,6 +2627,43 @@ static void tcp_chr_close(CharDriverState *chr)
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
+static void recon_timeout(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    QemuOpts *opts = chr->opts;
+    TCPCharDriver *tcp = (TCPCharDriver *)chr->opaque;
+    int fd = -1;
+    Error *local_err = NULL;
+
+    if (chr->be_open) {
+        return;
+    }
+
+    if (tcp->is_unix) {
+        fd = unix_connect_opts(opts, &local_err, NULL, NULL);
+    } else {
+        fd = inet_connect_opts(opts, &local_err, NULL, NULL);
+    }
+
+    if (fd < 0) {
+        goto fail;
+    }
+
+    tcp->fd = fd;
+    socket_set_nodelay(fd);
+    tcp->chan = io_channel_from_socket(tcp->fd);
+    tcp_chr_connect(chr);
+    printf("chardev: socket reconnect sucess\n");
+    return;
+
+fail:
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+}
+
 static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
                                                 bool is_listen, bool is_telnet,
                                                 bool is_waitconnect,
@@ -2693,6 +2738,11 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
         socket_set_nodelay(fd);
         s->chan = io_channel_from_socket(s->fd);
         tcp_chr_connect(chr);
+        chr->recon_timer = timer_new(QEMU_CLOCK_REALTIME, SCALE_NS,
+                                 recon_timeout, chr);
+        timer_mod(chr->recon_timer,
+                   (get_clock() +
+                    (CHR_SOCK_RECONNECT_TIME * get_ticks_per_sec())));
     }
 
     if (is_listen && is_waitconnect) {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] qemu-char: Allow a chardev to reconnect if disconnected

[Corey Minyard]

On 04/10/2014 06:43 AM, arei.gonglei@huawei.com wrote:
> From: Huangweidong <weidong.huang@huawei.com>
>
> Allow a socket chardev reconnect if the connection drops while in use.
>
> Signed-off-by: Huangweidong <weidong.huang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> This patch is modified according to corey's patch. Some changes below:
> 1. IMO it's unnecessary that chardev reconnect if it fails to connect at startup.
> Qemu exit in this scene. In this way the patch does not change interface of chardev.
> It would be much more simple.

I believe that it should not stop qemu if it fails at startup. 
Otherwise you constrain the start order and you can prevent a server
from coming up because of a missing resource that may not be that
critical at the moment.  With the current implementation, client sockets
really aren't that useful for a critical system.  Reconnecting makes it
usable in a critical system.

> 2. I set the reconnect timer one second, just like pty.

I'm not too picky about the time.  A couple of things about this:

With this patch, the default behavior changes to reconnect.  That might
cause issues for some users.  Adding a configurable timeout is easy if
you have to specify something on the command line, that's why I did it.

Also, if something is listening to connect/disconnect events from the
device, it will get a connect then disconnect every second.  It's
probably better to wait until the connection is actually established
before you report it up.

-corey
>
>  include/sysemu/char.h |  2 ++
>  qemu-char.c           | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index b81a6ff..f646ac8 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -19,6 +19,7 @@
>  #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
>  #define CHR_EVENT_CLOSED  5 /* connection closed */
>  
> +#define CHR_SOCK_RECONNECT_TIME 1 /* reconnection time (second) */
>  
>  #define CHR_IOCTL_SERIAL_SET_PARAMS   1
>  typedef struct {
> @@ -82,6 +83,7 @@ struct CharDriverState {
>      guint fd_in_tag;
>      QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
> +    QEMUTimer *recon_timer;
>  };
>  
>  /**
> diff --git a/qemu-char.c b/qemu-char.c
> index 54ed244..a87a345 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -96,9 +96,17 @@ void qemu_chr_be_event(CharDriverState *s, int event)
>      /* Keep track if the char device is open */
>      switch (event) {
>          case CHR_EVENT_OPENED:
> +            if (s->recon_timer) {
> +                timer_del(s->recon_timer);
> +            }
>              s->be_open = 1;
>              break;
>          case CHR_EVENT_CLOSED:
> +            if (s->recon_timer) {
> +                timer_mod(s->recon_timer,
> +                (get_clock() +
> +                 (CHR_SOCK_RECONNECT_TIME * get_ticks_per_sec())));
> +            }
>              s->be_open = 0;
>              break;
>      }
> @@ -2619,6 +2627,43 @@ static void tcp_chr_close(CharDriverState *chr)
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
>  
> +static void recon_timeout(void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    QemuOpts *opts = chr->opts;
> +    TCPCharDriver *tcp = (TCPCharDriver *)chr->opaque;
> +    int fd = -1;
> +    Error *local_err = NULL;
> +
> +    if (chr->be_open) {
> +        return;
> +    }
> +
> +    if (tcp->is_unix) {
> +        fd = unix_connect_opts(opts, &local_err, NULL, NULL);
> +    } else {
> +        fd = inet_connect_opts(opts, &local_err, NULL, NULL);
> +    }
> +
> +    if (fd < 0) {
> +        goto fail;
> +    }
> +
> +    tcp->fd = fd;
> +    socket_set_nodelay(fd);
> +    tcp->chan = io_channel_from_socket(tcp->fd);
> +    tcp_chr_connect(chr);
> +    printf("chardev: socket reconnect sucess\n");
> +    return;
> +
> +fail:
> +    if (local_err) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    }
> +    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +}
> +
>  static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
>                                                  bool is_listen, bool is_telnet,
>                                                  bool is_waitconnect,
> @@ -2693,6 +2738,11 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
>          socket_set_nodelay(fd);
>          s->chan = io_channel_from_socket(s->fd);
>          tcp_chr_connect(chr);
> +        chr->recon_timer = timer_new(QEMU_CLOCK_REALTIME, SCALE_NS,
> +                                 recon_timeout, chr);
> +        timer_mod(chr->recon_timer,
> +                   (get_clock() +
> +                    (CHR_SOCK_RECONNECT_TIME * get_ticks_per_sec())));
>      }
>  
>      if (is_listen && is_waitconnect) {

Re: [Qemu-devel] [PATCH] qemu-char: Allow a chardev to reconnect if disconnected

[Gerd Hoffmann]

  Hi,

> > This patch is modified according to corey's patch. Some changes below:
> > 1. IMO it's unnecessary that chardev reconnect if it fails to connect at startup.
> > Qemu exit in this scene. In this way the patch does not change interface of chardev.
> > It would be much more simple.
> 
> I believe that it should not stop qemu if it fails at startup. 
> Otherwise you constrain the start order and you can prevent a server
> from coming up because of a missing resource that may not be that
> critical at the moment.

With reconnect being implemented (and active, guess we should add a
option for it) it we might rethink this indeed.

> With the current implementation, client sockets
> really aren't that useful for a critical system.  Reconnecting makes it
> usable in a critical system.

I consider client sockets not very useful at all, I always put my
sockets into server mode.  YMMV though.

> > 2. I set the reconnect timer one second, just like pty.
> 
> I'm not too picky about the time.  A couple of things about this:

Well, sockets and pty are different.  pty is a local thing.  sockets (at
least tcp) goes out to the network.  After each failed reconnect the
time to retry should be increased a bit so you don't flood the
non-responding server with connect requests.

> With this patch, the default behavior changes to reconnect.  That might
> cause issues for some users.  Adding a configurable timeout is easy if
> you have to specify something on the command line, that's why I did it.

Specifying the timeout (best with min+max) could enable reconnect at the
same time, i.e. something like

  -chardev socket,host=1.2.3.4,port=5678,reconnect=1-300

would turn on reconnect, with 1 second min and 300 seconds (5min) max
delay.

cheers,
  Gerd