[Qemu-devel] Re: [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Gerd Hoffmann]

> You've got whitespace damage.  You're mixing tabs and spaces.  Please
> only use spaces.

Fixed.

>> +    if ((size == -1 && EIO == errno) ||
>> +    (size == 0)) {
>>   
> 
> errno == EIO

Fixed.

>> +    /*
>> +     * Short timeout here: just need wait long enougth that qemu makes
>> +     * it through the poll loop once.  When reconnected we want a
>> +     * short timeout so we notice it almost instantly.  Otherwise
>> +     * read() gives us -EIO instantly, making pty_chr_state() reset the
>> +     * timeout to the normal (much longer) poll interval before the
>> +     * timer triggers.
>> +     */
>> +    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 10);
>>   
> 
> Perhaps what you really want here is a bottom half?

Looked promising, tried, doesn't work.  bh is called *before* the pty
file handle is checked at least once for being readable.

>> +    if (s->connected) {
>> +        fprintf(stderr,"%s: %s disconnected\n", __FUNCTION__,
>> ptsname(s->fd));
>>   
> 
> Obviously this debugging has to go.

Dropped.

>> +    /* Try reading again ... */
>> +#if 0
>> +    fprintf(stderr,"%s: %s checking ...\n", __FUNCTION__,
>> ptsname(s->fd));
>> +#endif

... this too while being at it ;)

>> +    if (!s) {
>> +        free(chr);
>>   
> 
> qemu_free()

Fixed.

New patch attached.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

>From 4362b66782ac68d8e1efb04fea1befeb20587677 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <address@hidden>
Date: Wed, 23 Jul 2008 16:35:40 +0200
Subject: [PATCH 03/23] Attempt to detect unconnected ptys.

This patch moves the pty char device imlementation away from the generic
filehandle code.  It tries to detect as good as possible whenever there
is someone connected to the slave pty device and only send data down the
road in case someone is listening.  Unfortunaly we have to poll via
timer once in a while to check the status because we have to use read()
on the master pty to figure the status (returns -EIO when unconnected).

Poll intervall for an idle guest is one second, when the guest sends
data to the virtual device linked to the pty we check more frequently.

The point for all of this is to avoid qemu blocking and not responding
any more.  Writing to the master pty handle succeeds even when nobody is
connected to (and reading from) to the slave end of the pty.  The kernel
just bufferes the writes.  And as soon as the kernel buffer is full the
write() call blocks forever ...

Signed-off-by: Gerd Hoffmann <address@hidden>
---
 vl.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 145 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 23b5243..8801615 100644
--- a/vl.c
+++ b/vl.c
@@ -2474,21 +2474,162 @@ void cfmakeraw (struct termios *termios_p)
 #endif
 
 #if defined(__linux__) || defined(__sun__)
+
+typedef struct {
+    int fd;
+    int connected;
+    int polling;
+    int read_bytes;
+    QEMUTimer *timer;
+} PtyCharDriver;
+
+static void pty_chr_update_read_handler(CharDriverState *chr);
+static void pty_chr_state(CharDriverState *chr, int connected);
+
+static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (!s->connected) {
+        /* guest sends data, check for (re-)connect */
+        pty_chr_update_read_handler(chr);
+        return 0;
+    }
+    return unix_write(s->fd, buf, len);
+}
+
+static int pty_chr_read_poll(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+
+    s->read_bytes = qemu_chr_can_read(chr);
+    return s->read_bytes;
+}
+
+static void pty_chr_read(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+    int size, len;
+    uint8_t buf[1024];
+
+    len = sizeof(buf);
+    if (len > s->read_bytes)
+        len = s->read_bytes;
+    if (len == 0)
+        return;
+    size = read(s->fd, buf, len);
+    if ((size == -1 && errno == EIO) ||
+        (size == 0)) {
+        pty_chr_state(chr, 0);
+        return;
+    }
+    if (size > 0) {
+        pty_chr_state(chr, 1);
+        qemu_chr_read(chr, buf, size);
+    }
+}
+
+static void pty_chr_update_read_handler(CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
+                         pty_chr_read, NULL, chr);
+    s->polling = 1;
+    /*
+     * Short timeout here: just need wait long enougth that qemu makes
+     * it through the poll loop once.  When reconnected we want a
+     * short timeout so we notice it almost instantly.  Otherwise
+     * read() gives us -EIO instantly, making pty_chr_state() reset the
+     * timeout to the normal (much longer) poll interval before the
+     * timer triggers.
+     */
+    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 10);
+}
+
+static void pty_chr_state(CharDriverState *chr, int connected)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    if (!connected) {
+        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+        s->connected = 0;
+        s->polling = 0;
+        /* (re-)connect poll interval for idle guests: once per second.
+         * We check more frequently in case the guests sends data to
+         * the virtual device linked to our pty. */
+        qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 1000);
+    } else {
+        if (!s->connected)
+            qemu_chr_reset(chr);
+        s->connected = 1;
+    }
+}
+
+void pty_chr_timer(void *opaque)
+{
+    struct CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+
+    if (s->connected)
+        return;
+    if (s->polling) {
+        /* If we arrive here without polling being cleared due
+         * read returning -EIO, then we are (re-)connected */
+        pty_chr_state(chr, 1);
+        return;
+    }
+
+    /* Next poll ... */
+    pty_chr_update_read_handler(chr);
+}
+
+static void pty_chr_close(struct CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    close(s->fd);
+    qemu_free(s);
+}
+
 static CharDriverState *qemu_chr_open_pty(void)
 {
+    CharDriverState *chr;
+    PtyCharDriver *s;
     struct termios tty;
-    int master_fd, slave_fd;
+    int slave_fd;
 
-    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
+    chr = qemu_mallocz(sizeof(CharDriverState));
+    if (!chr)
+        return NULL;
+    s = qemu_mallocz(sizeof(PtyCharDriver));
+    if (!s) {
+        qemu_free(chr);
+        return NULL;
+    }
+
+    if (openpty(&s->fd, &slave_fd, NULL, NULL, NULL) < 0) {
         return NULL;
     }
 
     /* Set raw attributes on the pty. */
     cfmakeraw(&tty);
     tcsetattr(slave_fd, TCSAFLUSH, &tty);
+    close(slave_fd);
+
+    fprintf(stderr, "char device redirected to %s\n", ptsname(s->fd));
 
-    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
-    return qemu_chr_open_fd(master_fd, master_fd);
+    chr->opaque = s;
+    chr->chr_write = pty_chr_write;
+    chr->chr_update_read_handler = pty_chr_update_read_handler;
+    chr->chr_close = pty_chr_close;
+
+    s->timer = qemu_new_timer(rt_clock, pty_chr_timer, chr);
+
+    return chr;
 }
 
 static void tty_serial_init(int fd, int speed,
-- 
1.5.4.1