From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47493) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYGPE-0004um-Cs for qemu-devel@nongnu.org; Thu, 10 Apr 2014 10:56:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WYGP8-0000xH-Tl for qemu-devel@nongnu.org; Thu, 10 Apr 2014 10:56:44 -0400 Received: from mail-oa0-x236.google.com ([2607:f8b0:4003:c02::236]:57644) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYGP8-0000x5-N3 for qemu-devel@nongnu.org; Thu, 10 Apr 2014 10:56:38 -0400 Received: by mail-oa0-f54.google.com with SMTP id n16so4640519oag.27 for ; Thu, 10 Apr 2014 07:56:37 -0700 (PDT) Sender: Corey Minyard Message-ID: <5346B123.3070606@acm.org> Date: Thu, 10 Apr 2014 09:56:35 -0500 From: Corey Minyard MIME-Version: 1.0 References: <1397130226-7332-1-git-send-email-arei.gonglei@huawei.com> In-Reply-To: <1397130226-7332-1-git-send-email-arei.gonglei@huawei.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-char: Allow a chardev to reconnect if disconnected Reply-To: minyard@acm.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: arei.gonglei@huawei.com, qemu-devel@nongnu.org Cc: bcketchum@gmail.com, Huangweidong , kraxel@redhat.com, afaerber@suse.de, mst@redhat.com On 04/10/2014 06:43 AM, arei.gonglei@huawei.com wrote: > From: Huangweidong > > Allow a socket chardev reconnect if the connection drops while in use. > > Signed-off-by: Huangweidong > Signed-off-by: Gonglei > --- > 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) {