[Qemu-devel] Socket reconnection.

[Qemu-devel] Socket reconnection.

[Ian Molton]

Hi folks,

I need my source of data for virtio-rng to be reliable - IOW. if the
server dies and comes back up, I want qemu to reconnect and suck down
fresh entropy, rather than hand the rngd process on the guest.

I'm using the chardev 'socket' type to make the connection to the host.

Would a patch adding (optional) auto-reconnect (with a back-off delay)
to the socket chardev be acceptable ?

If not, I'll have to cook up a thread to handle EGD requests, but that
seems perverse...

-Ian

Re: [Qemu-devel] Socket reconnection.

[Gerd Hoffmann]

   Hi,

> Would a patch adding (optional) auto-reconnect (with a back-off delay)
> to the socket chardev be acceptable ?

Sounds sensible to me.  And, yes, it should be an option (defaulting to 
off to maintain current behavior when not specified).

cheers,
   Gerd

Re: [Qemu-devel] Socket reconnection.

[Jamie Lokier]

Ian Molton wrote:
> Hi folks,
> 
> I need my source of data for virtio-rng to be reliable - IOW. if the
> server dies and comes back up, I want qemu to reconnect and suck down
> fresh entropy, rather than hand the rngd process on the guest.
> 
> I'm using the chardev 'socket' type to make the connection to the host.
> 
> Would a patch adding (optional) auto-reconnect (with a back-off delay)
> to the socket chardev be acceptable ?
> 
> If not, I'll have to cook up a thread to handle EGD requests, but that
> seems perverse...

I'm a bit puzzled.

Why isn't virtio-rng getting entropy from /dev/random on the host?

-- Jamie

Re: [Qemu-devel] Socket reconnection.

[Anthony Liguori]

Ian Molton wrote:
> Hi folks,
>
> I need my source of data for virtio-rng to be reliable - IOW. if the
> server dies and comes back up, I want qemu to reconnect and suck down
> fresh entropy, rather than hand the rngd process on the guest.
>
> I'm using the chardev 'socket' type to make the connection to the host.
>
> Would a patch adding (optional) auto-reconnect (with a back-off delay)
> to the socket chardev be acceptable ?
>
> If not, I'll have to cook up a thread to handle EGD requests, but that
> seems perverse...
>   

Hrm, I'm not sure.  What are the circumstances that this connection 
would die?  What happens while the connection is dead?

Regards,

Anthony Liguori
> -Ian
>
>
>   

Re: [Qemu-devel] Socket reconnection.

[Ian Molton]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

Anthony Liguori wrote:
> Ian Molton wrote:
>> Hi folks,
>>
>> I need my source of data for virtio-rng to be reliable - IOW. if the
>> server dies and comes back up, I want qemu to reconnect and suck down
>> fresh entropy, rather than hand the rngd process on the guest.
>>
>> I'm using the chardev 'socket' type to make the connection to the host.
>>
>> Would a patch adding (optional) auto-reconnect (with a back-off delay)
>> to the socket chardev be acceptable ?
>>
>> If not, I'll have to cook up a thread to handle EGD requests, but that
>> seems perverse...
>>   
> 
> Hrm, I'm not sure.  What are the circumstances that this connection
> would die?  What happens while the connection is dead?

The most common would be the entropy gathering daemon being restarted,
perhaps due to an upgrade. Its hardly a good idea to require all the
guest VMs to reboot on this occuring. Next most common I guess would be
the daemon crashing, but again, not something you'd want taking out your
guest VMs...

Whilst the connection is down, the guests will potentially starve of
entropy - but that only means they'll block processes that try to use
/dev/random if they run out altogether.

Here are two patches that implement socket reconnection. This first
cleans up the APIs needed a little  and the second implements the guts.

If these patches are acceptable, I will repost my 4-patch series which
also includes the SIZE parameter patch and an updated virtio-rng patch
that uses the reconnection infrastructure to enhance its reliability.

-Ian

[-- Attachment #2: 0001-socket-Rationalise-function-declarations.patch --]
[-- Type: text/x-patch, Size: 2133 bytes --]

>From 44dc6eddf23713f34874c2cbfa5cba763983cf6f Mon Sep 17 00:00:00 2001
From: Ian Molton <ian.molton@collabora.co.uk>
Date: Tue, 1 Dec 2009 11:16:25 +0000
Subject: [PATCH 1/4] socket: Rationalise function declarations

	This patch rationalises the declaration of inet_listen_opts such that
it matches the other inet_{listen,connect}_opts functions.

This change is needed for a patch adding socket reconection support.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 qemu-sockets.c |    9 +++++++--
 qemu_socket.h  |    2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 8801453..b655b3c 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -116,7 +116,7 @@ static void inet_print_addrinfo(const char *tag, struct addrinfo *res)
     }
 }
 
-int inet_listen_opts(QemuOpts *opts, int port_offset)
+static int do_inet_listen(QemuOpts *opts, int port_offset)
 {
     struct addrinfo ai,*res,*e;
     const char *addr;
@@ -216,6 +216,11 @@ listen:
     return slisten;
 }
 
+int inet_listen_opts(QemuOpts *opts)
+{
+    return do_inet_listen(opts, 0);
+}
+
 int inet_connect_opts(QemuOpts *opts)
 {
     struct addrinfo ai,*res,*e;
@@ -465,7 +470,7 @@ int inet_listen(const char *str, char *ostr, int olen,
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0);
     if (inet_parse(opts, str) == 0) {
-        sock = inet_listen_opts(opts, port_offset);
+        sock = do_inet_listen(opts, port_offset);
         if (sock != -1 && ostr) {
             optstr = strchr(str, ',');
             if (qemu_opt_get_bool(opts, "ipv6", 0)) {
diff --git a/qemu_socket.h b/qemu_socket.h
index c253b32..3b09e0a 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -36,7 +36,7 @@ void socket_set_nonblock(int fd);
 int send_all(int fd, const void *buf, int len1);
 
 /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
-int inet_listen_opts(QemuOpts *opts, int port_offset);
+int inet_listen_opts(QemuOpts *opts);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset);
 int inet_connect_opts(QemuOpts *opts);
-- 
1.6.5


[-- Attachment #3: 0002-socket-Add-a-reconnect-option.patch --]
[-- Type: text/x-patch, Size: 6992 bytes --]

>From 1641774343cec683a6e11c22ef2ab162ab2cb842 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian.molton@collabora.co.uk>
Date: Tue, 1 Dec 2009 11:18:41 +0000
Subject: [PATCH 2/4] socket: Add a reconnect option.

	Add a reconnect option that allows sockets to reconnect (after a
specified delay) to the specified server. This makes the virtio-rng driver
useful in production environments where the EGD server may need to be restarted.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 qemu-char.c   |  118 ++++++++++++++++++++++++++++++++++++++-------------------
 qemu-char.h   |    1 +
 qemu-config.c |    3 +
 3 files changed, 83 insertions(+), 39 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e202585..ec24c29 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1870,8 +1870,12 @@ typedef struct {
     int max_size;
     int do_telnetopt;
     int do_nodelay;
+    int reconnect;
     int is_unix;
     int msgfd;
+    QemuOpts *opts;
+    CharDriverState *chr;
+    int (*setup)(QemuOpts *opts);
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -2011,6 +2015,8 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
 }
 #endif
 
+static int qemu_chr_connect_socket(TCPCharDriver *s);
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
@@ -2030,10 +2036,18 @@ static void tcp_chr_read(void *opaque)
         if (s->listen_fd >= 0) {
             qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
         }
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        if (!s->reconnect)
+            qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
         closesocket(s->fd);
         s->fd = -1;
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        if (!s->reconnect) {
+            qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        } else {
+            do {
+                sleep(s->reconnect);
+            } while(!qemu_chr_connect_socket(s));
+            qemu_chr_event(chr, CHR_EVENT_RECONNECTED);
+        }
     } else if (size > 0) {
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
@@ -2137,7 +2151,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 {
     CharDriverState *chr = NULL;
     TCPCharDriver *s = NULL;
-    int fd = -1;
     int is_listen;
     int is_waitconnect;
     int do_nodelay;
@@ -2145,34 +2158,40 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     int is_telnet;
 
     is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    is_unix        = qemu_opt_get(opts, "path") != NULL;
+
     is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
     is_telnet      = qemu_opt_get_bool(opts, "telnet", 0);
     do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
-    is_unix        = qemu_opt_get(opts, "path") != NULL;
-    if (!is_listen)
+
+    if (!is_listen) {
         is_waitconnect = 0;
+    } else {
+        if (is_telnet)
+            s->do_telnetopt = 1;
+    }
+
 
-    chr = qemu_mallocz(sizeof(CharDriverState));
     s = qemu_mallocz(sizeof(TCPCharDriver));
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    s->opts = opts;
+
+    if (!is_listen && !is_telnet)
+        s->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
 
     if (is_unix) {
         if (is_listen) {
-            fd = unix_listen_opts(opts);
+            s->setup = unix_listen_opts;
         } else {
-            fd = unix_connect_opts(opts);
+            s->setup = unix_connect_opts;
         }
     } else {
         if (is_listen) {
-            fd = inet_listen_opts(opts, 0);
+            s->setup = inet_listen_opts;
         } else {
-            fd = inet_connect_opts(opts);
+            s->setup = inet_connect_opts;
         }
     }
-    if (fd < 0)
-        goto fail;
-
-    if (!is_waitconnect)
-        socket_set_nonblock(fd);
 
     s->connected = 0;
     s->fd = -1;
@@ -2186,19 +2205,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     chr->chr_close = tcp_chr_close;
     chr->get_msgfd = tcp_get_msgfd;
 
-    if (is_listen) {
-        s->listen_fd = fd;
-        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
-        if (is_telnet)
-            s->do_telnetopt = 1;
-
-    } else {
-        s->connected = 1;
-        s->fd = fd;
-        socket_set_nodelay(fd);
-        tcp_chr_connect(chr);
-    }
-
     /* for "info chardev" monitor command */
     chr->filename = qemu_malloc(256);
     if (is_unix) {
@@ -2215,22 +2221,56 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
                  qemu_opt_get_bool(opts, "server", 0) ? ",server" : "");
     }
 
-    if (is_listen && is_waitconnect) {
-        printf("QEMU waiting for connection on: %s\n",
-               chr->filename);
-        tcp_chr_accept(chr);
-        socket_set_nonblock(s->listen_fd);
-    }
-    return chr;
+    s->chr = chr;
+
+    if(qemu_chr_connect_socket(s))
+        return chr;
 
- fail:
-    if (fd >= 0)
-        closesocket(fd);
-    qemu_free(s);
     qemu_free(chr);
+    qemu_free(s);
+
     return NULL;
 }
 
+
+static int qemu_chr_connect_socket(TCPCharDriver *s)
+{
+    QemuOpts *opts = s->opts;
+    int is_listen;
+    int fd;
+    int is_waitconnect;
+    int do_nodelay;
+
+    is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
+    is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
+
+
+    fd = s->setup(s->opts);
+    if (fd < 0)
+        return 0;
+
+    if (!is_waitconnect)
+        socket_set_nonblock(fd);
+
+    if (is_listen) {
+        s->listen_fd = fd;
+        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, s->chr);
+        if (is_waitconnect) {
+            printf("QEMU waiting for connection on: %s\n",
+                   s->chr->filename);
+            tcp_chr_accept(s->chr);
+            socket_set_nonblock(s->listen_fd);
+        }
+    } else {
+        s->fd = fd;
+        socket_set_nodelay(fd);
+        tcp_chr_connect(s->chr);
+    }
+
+    return 1;
+}
+
 static QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
diff --git a/qemu-char.h b/qemu-char.h
index 9957db1..266536d 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -14,6 +14,7 @@
 #define CHR_EVENT_MUX_IN  3 /* mux-focus was set to this terminal */
 #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
 #define CHR_EVENT_CLOSED  5 /* connection closed */
+#define CHR_EVENT_RECONNECTED  6 /* reconnect event */
 
 
 #define CHR_IOCTL_SERIAL_SET_PARAMS   1
diff --git a/qemu-config.c b/qemu-config.c
index 590fc05..ff8b06e 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -140,6 +140,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "signal",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "reconnect",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end if list */ }
     },
-- 
1.6.5

Re: [Qemu-devel] Socket reconnection.

[Ian Molton]

Jamie Lokier wrote:

> I'm a bit puzzled.
> 
> Why isn't virtio-rng getting entropy from /dev/random on the host?

/dev/random may not be available. Besides, not all entropy comes from
/dev/random.

-Ian

RE: [Qemu-devel] Socket reconnection.

[Krumme, Chris]

Hello Ian,

Since you did not inline your source I will paste in a chunk:


@@ -2030,10 +2036,18 @@ static void tcp_chr_read(void *opaque)
         if (s->listen_fd >= 0) {
             qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL,
chr);
         }
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        if (!s->reconnect)
+            qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
         closesocket(s->fd);
         s->fd = -1;
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        if (!s->reconnect) {
+            qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        } else {
+            do {
+                sleep(s->reconnect);
+            } while(!qemu_chr_connect_socket(s));
+            qemu_chr_event(chr, CHR_EVENT_RECONNECTED);
+        }
     } else if (size > 0) {
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);


Should you be introducing a while sleep loop into Qemu here?

I would think you should be returning 'no data', maybe after trying
once.

Hope this helps.

Chris

 

> -----Original Message-----
> From: 
> qemu-devel-bounces+chris.krumme=windriver.com@nongnu.org 
> [mailto:qemu-devel-bounces+chris.krumme=windriver.com@nongnu.o
> rg] On Behalf Of Ian Molton
> Sent: Tuesday, December 01, 2009 5:54 AM
> To: Anthony Liguori
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] Socket reconnection.
> 
> Anthony Liguori wrote:
> > Ian Molton wrote:
> >> Hi folks,
> >>
> >> I need my source of data for virtio-rng to be reliable - 
> IOW. if the
> >> server dies and comes back up, I want qemu to reconnect 
> and suck down
> >> fresh entropy, rather than hand the rngd process on the guest.
> >>
> >> I'm using the chardev 'socket' type to make the connection 
> to the host.
> >>
> >> Would a patch adding (optional) auto-reconnect (with a 
> back-off delay)
> >> to the socket chardev be acceptable ?
> >>
> >> If not, I'll have to cook up a thread to handle EGD 
> requests, but that
> >> seems perverse...
> >>   
> > 
> > Hrm, I'm not sure.  What are the circumstances that this connection
> > would die?  What happens while the connection is dead?
> 
> The most common would be the entropy gathering daemon being restarted,
> perhaps due to an upgrade. Its hardly a good idea to require all the
> guest VMs to reboot on this occuring. Next most common I 
> guess would be
> the daemon crashing, but again, not something you'd want 
> taking out your
> guest VMs...
> 
> Whilst the connection is down, the guests will potentially starve of
> entropy - but that only means they'll block processes that try to use
> /dev/random if they run out altogether.
> 
> Here are two patches that implement socket reconnection. This first
> cleans up the APIs needed a little  and the second implements 
> the guts.
> 
> If these patches are acceptable, I will repost my 4-patch series which
> also includes the SIZE parameter patch and an updated virtio-rng patch
> that uses the reconnection infrastructure to enhance its reliability.
> 
> -Ian
> 

Re: [Qemu-devel] Socket reconnection.

[Ian Molton]

Krumme, Chris wrote:
> Hello Ian,

Hi!

> Should you be introducing a while sleep loop into Qemu here?
> 
> I would think you should be returning 'no data', maybe after trying
> once.

Indeed.

I could have sworn I tried that. Too much crack, will re-do.

Re: [Qemu-devel] Socket reconnection.

[Anthony Liguori]

Krumme, Chris wrote:
> Hello Ian,
>
> Since you did not inline your source I will paste in a chunk:
>
>
> @@ -2030,10 +2036,18 @@ static void tcp_chr_read(void *opaque)
>          if (s->listen_fd >= 0) {
>              qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL,
> chr);
>          }
> -        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
> +        if (!s->reconnect)
> +            qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
>          closesocket(s->fd);
>          s->fd = -1;
> -        qemu_chr_event(chr, CHR_EVENT_CLOSED);
> +        if (!s->reconnect) {
> +            qemu_chr_event(chr, CHR_EVENT_CLOSED);
> +        } else {
> +            do {
> +                sleep(s->reconnect);
> +            } while(!qemu_chr_connect_socket(s));
> +            qemu_chr_event(chr, CHR_EVENT_RECONNECTED);
> +        }
>      } else if (size > 0) {
>          if (s->do_telnetopt)
>              tcp_chr_process_IAC_bytes(chr, s, buf, &size);
>
>
> Should you be introducing a while sleep loop into Qemu here?
>
> I would think you should be returning 'no data', maybe after trying
> once.
>
> Hope this helps.
>
> Chris
>   

sleep() in qemu is very, very wrong.  It will pause the guest's 
execution and all sorts of badness can ensue.

The right thing to do is set a timer and not generate data while 
disconnected.  I still am not confident this is really a great thing to do.

Regards,

Anthony Liguori

Re: [Qemu-devel] Socket reconnection.

[Ian Molton]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

Anthony Liguori wrote:

> sleep() in qemu is very, very wrong.  It will pause the guest's
> execution and all sorts of badness can ensue.

Quite...

> The right thing to do is set a timer and not generate data while
> disconnected.

New patch attached, now with less crack...

>  I still am not confident this is really a great thing to do.

What other option is there than to drop the ability to feed entropy to
the guest when the hosts egd link drops?

btw. Does anyone know how to get t-bird to inline patches?

-Ian

[-- Attachment #2: 0002-socket-Add-a-reconnect-option.patch --]
[-- Type: text/x-patch, Size: 9254 bytes --]

>From e9d4be9cd0ef9e34c65939d4604874035c45bf34 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian.molton@collabora.co.uk>
Date: Tue, 1 Dec 2009 11:18:41 +0000
Subject: [PATCH 2/4] socket: Add a reconnect option.

	Add a reconnect option that allows sockets to reconnect (after a
specified delay) to the specified server. This makes the virtio-rng driver
useful in production environments where the EGD server may need to be restarted.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 qemu-char.c   |  177 ++++++++++++++++++++++++++++++++++++++++++++-------------
 qemu-char.h   |    2 +
 qemu-config.c |    3 +
 vl.c          |    4 +
 4 files changed, 147 insertions(+), 39 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e202585..714c119 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1870,8 +1870,12 @@ typedef struct {
     int max_size;
     int do_telnetopt;
     int do_nodelay;
+    int reconnect;
     int is_unix;
     int msgfd;
+    QemuOpts *opts;
+    CharDriverState *chr;
+    int (*setup)(QemuOpts *opts);
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -2011,6 +2015,69 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
 }
 #endif
 
+struct reconnect_list {
+    TCPCharDriver *s;
+    uint64_t when;
+    struct reconnect_list *next;
+};
+
+static struct reconnect_list *rc_list;
+
+static int qemu_chr_sched_reconnect(TCPCharDriver *s)
+{
+    struct reconnect_list *new = qemu_malloc(sizeof(*new));
+    struct timeval tv;
+
+    if(!new)
+        return 1;
+
+    gettimeofday(&tv, NULL);
+    new->s = s;
+    new->when = (s->reconnect + tv.tv_sec) * 1000000 + tv.tv_usec;
+    new->next = rc_list;
+    rc_list = new;
+
+    return 0;
+}
+
+static int qemu_chr_connect_socket(TCPCharDriver *s);
+
+void qemu_chr_reconnect(void)
+{
+    struct reconnect_list *this = rc_list, *prev = NULL;
+    struct timeval tv;
+    uint64_t now;
+
+    if(!this)
+        return;
+
+    gettimeofday(&tv, NULL);
+    now = tv.tv_sec * 1000000 + tv.tv_usec;
+
+    while (this) {
+        if (this->when <= now) {
+            if(qemu_chr_connect_socket(this->s)) {
+                if(prev)
+                    prev->next = this->next;
+                else
+                    rc_list = NULL;
+                qemu_chr_event(this->s->chr, CHR_EVENT_RECONNECTED);
+                free(this);
+		if(prev)
+			this = prev;
+		else
+			this = NULL;
+            }
+            else {
+                this->when += this->s->reconnect * 1000000;
+            }
+        }
+        prev = this;
+	if(this)
+            this = this->next;
+    }
+}
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
@@ -2030,10 +2097,16 @@ static void tcp_chr_read(void *opaque)
         if (s->listen_fd >= 0) {
             qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
         }
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        if (!s->reconnect)
+            qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
         closesocket(s->fd);
         s->fd = -1;
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        if (!s->reconnect) {
+            qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        } else if (qemu_chr_sched_reconnect(s)) {
+            printf("Unable to queue socket for reconnection.\n");
+            qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        }
     } else if (size > 0) {
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
@@ -2137,7 +2210,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 {
     CharDriverState *chr = NULL;
     TCPCharDriver *s = NULL;
-    int fd = -1;
     int is_listen;
     int is_waitconnect;
     int do_nodelay;
@@ -2145,34 +2217,40 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     int is_telnet;
 
     is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    is_unix        = qemu_opt_get(opts, "path") != NULL;
+
     is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
     is_telnet      = qemu_opt_get_bool(opts, "telnet", 0);
     do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
-    is_unix        = qemu_opt_get(opts, "path") != NULL;
-    if (!is_listen)
+
+    if (!is_listen) {
         is_waitconnect = 0;
+    } else {
+        if (is_telnet)
+            s->do_telnetopt = 1;
+    }
+
 
-    chr = qemu_mallocz(sizeof(CharDriverState));
     s = qemu_mallocz(sizeof(TCPCharDriver));
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    s->opts = opts;
+
+    if (!is_listen && !is_telnet)
+        s->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
 
     if (is_unix) {
         if (is_listen) {
-            fd = unix_listen_opts(opts);
+            s->setup = unix_listen_opts;
         } else {
-            fd = unix_connect_opts(opts);
+            s->setup = unix_connect_opts;
         }
     } else {
         if (is_listen) {
-            fd = inet_listen_opts(opts, 0);
+            s->setup = inet_listen_opts;
         } else {
-            fd = inet_connect_opts(opts);
+            s->setup = inet_connect_opts;
         }
     }
-    if (fd < 0)
-        goto fail;
-
-    if (!is_waitconnect)
-        socket_set_nonblock(fd);
 
     s->connected = 0;
     s->fd = -1;
@@ -2186,19 +2264,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     chr->chr_close = tcp_chr_close;
     chr->get_msgfd = tcp_get_msgfd;
 
-    if (is_listen) {
-        s->listen_fd = fd;
-        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
-        if (is_telnet)
-            s->do_telnetopt = 1;
-
-    } else {
-        s->connected = 1;
-        s->fd = fd;
-        socket_set_nodelay(fd);
-        tcp_chr_connect(chr);
-    }
-
     /* for "info chardev" monitor command */
     chr->filename = qemu_malloc(256);
     if (is_unix) {
@@ -2215,22 +2280,56 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
                  qemu_opt_get_bool(opts, "server", 0) ? ",server" : "");
     }
 
-    if (is_listen && is_waitconnect) {
-        printf("QEMU waiting for connection on: %s\n",
-               chr->filename);
-        tcp_chr_accept(chr);
-        socket_set_nonblock(s->listen_fd);
-    }
-    return chr;
+    s->chr = chr;
+
+    if(qemu_chr_connect_socket(s))
+        return chr;
 
- fail:
-    if (fd >= 0)
-        closesocket(fd);
-    qemu_free(s);
     qemu_free(chr);
+    qemu_free(s);
+
     return NULL;
 }
 
+
+static int qemu_chr_connect_socket(TCPCharDriver *s)
+{
+    QemuOpts *opts = s->opts;
+    int is_listen;
+    int fd;
+    int is_waitconnect;
+    int do_nodelay;
+
+    is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
+    is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
+
+
+    fd = s->setup(s->opts);
+    if (fd < 0)
+        return 0;
+
+    if (!is_waitconnect)
+        socket_set_nonblock(fd);
+
+    if (is_listen) {
+        s->listen_fd = fd;
+        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, s->chr);
+        if (is_waitconnect) {
+            printf("QEMU waiting for connection on: %s\n",
+                   s->chr->filename);
+            tcp_chr_accept(s->chr);
+            socket_set_nonblock(s->listen_fd);
+        }
+    } else {
+        s->fd = fd;
+        socket_set_nodelay(fd);
+        tcp_chr_connect(s->chr);
+    }
+
+    return 1;
+}
+
 static QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
diff --git a/qemu-char.h b/qemu-char.h
index 9957db1..dc954e2 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -14,6 +14,7 @@
 #define CHR_EVENT_MUX_IN  3 /* mux-focus was set to this terminal */
 #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
 #define CHR_EVENT_CLOSED  5 /* connection closed */
+#define CHR_EVENT_RECONNECTED  6 /* reconnect event */
 
 
 #define CHR_IOCTL_SERIAL_SET_PARAMS   1
@@ -73,6 +74,7 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
                                     void (*init)(struct CharDriverState *s));
 CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*init)(struct CharDriverState *s));
 void qemu_chr_close(CharDriverState *chr);
+void qemu_chr_reconnect(void);
 void qemu_chr_printf(CharDriverState *s, const char *fmt, ...);
 int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len);
 void qemu_chr_send_event(CharDriverState *s, int event);
diff --git a/qemu-config.c b/qemu-config.c
index 590fc05..ff8b06e 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -140,6 +140,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "signal",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "reconnect",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end if list */ }
     },
diff --git a/vl.c b/vl.c
index 44763af..5876c3e 100644
--- a/vl.c
+++ b/vl.c
@@ -3795,6 +3795,10 @@ void main_loop_wait(int timeout)
 
     host_main_loop_wait(&timeout);
 
+    /* Reconnect any disconnected sockets, if necessary */
+
+    qemu_chr_reconnect();
+
     /* poll any events */
     /* XXX: separate device handlers from system ones */
     nfds = -1;
-- 
1.6.5

[Qemu-devel] Re: Socket reconnection.

[Jan Kiszka]

Ian Molton wrote:
> btw. Does anyone know how to get t-bird to inline patches?

I'm using Thunderbird for single patches. For a fairly efficient
workflow, I've installed Toggle Word Wrap [1] and defined a short-cut
for it (here: Alt-W). After copy&pasting a patch into the editor window,
I simply switch off the wrapping and can then send out a properly
formatted patch. The only pitfall is to forget to switch (some visual
indication of the wrapping mode in the status bar or so would be nice).

Jan

[1] https://addons.mozilla.org/de/firefox/addon/2351

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Socket reconnection.

[Anthony Liguori]

Ian Molton wrote:
> Anthony Liguori wrote:
>
>   
>> sleep() in qemu is very, very wrong.  It will pause the guest's
>> execution and all sorts of badness can ensue.
>>     
>
> Quite...
>
>   
>> The right thing to do is set a timer and not generate data while
>> disconnected.
>>     
>
> New patch attached, now with less crack...
>
>   
>>  I still am not confident this is really a great thing to do.
>>     
>
> What other option is there than to drop the ability to feed entropy to
> the guest when the hosts egd link drops?
>
> btw. Does anyone know how to get t-bird to inline patches?
>
> -Ian
>   

Ah, your mailer didn't attach it correctly...

Anyway, it's broken wrt CodingStyle.

Regards,

Anthony Liguori

RE: [Qemu-devel] Socket reconnection.

[Krumme, Chris]

Hello Ian,

Pasting chunks, then commenting:

+static int qemu_chr_sched_reconnect(TCPCharDriver *s)
+{
+    struct reconnect_list *new = qemu_malloc(sizeof(*new));
+    struct timeval tv;
+
+    if(!new)
+        return 1;


Qemu_malloc will never return 0, so sched function can return void.


+    while (this) {
+        if (this->when <= now) {
+            if(qemu_chr_connect_socket(this->s)) {
+                if(prev)
+                    prev->next = this->next;
+                else
+                    rc_list = NULL;
+                qemu_chr_event(this->s->chr, CHR_EVENT_RECONNECTED);
+                free(this);
+		if(prev)
+			this = prev;
+		else
+			this = NULL;
+            }
+            else {
+                this->when += this->s->reconnect * 1000000;
+            }
+        }
+        prev = this;
+	if(this)
+            this = this->next;

This is a mixture of tabs and spaces, for new code pick one.

The if(prev) can just be this = prev;  if prev is NULL you want NULL
anyway.

Thanks

Chris
 

> -----Original Message-----
> From: Ian Molton [mailto:ian.molton@collabora.co.uk] 
> Sent: Wednesday, December 02, 2009 4:41 AM
> To: Anthony Liguori
> Cc: Krumme, Chris; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] Socket reconnection.
> 
> Anthony Liguori wrote:
> 
> > sleep() in qemu is very, very wrong.  It will pause the guest's
> > execution and all sorts of badness can ensue.
> 
> Quite...
> 
> > The right thing to do is set a timer and not generate data while
> > disconnected.
> 
> New patch attached, now with less crack...
> 
> >  I still am not confident this is really a great thing to do.
> 
> What other option is there than to drop the ability to feed entropy to
> the guest when the hosts egd link drops?
> 
> btw. Does anyone know how to get t-bird to inline patches?
> 
> -Ian
> 

Re: [Qemu-devel] Socket reconnection.

[Ian Molton]

Krumme, Chris wrote:
> Hello Ian,

Hello!

> Qemu_malloc will never return 0, so sched function can return void.

Gah! nasty. Check removed.

> This is a mixture of tabs and spaces, for new code pick one.

I've been sticking with spaces generally. I can't find 'CodingStyle' so
I copied. I think spaces suck for this though. What is acceptable?
linux kernel style ?
 
> The if(prev) can just be this = prev;  if prev is NULL you want NULL
> anyway.

Quite. Fixed.

> Thanks

Likewise.

Fresh patch attached. Anthony, if this is ok, I can rebase this and its
prerequisite.

>From 05581c5badd693b7537fe57f85a2ff5ddcb7972d Mon Sep 17 00:00:00 2001
From: Ian Molton <ian.molton@collabora.co.uk>
Date: Tue, 1 Dec 2009 11:18:41 +0000
Subject: [PATCH 2/4] socket: Add a reconnect option.

	Add a reconnect option that allows sockets to reconnect (after a
specified delay) to the specified server. This makes the virtio-rng driver
useful in production environments where the EGD server may need to be restarted.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 qemu-char.c   |  169 ++++++++++++++++++++++++++++++++++++++++++++-------------
 qemu-char.h   |    2 +
 qemu-config.c |    3 +
 vl.c          |    4 ++
 4 files changed, 139 insertions(+), 39 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e202585..f20d697 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1870,8 +1870,12 @@ typedef struct {
     int max_size;
     int do_telnetopt;
     int do_nodelay;
+    int reconnect;
     int is_unix;
     int msgfd;
+    QemuOpts *opts;
+    CharDriverState *chr;
+    int (*setup)(QemuOpts *opts);
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -2011,6 +2015,61 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
 }
 #endif
 
+struct reconnect_list {
+    TCPCharDriver *s;
+    uint64_t when;
+    struct reconnect_list *next;
+};
+
+static struct reconnect_list *rc_list;
+
+static void qemu_chr_sched_reconnect(TCPCharDriver *s)
+{
+    struct reconnect_list *new = qemu_malloc(sizeof(*new));
+    struct timeval tv;
+
+    gettimeofday(&tv, NULL);
+    new->s = s;
+    new->when = (s->reconnect + tv.tv_sec) * 1000000 + tv.tv_usec;
+    new->next = rc_list;
+    rc_list = new;
+}
+
+static int qemu_chr_connect_socket(TCPCharDriver *s);
+
+void qemu_chr_reconnect(void)
+{
+    struct reconnect_list *this = rc_list, *prev = NULL;
+    struct timeval tv;
+    uint64_t now;
+
+    if (!this)
+        return;
+
+    gettimeofday(&tv, NULL);
+    now = tv.tv_sec * 1000000 + tv.tv_usec;
+
+    while (this) {
+        if (this->when <= now) {
+            if (qemu_chr_connect_socket(this->s)) {
+                if (prev)
+                    prev->next = this->next;
+                else
+                    rc_list = NULL;
+                qemu_chr_event(this->s->chr, CHR_EVENT_RECONNECTED);
+                free(this);
+                this = prev;
+            }
+            else {
+                this->when += this->s->reconnect * 1000000;
+            }
+        }
+        prev = this;
+	if (this)
+            this = this->next;
+    }
+}
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
@@ -2030,10 +2089,16 @@ static void tcp_chr_read(void *opaque)
         if (s->listen_fd >= 0) {
             qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
         }
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        if (!s->reconnect) {
+            qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        }
         closesocket(s->fd);
         s->fd = -1;
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        if (s->reconnect) {
+            qemu_chr_sched_reconnect(s);
+        } else {
+            qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        }
     } else if (size > 0) {
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
@@ -2137,7 +2202,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 {
     CharDriverState *chr = NULL;
     TCPCharDriver *s = NULL;
-    int fd = -1;
     int is_listen;
     int is_waitconnect;
     int do_nodelay;
@@ -2145,34 +2209,40 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     int is_telnet;
 
     is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    is_unix        = qemu_opt_get(opts, "path") != NULL;
+
     is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
     is_telnet      = qemu_opt_get_bool(opts, "telnet", 0);
     do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
-    is_unix        = qemu_opt_get(opts, "path") != NULL;
-    if (!is_listen)
+
+    if (!is_listen) {
         is_waitconnect = 0;
+    } else {
+        if (is_telnet)
+            s->do_telnetopt = 1;
+    }
+
 
-    chr = qemu_mallocz(sizeof(CharDriverState));
     s = qemu_mallocz(sizeof(TCPCharDriver));
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    s->opts = opts;
+
+    if (!is_listen && !is_telnet)
+        s->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
 
     if (is_unix) {
         if (is_listen) {
-            fd = unix_listen_opts(opts);
+            s->setup = unix_listen_opts;
         } else {
-            fd = unix_connect_opts(opts);
+            s->setup = unix_connect_opts;
         }
     } else {
         if (is_listen) {
-            fd = inet_listen_opts(opts, 0);
+            s->setup = inet_listen_opts;
         } else {
-            fd = inet_connect_opts(opts);
+            s->setup = inet_connect_opts;
         }
     }
-    if (fd < 0)
-        goto fail;
-
-    if (!is_waitconnect)
-        socket_set_nonblock(fd);
 
     s->connected = 0;
     s->fd = -1;
@@ -2186,19 +2256,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     chr->chr_close = tcp_chr_close;
     chr->get_msgfd = tcp_get_msgfd;
 
-    if (is_listen) {
-        s->listen_fd = fd;
-        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
-        if (is_telnet)
-            s->do_telnetopt = 1;
-
-    } else {
-        s->connected = 1;
-        s->fd = fd;
-        socket_set_nodelay(fd);
-        tcp_chr_connect(chr);
-    }
-
     /* for "info chardev" monitor command */
     chr->filename = qemu_malloc(256);
     if (is_unix) {
@@ -2215,22 +2272,56 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
                  qemu_opt_get_bool(opts, "server", 0) ? ",server" : "");
     }
 
-    if (is_listen && is_waitconnect) {
-        printf("QEMU waiting for connection on: %s\n",
-               chr->filename);
-        tcp_chr_accept(chr);
-        socket_set_nonblock(s->listen_fd);
-    }
-    return chr;
+    s->chr = chr;
+
+    if(qemu_chr_connect_socket(s))
+        return chr;
 
- fail:
-    if (fd >= 0)
-        closesocket(fd);
-    qemu_free(s);
     qemu_free(chr);
+    qemu_free(s);
+
     return NULL;
 }
 
+
+static int qemu_chr_connect_socket(TCPCharDriver *s)
+{
+    QemuOpts *opts = s->opts;
+    int is_listen;
+    int fd;
+    int is_waitconnect;
+    int do_nodelay;
+
+    is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
+    is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
+
+
+    fd = s->setup(s->opts);
+    if (fd < 0)
+        return 0;
+
+    if (!is_waitconnect)
+        socket_set_nonblock(fd);
+
+    if (is_listen) {
+        s->listen_fd = fd;
+        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, s->chr);
+        if (is_waitconnect) {
+            printf("QEMU waiting for connection on: %s\n",
+                   s->chr->filename);
+            tcp_chr_accept(s->chr);
+            socket_set_nonblock(s->listen_fd);
+        }
+    } else {
+        s->fd = fd;
+        socket_set_nodelay(fd);
+        tcp_chr_connect(s->chr);
+    }
+
+    return 1;
+}
+
 static QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
diff --git a/qemu-char.h b/qemu-char.h
index 9957db1..dc954e2 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -14,6 +14,7 @@
 #define CHR_EVENT_MUX_IN  3 /* mux-focus was set to this terminal */
 #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
 #define CHR_EVENT_CLOSED  5 /* connection closed */
+#define CHR_EVENT_RECONNECTED  6 /* reconnect event */
 
 
 #define CHR_IOCTL_SERIAL_SET_PARAMS   1
@@ -73,6 +74,7 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
                                     void (*init)(struct CharDriverState *s));
 CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*init)(struct CharDriverState *s));
 void qemu_chr_close(CharDriverState *chr);
+void qemu_chr_reconnect(void);
 void qemu_chr_printf(CharDriverState *s, const char *fmt, ...);
 int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len);
 void qemu_chr_send_event(CharDriverState *s, int event);
diff --git a/qemu-config.c b/qemu-config.c
index 590fc05..ff8b06e 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -140,6 +140,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "signal",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "reconnect",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end if list */ }
     },
diff --git a/vl.c b/vl.c
index 44763af..5876c3e 100644
--- a/vl.c
+++ b/vl.c
@@ -3795,6 +3795,10 @@ void main_loop_wait(int timeout)
 
     host_main_loop_wait(&timeout);
 
+    /* Reconnect any disconnected sockets, if necessary */
+
+    qemu_chr_reconnect();
+
     /* poll any events */
     /* XXX: separate device handlers from system ones */
     nfds = -1;
-- 
1.6.5

Re: [Qemu-devel] Socket reconnection.

[Kevin Wolf]

Am 03.12.2009 11:04, schrieb Ian Molton:
> Krumme, Chris wrote:
>> This is a mixture of tabs and spaces, for new code pick one.
> 
> I've been sticking with spaces generally. I can't find 'CodingStyle' so
> I copied. I think spaces suck for this though. What is acceptable?
> linux kernel style ?

Four spaces. The file is called CODING_STYLE and it's in the top level
directory.

Kevin

Re: [Qemu-devel] Socket reconnection.

[Anthony Liguori]

Ian Molton wrote:
> Fresh patch attached. Anthony, if this is ok, I can rebase this and its
> prerequisite.
>
> From 05581c5badd693b7537fe57f85a2ff5ddcb7972d Mon Sep 17 00:00:00 2001
> From: Ian Molton <ian.molton@collabora.co.uk>
> Date: Tue, 1 Dec 2009 11:18:41 +0000
> Subject: [PATCH 2/4] socket: Add a reconnect option.
>
> 	Add a reconnect option that allows sockets to reconnect (after a
> specified delay) to the specified server. This makes the virtio-rng driver
> useful in production environments where the EGD server may need to be restarted.
>
> Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
> ---
>  qemu-char.c   |  169 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  qemu-char.h   |    2 +
>  qemu-config.c |    3 +
>  vl.c          |    4 ++
>  4 files changed, 139 insertions(+), 39 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index e202585..f20d697 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1870,8 +1870,12 @@ typedef struct {
>      int max_size;
>      int do_telnetopt;
>      int do_nodelay;
> +    int reconnect;
>      int is_unix;
>      int msgfd;
> +    QemuOpts *opts;
> +    CharDriverState *chr;
> +    int (*setup)(QemuOpts *opts);
>  } TCPCharDriver;
>  
>  static void tcp_chr_accept(void *opaque);
> @@ -2011,6 +2015,61 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>  }
>  #endif
>  
> +struct reconnect_list {
>   

CODING_STYLE is off (as I mentioned before).

> +    TCPCharDriver *s;
> +    uint64_t when;
> +    struct reconnect_list *next;
> +};
> +
> +static struct reconnect_list *rc_list;
> +
> +static void qemu_chr_sched_reconnect(TCPCharDriver *s)
> +{
> +    struct reconnect_list *new = qemu_malloc(sizeof(*new));
> +    struct timeval tv;
> +
> +    gettimeofday(&tv, NULL);
>   

This will break Win32 (use qemu_gettimeofday).

> +    new->s = s;
> +    new->when = (s->reconnect + tv.tv_sec) * 1000000 + tv.tv_usec;
> +    new->next = rc_list;
> +    rc_list = new;
>   

Don't open code a list, use one of the sys-queue types.

> +}
> +
> +static int qemu_chr_connect_socket(TCPCharDriver *s);
>   

Forward declarations usually imply some form of code motion is required.

> +void qemu_chr_reconnect(void)
> +{
> +    struct reconnect_list *this = rc_list, *prev = NULL;
> +    struct timeval tv;
> +    uint64_t now;
> +
> +    if (!this)
> +        return;
> +
> +    gettimeofday(&tv, NULL);
> +    now = tv.tv_sec * 1000000 + tv.tv_usec;
> +
> +    while (this) {
> +        if (this->when <= now) {
> +            if (qemu_chr_connect_socket(this->s)) {
> +                if (prev)
> +                    prev->next = this->next;
> +                else
> +                    rc_list = NULL;
> +                qemu_chr_event(this->s->chr, CHR_EVENT_RECONNECTED);
> +                free(this);
> +                this = prev;
> +            }
> +            else {
> +                this->when += this->s->reconnect * 1000000;
> +            }
> +        }
> +        prev = this;
> +	if (this)
> +            this = this->next;
> +    }
> +}
> +
>   

Mixing tabs and spaces.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH]Socket reconnection.

[Ian Molton]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

Anthony Liguori wrote:

New patch attached, addressing the below. Thanks!

> CODING_STYLE is off (as I mentioned before).

Hopefully fixed now.

>> +    gettimeofday(&tv, NULL);
>>   
> 
> This will break Win32 (use qemu_gettimeofday).
 
Done

> Don't open code a list, use one of the sys-queue types.

I only wanted a singly linked list. However, changed to a QLIST

>> +static int qemu_chr_connect_socket(TCPCharDriver *s);
>>   
> 
> Forward declarations usually imply some form of code motion is required.

Cant see any way around it here. I've reordered it so the new code needs
the forward dec. rather than adding one for the old code.
 
> Mixing tabs and spaces.

fixed.

[-- Attachment #2: 0002-socket-Add-a-reconnect-option.patch --]
[-- Type: text/x-patch, Size: 8949 bytes --]

>From 8d12cabf09996eef9adcc121ea3cbec70d48b3b4 Mon Sep 17 00:00:00 2001
From: Ian Molton <ian.molton@collabora.co.uk>
Date: Tue, 1 Dec 2009 11:18:41 +0000
Subject: [PATCH 2/4] socket: Add a reconnect option.

	Add a reconnect option that allows sockets to reconnect (after a
specified delay) to the specified server. This makes the virtio-rng driver
useful in production environments where the EGD server may need to be restarted.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 qemu-char.c   |  159 +++++++++++++++++++++++++++++++++++++++++++--------------
 qemu-char.h   |    2 +
 qemu-config.c |    3 +
 vl.c          |    4 ++
 4 files changed, 129 insertions(+), 39 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e202585..45c9f3d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1870,8 +1870,12 @@ typedef struct {
     int max_size;
     int do_telnetopt;
     int do_nodelay;
+    int reconnect;
     int is_unix;
     int msgfd;
+    QemuOpts *opts;
+    CharDriverState *chr;
+    int (*setup)(QemuOpts *opts);
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -2011,6 +2015,8 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
 }
 #endif
 
+static void qemu_chr_sched_reconnect(TCPCharDriver *s);
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
@@ -2030,10 +2036,16 @@ static void tcp_chr_read(void *opaque)
         if (s->listen_fd >= 0) {
             qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
         }
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        if (!s->reconnect) {
+            qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        }
         closesocket(s->fd);
         s->fd = -1;
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        if (s->reconnect) {
+            qemu_chr_sched_reconnect(s);
+        } else {
+            qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        }
     } else if (size > 0) {
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
@@ -2133,11 +2145,92 @@ static void tcp_chr_close(CharDriverState *chr)
     qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
+static int qemu_chr_connect_socket(TCPCharDriver *s)
+{
+    QemuOpts *opts = s->opts;
+    int is_listen;
+    int fd;
+    int is_waitconnect;
+    int do_nodelay;
+
+    is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
+    is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
+
+
+    fd = s->setup(s->opts);
+    if (fd < 0)
+        return 0;
+
+    if (!is_waitconnect)
+        socket_set_nonblock(fd);
+
+    if (is_listen) {
+        s->listen_fd = fd;
+        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, s->chr);
+        if (is_waitconnect) {
+            printf("QEMU waiting for connection on: %s\n",
+                   s->chr->filename);
+            tcp_chr_accept(s->chr);
+            socket_set_nonblock(s->listen_fd);
+        }
+    } else {
+        s->fd = fd;
+        socket_set_nodelay(fd);
+        tcp_chr_connect(s->chr);
+    }
+
+    return 1;
+}
+
+static QLIST_HEAD(reconnect_list_head, reconnect_list_entry) rcl_head;
+
+typedef struct reconnect_list_entry {
+    TCPCharDriver *s;
+    uint64_t when;
+    QLIST_ENTRY(reconnect_list_entry) entries;
+} reconnect_list_entry;
+
+static void qemu_chr_sched_reconnect(TCPCharDriver *s)
+{
+    reconnect_list_entry *new = qemu_malloc(sizeof(*new));
+    struct timeval tv;
+
+    qemu_gettimeofday(&tv);
+    new->s = s;
+    new->when = (s->reconnect + tv.tv_sec) * 1000000 + tv.tv_usec;
+    QLIST_INSERT_HEAD(&rcl_head, new, entries);
+}
+
+void qemu_chr_reconnect(void)
+{
+    struct timeval tv;
+    uint64_t now;
+    reconnect_list_entry *np;
+
+    if (!rcl_head.lh_first)
+        return;
+
+    gettimeofday(&tv, NULL);
+    now = tv.tv_sec * 1000000 + tv.tv_usec;
+
+    for (np = rcl_head.lh_first; np != NULL; np = np->entries.le_next) {
+        if (np->when <= now) {
+            if (qemu_chr_connect_socket(np->s)) {
+                qemu_chr_event(np->s->chr, CHR_EVENT_RECONNECTED);
+                QLIST_REMOVE(np, entries);
+            }
+            else {
+                np->when += np->s->reconnect * 1000000;
+            }
+        }
+    }
+}
+
 static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 {
     CharDriverState *chr = NULL;
     TCPCharDriver *s = NULL;
-    int fd = -1;
     int is_listen;
     int is_waitconnect;
     int do_nodelay;
@@ -2145,34 +2238,40 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     int is_telnet;
 
     is_listen      = qemu_opt_get_bool(opts, "server", 0);
+    is_unix        = qemu_opt_get(opts, "path") != NULL;
+
     is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
     is_telnet      = qemu_opt_get_bool(opts, "telnet", 0);
     do_nodelay     = !qemu_opt_get_bool(opts, "delay", 1);
-    is_unix        = qemu_opt_get(opts, "path") != NULL;
-    if (!is_listen)
+
+    if (!is_listen) {
         is_waitconnect = 0;
+    } else {
+        if (is_telnet)
+            s->do_telnetopt = 1;
+    }
+
 
-    chr = qemu_mallocz(sizeof(CharDriverState));
     s = qemu_mallocz(sizeof(TCPCharDriver));
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    s->opts = opts;
+
+    if (!is_listen && !is_telnet)
+        s->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
 
     if (is_unix) {
         if (is_listen) {
-            fd = unix_listen_opts(opts);
+            s->setup = unix_listen_opts;
         } else {
-            fd = unix_connect_opts(opts);
+            s->setup = unix_connect_opts;
         }
     } else {
         if (is_listen) {
-            fd = inet_listen_opts(opts, 0);
+            s->setup = inet_listen_opts;
         } else {
-            fd = inet_connect_opts(opts);
+            s->setup = inet_connect_opts;
         }
     }
-    if (fd < 0)
-        goto fail;
-
-    if (!is_waitconnect)
-        socket_set_nonblock(fd);
 
     s->connected = 0;
     s->fd = -1;
@@ -2186,19 +2285,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     chr->chr_close = tcp_chr_close;
     chr->get_msgfd = tcp_get_msgfd;
 
-    if (is_listen) {
-        s->listen_fd = fd;
-        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
-        if (is_telnet)
-            s->do_telnetopt = 1;
-
-    } else {
-        s->connected = 1;
-        s->fd = fd;
-        socket_set_nodelay(fd);
-        tcp_chr_connect(chr);
-    }
-
     /* for "info chardev" monitor command */
     chr->filename = qemu_malloc(256);
     if (is_unix) {
@@ -2215,19 +2301,14 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
                  qemu_opt_get_bool(opts, "server", 0) ? ",server" : "");
     }
 
-    if (is_listen && is_waitconnect) {
-        printf("QEMU waiting for connection on: %s\n",
-               chr->filename);
-        tcp_chr_accept(chr);
-        socket_set_nonblock(s->listen_fd);
-    }
-    return chr;
+    s->chr = chr;
+
+    if(qemu_chr_connect_socket(s))
+        return chr;
 
- fail:
-    if (fd >= 0)
-        closesocket(fd);
-    qemu_free(s);
     qemu_free(chr);
+    qemu_free(s);
+
     return NULL;
 }
 
diff --git a/qemu-char.h b/qemu-char.h
index 9957db1..dc954e2 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -14,6 +14,7 @@
 #define CHR_EVENT_MUX_IN  3 /* mux-focus was set to this terminal */
 #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
 #define CHR_EVENT_CLOSED  5 /* connection closed */
+#define CHR_EVENT_RECONNECTED  6 /* reconnect event */
 
 
 #define CHR_IOCTL_SERIAL_SET_PARAMS   1
@@ -73,6 +74,7 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
                                     void (*init)(struct CharDriverState *s));
 CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*init)(struct CharDriverState *s));
 void qemu_chr_close(CharDriverState *chr);
+void qemu_chr_reconnect(void);
 void qemu_chr_printf(CharDriverState *s, const char *fmt, ...);
 int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len);
 void qemu_chr_send_event(CharDriverState *s, int event);
diff --git a/qemu-config.c b/qemu-config.c
index 590fc05..ff8b06e 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -140,6 +140,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "signal",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "reconnect",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end if list */ }
     },
diff --git a/vl.c b/vl.c
index 44763af..5876c3e 100644
--- a/vl.c
+++ b/vl.c
@@ -3795,6 +3795,10 @@ void main_loop_wait(int timeout)
 
     host_main_loop_wait(&timeout);
 
+    /* Reconnect any disconnected sockets, if necessary */
+
+    qemu_chr_reconnect();
+
     /* poll any events */
     /* XXX: separate device handlers from system ones */
     nfds = -1;
-- 
1.6.5

Re: [Qemu-devel] [PATCH]Socket reconnection.

[Ian Molton]

Ian Molton wrote:
> Anthony Liguori wrote:
> 
> New patch attached, addressing the below. Thanks!

Did this patch make it? I cant seem to find my earlier patches in the
.git repo you mentioned, does that mean they have been dropped, or that
they have been merged upstream ?

I can repost all four patches in dependency order if you prefer,
especially as the current version of the virtio-rng patch has been
modified to work with the socket reconnect support.

Thanks,

-Ian

Re: [Qemu-devel] Socket reconnection.

[Jamie Lokier]

Ian Molton wrote:
> Jamie Lokier wrote:
> 
> > I'm a bit puzzled.
> > 
> > Why isn't virtio-rng getting entropy from /dev/random on the host?
> 
> /dev/random may not be available.

Understood on a non-Linux host.

> Besides, not all entropy comes from /dev/random.

On a Linux host, why isn't rngd simply injecting it's entropy into
/dev/random where it would be more convenient to access?  (No need for
socket reconnection code, for example).

-- Jamie

Re: [Qemu-devel] Socket reconnection.

[Ian Molton]

Jamie Lokier wrote:
> Ian Molton wrote:
>> Jamie Lokier wrote:
>>
>>> I'm a bit puzzled.
>>>
>>> Why isn't virtio-rng getting entropy from /dev/random on the host?
>> /dev/random may not be available.
> 
> Understood on a non-Linux host.

Or a linux host with a user with insufficient privs...

>> Besides, not all entropy comes from /dev/random.
> 
> On a Linux host, why isn't rngd simply injecting it's entropy into
> /dev/random where it would be more convenient to access?  (No need for
> socket reconnection code, for example).

Who knows? lack of privs, an admin who only uses egd, a machine which is
being fed entropy by egd via a tunnel. User doesnt trust /dev/random,
/dev/random known to be failing FIPS tests on a shared machine - there
could be any number of reasons. In our case, entropy is comming from
hardware via egd, to be used in the guest VMs. why feed it into RNGD,
then the hosts entropy pool, THEN the guests - just feed them directly.
the egd daemon in this case also offers load balancing to all consumers
of entropy.

Since we need this on hosts without /dev/random anyway, I dont see why
we would need to deliberately cripple qemu on linux hosts...

-Ian

Re: [Qemu-devel] Socket reconnection.

[Jamie Lokier]

Ian Molton wrote:
> >> Besides, not all entropy comes from /dev/random.
> > 
> > On a Linux host, why isn't rngd simply injecting it's entropy into
> > /dev/random where it would be more convenient to access?  (No need for
> > socket reconnection code, for example).
> 
> Who knows? lack of privs, an admin who only uses egd, a machine which is
> being fed entropy by egd via a tunnel. User doesnt trust /dev/random,
> /dev/random known to be failing FIPS tests on a shared machine - there
> could be any number of reasons. In our case, entropy is comming from
> hardware via egd, to be used in the guest VMs. why feed it into RNGD,
> then the hosts entropy pool, THEN the guests - just feed them directly.
> the egd daemon in this case also offers load balancing to all consumers
> of entropy.

Those arguments are weak because they also apply the other way: an
admin who only uses /dev/random (got lots of those), a machine which
is being fed entropy into /dev/random via a tunnel (been there), user
doesn't trust egd (why should I trust it more than the kernel?
userspace is more easily corrupted - and with a socket, it might not
even be egd running), etc.

I grant you there are advantages sometimes, but also disadvantages.
Why would I run egd on a guest VM running an embedded system with 16MB
RAM, where every process is precious, for example?  But those systems
need entropy too.

As for why feed it "directly" via hardware -> egd -> guest", the
reason surely is because most clients read /dev/random or
/dev/urandom, so entropy that isn't injected into /dev/random is wasted.

Anyway, I've just read the rngd manual, and it does inject it's
entropy into /dev/random so that's the end of that discussion :-)

But the main issue is below.  On all host systems I have access to,
there is _no_ entropy available from egd/rngd...

> Since we need this on hosts without /dev/random anyway, I dont see why
> we would need to deliberately cripple qemu on linux hosts...

Since nobody has asked you to cripple anything, that is irrelevant.
Options don't cripple.

The lack of option is crippling qemu on linux hosts which don't run egd.

Which hosts are those?  Well, I've just checked five live systems,
four servers and one laptop, running:

    Ubuntu 9.10 (Karmic)  - no egd running, no rngd running
    Ubuntu 9.04 (Jaunty)  - no egd running, no rngd running
    Debian 4.0 (Etch)     - no egd running, no rngd running
    CentOS 4.0            - no egd running, no rngd running
    RHEL 4.0              - no egd running, no rngd running

So if I understand right, the virtio-rng host driver won't work on any
host system I have access to?  Is that correct?

Btw, /dev/random is available on virtually every host - Linux,
Solaris, FreeBSD, Mac OS X, NetBSD, OpenBSD, Tru64, HP-UX, AIX, and
there's even a similar device for Windows :-)

I grant you the egd/rngd entropy is preferable on a system that's
running it.  It is much more careful.  (Let's ignore this in rngd's
man page: "Do not do that unless you trust rngd's source of random
data").  So a good default would be to use the egd/rngd socket when
available, and fall back to /dev/random or even /dev/urandom (at user
request) when not.

My tests show that egd/rngd are not running on any host system I have
access to - and that's quite a diverse range of Linuxes.  It would
have to be run specially, which sounds problematic as qemu would be
the only process using it.

-- Jamie

Re: [Qemu-devel] Socket reconnection.

[Ian Molton]

Jamie Lokier wrote:
> Ian Molton wrote:
>>>> Besides, not all entropy comes from /dev/random.

> Those arguments are weak.

No worse than the counterarguments.

If nothing else, qemu is a useful tool for testing kernel subsystems and
in fact the virtio code triggered and caused to be fixed a number of
bugs / flaws in the kernels hw random core and virtio driver. That alone
I think shows it has value.

> I grant you there are advantages sometimes, but also disadvantages.
> Why would I run egd on a guest VM running an embedded system with 16MB
> RAM, where every process is precious, for example?  But those systems
> need entropy too.

I have no idea - too much crack? :-)

one might run rngd, or use /dev/hwrng directly, but I cant see why you'd
want to run egd on it.

> As for why feed it "directly" via hardware -> egd -> guest", the
> reason surely is because most clients read /dev/random or
> /dev/urandom, so entropy that isn't injected into /dev/random is wasted.

Im not proposing that. I'm saying the HOST machine would be getting
entropy from egd, directly into qemu. That entropy comes out of the
guests /dev/hwrng.

I can see value there too - the hosts /dev/random pool cannot be
compromised by the guest OS. But nothings stopping you specifying
/dev/random as qemus entropy source.

> But the main issue is below.  On all host systems I have access to,
> there is _no_ entropy available from egd/rngd...

I have no system here with that hardware either - however I had access
(via an ssh tunnel) to another machine that did have it, and could feed
qemu direct from that stream. qemu spoke egd over the tunnel, and fed
the guests /dev/hwrng. The guest was passing FIPS tests at the same
speed the hardware could produce entropy. Are you suggesting that I
should have fed the hosts entropy pool (via RNGD) and then fed that to
the guest ia qemu? why?

>> Since we need this on hosts without /dev/random anyway, I dont see why
>> we would need to deliberately cripple qemu on linux hosts...
>
> The lack of option is crippling qemu on linux hosts which don't run egd.

Sorry, what?

> Which hosts are those?  Well, I've just checked five live systems,
> four servers and one laptop, running:
> 
>     Ubuntu 9.10 (Karmic)  - no egd running, no rngd running
>     Ubuntu 9.04 (Jaunty)  - no egd running, no rngd running
>     Debian 4.0 (Etch)     - no egd running, no rngd running

apt-get install rng-tools

>     CentOS 4.0            - no egd running, no rngd running
>     RHEL 4.0              - no egd running, no rngd running

Presumably one can install rngd easily on these systems too.

I have a machine here that is not running X - does that make X worthless?

> So if I understand right, the virtio-rng host driver won't work on any
> host system I have access to?  Is that correct?

No, because it is just as happy reading raw data from /dev/random as it
it to speak EGD protocol over a socket. Have you actually tried the patch?

-Ian