[Qemu-devel] [PATCH 0/5] chardev, virtio-console: flow control, error handling

[Qemu-devel] [PATCH 0/5] chardev, virtio-console: flow control, error handling

[Amit Shah]

Hello,

(This series is based on the previous series that I've sent out for
virtio-serial).

This patch series adds fixes for the chardev interface to let callers
of qemu_chr_write() know how many bytes were sent and how many are
unsent in case of errors.

In case of EAGAIN, the unix_write() function just kept spinning while
attempting to write to the chardev till it succeeded. This resulted in
a stuck VM in case a chardev had opened connection but wasn't reading
anything from qemu.

There are two fixes for that case:
- Poll for POLLOUT instead of directly attempting write(), which helps
  relax the CPU (and we become greener).
- If the file that we're writing to is nonblocking, return -EAGAIN to
  the caller of qemu_chr_write() so that appropriate actions can be
  taken higher up in the stack. This is done when POLLOUT doesn't get
  set in a 10-ms timeout that's specified to poll.

Using these changes, the virtio-console code is modified to handle
slow remote connections by letting the virtio-serial-bus code know of
the traffic jam and throttle the port.

I've tested this with my auto-virtserial testsuite to check for data
sent from guest to a host socket. Throttling works fine.

Comments?

Amit Shah (5):
  char: Let the caller know how many bytes were written in case of
    incomplete writes
  char: unix write: Add some sleep to ease off spinning in a tight loop
  char: unix: For files that are nonblocking, report -EAGAIN to calling
    functions
  virtio-console: Factor out common init between console and generic
    ports
  virtio-console: Throttle virtio-serial-bus if we can't consume any
    more guest data

 console.c           |   10 ++--
 gdbstub.c           |   15 +++--
 hw/bt-hci-csr.c     |    8 ++--
 hw/debugcon.c       |    5 +-
 hw/msmouse.c        |    5 +-
 hw/parallel.c       |    5 ++-
 hw/serial.c         |    4 +-
 hw/usb-serial.c     |    4 +-
 hw/virtio-console.c |  146 ++++++++++++++++++++++++++++++++++++++++++++------
 monitor.c           |    4 +-
 net/socket.c        |    7 ++-
 qemu-char.c         |  125 ++++++++++++++++++++++++++++++--------------
 qemu-char.h         |    7 ++-
 qemu_socket.h       |    2 +-
 slirp/slirp.c       |   12 +++--
 15 files changed, 268 insertions(+), 91 deletions(-)

[Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Amit Shah]

There might be cases where a few bytes would have been sent out to char
devices and some not. Currently the return values from qemu_chr_write()
to char devs are only -1, indicating an error, or the complete length
of the string passed.

Make 'len' a pointer instead, and indicate how much of the string was
written. The return value will either be the same as 'len' or a negative
number indicating an error condition.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 console.c           |   10 +++---
 gdbstub.c           |   15 +++++---
 hw/bt-hci-csr.c     |    8 ++--
 hw/debugcon.c       |    5 ++-
 hw/msmouse.c        |    5 ++-
 hw/parallel.c       |    5 ++-
 hw/serial.c         |    4 ++-
 hw/usb-serial.c     |    4 +-
 hw/virtio-console.c |    2 +-
 monitor.c           |    4 ++-
 net/socket.c        |    7 +++-
 qemu-char.c         |   96 +++++++++++++++++++++++++++++++-------------------
 qemu-char.h         |    7 +++-
 qemu_socket.h       |    2 +-
 slirp/slirp.c       |   12 ++++---
 15 files changed, 114 insertions(+), 72 deletions(-)

diff --git a/console.c b/console.c
index 8bcd00b..058264c 100644
--- a/console.c
+++ b/console.c
@@ -1078,7 +1078,7 @@ void console_select(unsigned int index)
     }
 }
 
-static int console_puts(CharDriverState *chr, const uint8_t *buf, int len)
+static int console_puts(CharDriverState *chr, const uint8_t *buf, size_t *len)
 {
     TextConsole *s = chr->opaque;
     int i;
@@ -1088,7 +1088,7 @@ static int console_puts(CharDriverState *chr, const uint8_t *buf, int len)
     s->update_x1 = 0;
     s->update_y1 = 0;
     console_show_cursor(s, 0);
-    for(i = 0; i < len; i++) {
+    for(i = 0; i < *len; i++) {
         console_putchar(s, buf[i]);
     }
     console_show_cursor(s, 1);
@@ -1097,7 +1097,7 @@ static int console_puts(CharDriverState *chr, const uint8_t *buf, int len)
                    s->update_x1 - s->update_x0,
                    s->update_y1 - s->update_y0);
     }
-    return len;
+    return *len;
 }
 
 static void console_send_event(CharDriverState *chr, int event)
@@ -1498,11 +1498,11 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds, QemuOpt
 
     if (chr->label) {
         char msg[128];
-        int len;
+        size_t len;
 
         s->t_attrib.bgcol = COLOR_BLUE;
         len = snprintf(msg, sizeof(msg), "%s console\r\n", chr->label);
-        console_puts(chr, (uint8_t*)msg, len);
+        console_puts(chr, (uint8_t*)msg, &len);
         s->t_attrib = s->t_attrib_default;
     }
 
diff --git a/gdbstub.c b/gdbstub.c
index 93c4850..1af8c17 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -376,7 +376,9 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int len)
         }
     }
 #else
-    qemu_chr_write(s->chr, buf, len);
+    size_t tmplen = len;
+
+    qemu_chr_write(s->chr, buf, &tmplen);
 #endif
 }
 
@@ -2595,22 +2597,23 @@ static void gdb_monitor_output(GDBState *s, const char *msg, int len)
     put_packet(s, buf);
 }
 
-static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf, int len)
+static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf,
+			     size_t *len)
 {
     const char *p = (const char *)buf;
     int max_sz;
 
     max_sz = (sizeof(gdbserver_state->last_packet) - 2) / 2;
     for (;;) {
-        if (len <= max_sz) {
-            gdb_monitor_output(gdbserver_state, p, len);
+        if (*len <= max_sz) {
+            gdb_monitor_output(gdbserver_state, p, *len);
             break;
         }
         gdb_monitor_output(gdbserver_state, p, max_sz);
         p += max_sz;
-        len -= max_sz;
+        *len -= max_sz;
     }
-    return len;
+    return *len;
 }
 
 #ifndef _WIN32
diff --git a/hw/bt-hci-csr.c b/hw/bt-hci-csr.c
index 7300ea6..741c8ff 100644
--- a/hw/bt-hci-csr.c
+++ b/hw/bt-hci-csr.c
@@ -296,7 +296,7 @@ static int csrhci_data_len(const uint8_t *pkt)
 }
 
 static int csrhci_write(struct CharDriverState *chr,
-                const uint8_t *buf, int len)
+                const uint8_t *buf, size_t *len)
 {
     struct csrhci_s *s = (struct csrhci_s *) chr->opaque;
     int plen = s->in_len;
@@ -304,8 +304,8 @@ static int csrhci_write(struct CharDriverState *chr,
     if (!s->enable)
         return 0;
 
-    s->in_len += len;
-    memcpy(s->inpkt + plen, buf, len);
+    s->in_len += *len;
+    memcpy(s->inpkt + plen, buf, *len);
 
     while (1) {
         if (s->in_len >= 2 && plen < 2)
@@ -326,7 +326,7 @@ static int csrhci_write(struct CharDriverState *chr,
             break;
     }
 
-    return len;
+    return *len;
 }
 
 static void csrhci_out_hci_packet_event(void *opaque,
diff --git a/hw/debugcon.c b/hw/debugcon.c
index d549091..88cca11 100644
--- a/hw/debugcon.c
+++ b/hw/debugcon.c
@@ -46,15 +46,16 @@ static void debugcon_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
     DebugconState *s = opaque;
     unsigned char ch = val;
+    size_t tmplen;
 
 #ifdef DEBUG_DEBUGCON
     printf("debugcon: write addr=0x%04x val=0x%02x\n", addr, val);
 #endif
 
-    qemu_chr_write(s->chr, &ch, 1);
+    tmplen = 1;
+    qemu_chr_write(s->chr, &ch, &tmplen);
 }
 
-
 static uint32_t debugcon_ioport_read(void *opaque, uint32_t addr)
 {
     DebugconState *s = opaque;
diff --git a/hw/msmouse.c b/hw/msmouse.c
index 05f893c..68a848a 100644
--- a/hw/msmouse.c
+++ b/hw/msmouse.c
@@ -53,10 +53,11 @@ static void msmouse_event(void *opaque,
     qemu_chr_read(chr, bytes, 4);
 }
 
-static int msmouse_chr_write (struct CharDriverState *s, const uint8_t *buf, int len)
+static int msmouse_chr_write (struct CharDriverState *s, const uint8_t *buf,
+			      size_t *len)
 {
     /* Ignore writes to mouse port */
-    return len;
+    return *len;
 }
 
 static void msmouse_chr_close (struct CharDriverState *chr)
diff --git a/hw/parallel.c b/hw/parallel.c
index 12693d4..b9055ab 100644
--- a/hw/parallel.c
+++ b/hw/parallel.c
@@ -116,10 +116,13 @@ parallel_ioport_write_sw(void *opaque, uint32_t addr, uint32_t val)
             s->status |= PARA_STS_ERROR;
         }
         else if (val & PARA_CTR_SELECT) {
+            size_t tmplen;
+
+            tmplen = 1;
             if (val & PARA_CTR_STROBE) {
                 s->status &= ~PARA_STS_BUSY;
                 if ((s->control & PARA_CTR_STROBE) == 0)
-                    qemu_chr_write(s->chr, &s->dataw, 1);
+                    qemu_chr_write(s->chr, &s->dataw, &tmplen);
             } else {
                 if (s->control & PARA_CTR_INTEN) {
                     s->irq_pending = 1;
diff --git a/hw/serial.c b/hw/serial.c
index 90213c4..8d69a4a 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -311,6 +311,7 @@ static void serial_xmit(void *opaque)
 {
     SerialState *s = opaque;
     uint64_t new_xmit_ts = qemu_get_clock(vm_clock);
+    size_t tmplen;
 
     if (s->tsr_retry <= 0) {
         if (s->fcr & UART_FCR_FE) {
@@ -323,10 +324,11 @@ static void serial_xmit(void *opaque)
         }
     }
 
+    tmplen = 1;
     if (s->mcr & UART_MCR_LOOP) {
         /* in loopback mode, say that we just received a char */
         serial_receive1(s, &s->tsr, 1);
-    } else if (qemu_chr_write(s->chr, &s->tsr, 1) != 1) {
+    } else if (qemu_chr_write(s->chr, &s->tsr, &tmplen) != 1) {
         if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
             s->tsr_retry++;
             qemu_mod_timer(s->transmit_timer,  new_xmit_ts + s->char_transmit_time);
diff --git a/hw/usb-serial.c b/hw/usb-serial.c
index 69f0e44..4b3eaa9 100644
--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -426,14 +426,14 @@ static int usb_serial_handle_data(USBDevice *dev, USBPacket *p)
     int ret = 0;
     uint8_t devep = p->devep;
     uint8_t *data = p->data;
-    int len = p->len;
+    size_t len = p->len;
     int first_len;
 
     switch (p->pid) {
     case USB_TOKEN_OUT:
         if (devep != 2)
             goto fail;
-        qemu_chr_write(s->cs, data, len);
+        qemu_chr_write(s->cs, data, &len);
         break;
 
     case USB_TOKEN_IN:
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index caea11f..144efce 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -24,7 +24,7 @@ static void flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    qemu_chr_write(vcon->chr, buf, len);
+    qemu_chr_write(vcon->chr, buf, &len);
 }
 
 /* Readiness of the guest to accept data on a port */
diff --git a/monitor.c b/monitor.c
index f1b4e6b..4a0e1b6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -232,7 +232,9 @@ static int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
 void monitor_flush(Monitor *mon)
 {
     if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
-        qemu_chr_write(mon->chr, mon->outbuf, mon->outbuf_index);
+	size_t tmplen = mon->outbuf_index;
+
+        qemu_chr_write(mon->chr, mon->outbuf, &tmplen);
         mon->outbuf_index = 0;
     }
 }
diff --git a/net/socket.c b/net/socket.c
index 1c4e153..f1337fa 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -54,10 +54,13 @@ static ssize_t net_socket_receive(VLANClientState *nc, const uint8_t *buf, size_
 {
     NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
     uint32_t len;
+    size_t len_len;
+
     len = htonl(size);
+    len_len = sizeof(len);
 
-    send_all(s->fd, (const uint8_t *)&len, sizeof(len));
-    return send_all(s->fd, buf, size);
+    send_all(s->fd, (const uint8_t *)&len, &len_len);
+    return send_all(s->fd, buf, &size);
 }
 
 static ssize_t net_socket_receive_dgram(VLANClientState *nc, const uint8_t *buf, size_t size)
diff --git a/qemu-char.c b/qemu-char.c
index 048da3f..d5b1662 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -130,7 +130,7 @@ void qemu_chr_generic_open(CharDriverState *s)
     }
 }
 
-int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len)
+int qemu_chr_write(CharDriverState *s, const uint8_t *buf, size_t *len)
 {
     return s->chr_write(s, buf, len);
 }
@@ -169,9 +169,12 @@ void qemu_chr_printf(CharDriverState *s, const char *fmt, ...)
 {
     char buf[READ_BUF_LEN];
     va_list ap;
+    size_t len;
+
     va_start(ap, fmt);
     vsnprintf(buf, sizeof(buf), fmt, ap);
-    qemu_chr_write(s, (uint8_t *)buf, strlen(buf));
+    len = strlen(buf);
+    qemu_chr_write(s, (uint8_t *)buf, &len);
     va_end(ap);
 }
 
@@ -195,9 +198,9 @@ void qemu_chr_add_handlers(CharDriverState *s,
         s->chr_update_read_handler(s);
 }
 
-static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+static int null_chr_write(CharDriverState *chr, const uint8_t *buf, size_t *len)
 {
-    return len;
+    return *len;
 }
 
 static CharDriverState *qemu_chr_open_null(QemuOpts *opts)
@@ -235,7 +238,7 @@ typedef struct {
 } MuxDriver;
 
 
-static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, size_t *len)
 {
     MuxDriver *d = chr->opaque;
     int ret;
@@ -245,7 +248,9 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
         int i;
 
         ret = 0;
-        for (i = 0; i < len; i++) {
+        for (i = 0; i < *len; i++) {
+            size_t tmplen;
+
             if (d->linestart) {
                 char buf1[64];
                 int64_t ti;
@@ -262,10 +267,12 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
                          (secs / 60) % 60,
                          secs % 60,
                          (int)(ti % 1000));
-                d->drv->chr_write(d->drv, (uint8_t *)buf1, strlen(buf1));
+                tmplen = strlen(buf1);
+                d->drv->chr_write(d->drv, (uint8_t *)buf1, &tmplen);
                 d->linestart = 0;
             }
-            ret += d->drv->chr_write(d->drv, buf+i, 1);
+            tmplen = 1;
+            ret += d->drv->chr_write(d->drv, buf+i, &tmplen);
             if (buf[i] == '\n') {
                 d->linestart = 1;
             }
@@ -291,6 +298,7 @@ static void mux_print_help(CharDriverState *chr)
     int i, j;
     char ebuf[15] = "Escape-Char";
     char cbuf[50] = "\n\r";
+    size_t tmplen;
 
     if (term_escape_char > 0 && term_escape_char < 26) {
         snprintf(cbuf, sizeof(cbuf), "\n\r");
@@ -300,13 +308,18 @@ static void mux_print_help(CharDriverState *chr)
                  "\n\rEscape-Char set to Ascii: 0x%02x\n\r\n\r",
                  term_escape_char);
     }
-    chr->chr_write(chr, (uint8_t *)cbuf, strlen(cbuf));
+    tmplen = strlen(cbuf);
+    chr->chr_write(chr, (uint8_t *)cbuf, &tmplen);
     for (i = 0; mux_help[i] != NULL; i++) {
         for (j=0; mux_help[i][j] != '\0'; j++) {
-            if (mux_help[i][j] == '%')
-                chr->chr_write(chr, (uint8_t *)ebuf, strlen(ebuf));
-            else
-                chr->chr_write(chr, (uint8_t *)&mux_help[i][j], 1);
+
+            if (mux_help[i][j] == '%') {
+                tmplen = strlen(ebuf);
+                chr->chr_write(chr, (uint8_t *)ebuf, &tmplen);
+            } else {
+                tmplen = 1;
+                chr->chr_write(chr, (uint8_t *)&mux_help[i][j], &tmplen);
+            }
         }
     }
 }
@@ -331,7 +344,9 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
         case 'x':
             {
                  const char *term =  "QEMU: Terminated\n\r";
-                 chr->chr_write(chr,(uint8_t *)term,strlen(term));
+                 size_t tmplen = strlen(term);
+
+                 chr->chr_write(chr, (uint8_t *)term, &tmplen);
                  exit(0);
                  break;
             }
@@ -470,13 +485,14 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
 
 
 #ifdef _WIN32
-int send_all(int fd, const void *buf, int len1)
+int send_all(int fd, const void *buf, size_t *len)
 {
-    int ret, len;
+    ssize_t tmplen, ret;
 
-    len = len1;
-    while (len > 0) {
-        ret = send(fd, buf, len, 0);
+    tmplen = *len;
+    *len = 0;
+    while (tmplen > 0) {
+        ret = send(fd, buf, tmplen, 0);
         if (ret < 0) {
             errno = WSAGetLastError();
             if (errno != WSAEWOULDBLOCK) {
@@ -486,7 +502,8 @@ int send_all(int fd, const void *buf, int len1)
             break;
         } else {
             buf += ret;
-            len -= ret;
+            *len += ret;
+            tmplen -= ret;
         }
     }
     return len1 - len;
@@ -494,13 +511,14 @@ int send_all(int fd, const void *buf, int len1)
 
 #else
 
-static int unix_write(int fd, const uint8_t *buf, int len1)
+static int unix_write(int fd, const uint8_t *buf, size_t *len)
 {
-    int ret, len;
+    ssize_t tmplen, ret;
 
-    len = len1;
-    while (len > 0) {
-        ret = write(fd, buf, len);
+    tmplen = *len;
+    *len = 0;
+    while (tmplen > 0) {
+        ret = write(fd, buf, tmplen);
         if (ret < 0) {
             if (errno != EINTR && errno != EAGAIN)
                 return -1;
@@ -508,15 +526,16 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
             break;
         } else {
             buf += ret;
-            len -= ret;
+            *len += ret;
+            tmplen -= ret;
         }
     }
-    return len1 - len;
+    return *len;
 }
 
-int send_all(int fd, const void *buf, int len1)
+int send_all(int fd, const void *buf, size_t *len)
 {
-    return unix_write(fd, buf, len1);
+    return unix_write(fd, buf, len);
 }
 #endif /* !_WIN32 */
 
@@ -530,7 +549,7 @@ typedef struct {
 #define STDIO_MAX_CLIENTS 1
 static int stdio_nb_clients = 0;
 
-static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, size_t *len)
 {
     FDCharDriver *s = chr->opaque;
     return send_all(s->fd_out, buf, len);
@@ -836,7 +855,7 @@ typedef struct {
 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)
+static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, size_t *len)
 {
     PtyCharDriver *s = chr->opaque;
 
@@ -1791,11 +1810,11 @@ typedef struct {
     int max_size;
 } NetCharDriver;
 
-static int udp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+static int udp_chr_write(CharDriverState *chr, const uint8_t *buf, size_t *len)
 {
     NetCharDriver *s = chr->opaque;
 
-    return send(s->fd, (const void *)buf, len, 0);
+    return send(s->fd, (const void *)buf, *len, 0);
 }
 
 static int udp_chr_read_poll(void *opaque)
@@ -1906,14 +1925,14 @@ typedef struct {
 
 static void tcp_chr_accept(void *opaque);
 
-static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, size_t *len)
 {
     TCPCharDriver *s = chr->opaque;
     if (s->connected) {
         return send_all(s->fd, buf, len);
     } else {
         /* XXX: indicate an error ? */
-        return len;
+        return *len;
     }
 }
 
@@ -2201,8 +2220,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     if (fd < 0)
         goto fail;
 
-    if (!is_waitconnect)
+    if (!is_waitconnect) {
         socket_set_nonblock(fd);
+    }
 
     s->connected = 0;
     s->fd = -1;
@@ -2225,7 +2245,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     } else {
         s->connected = 1;
         s->fd = fd;
-        socket_set_nodelay(fd);
+        if (s->do_nodelay) {
+            socket_set_nodelay(fd);
+        }
         tcp_chr_connect(chr);
     }
 
diff --git a/qemu-char.h b/qemu-char.h
index 3a9427b..8d964f0 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_CHAR_H
 #define QEMU_CHAR_H
 
+#include <stdbool.h>
+
 #include "qemu-common.h"
 #include "qemu-queue.h"
 #include "qemu-option.h"
@@ -52,7 +54,7 @@ typedef void IOEventHandler(void *opaque, int event);
 
 struct CharDriverState {
     void (*init)(struct CharDriverState *s);
-    int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
+    int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, size_t *len);
     void (*chr_update_read_handler)(struct CharDriverState *s);
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
     int (*get_msgfd)(struct CharDriverState *s);
@@ -67,6 +69,7 @@ struct CharDriverState {
     QEMUBH *bh;
     char *label;
     char *filename;
+    bool nonblock;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
@@ -76,7 +79,7 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
 CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*init)(struct CharDriverState *s));
 void qemu_chr_close(CharDriverState *chr);
 void qemu_chr_printf(CharDriverState *s, const char *fmt, ...);
-int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len);
+int qemu_chr_write(CharDriverState *s, const uint8_t *buf, size_t *len);
 void qemu_chr_send_event(CharDriverState *s, int event);
 void qemu_chr_add_handlers(CharDriverState *s,
                            IOCanReadHandler *fd_can_read,
diff --git a/qemu_socket.h b/qemu_socket.h
index 7ee46ac..004b104 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -35,7 +35,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
 int qemu_socket(int domain, int type, int protocol);
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
 void socket_set_nonblock(int fd);
-int send_all(int fd, const void *buf, int len1);
+int send_all(int fd, const void *buf, size_t *len);
 
 /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
 int inet_listen_opts(QemuOpts *opts, int port_offset);
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 3c785cd..572b109 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -798,12 +798,14 @@ int slirp_add_exec(Slirp *slirp, int do_pty, const void *args,
 
 ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags)
 {
-	if (so->s == -1 && so->extra) {
-		qemu_chr_write(so->extra, buf, len);
-		return len;
-	}
+    if (so->s == -1 && so->extra) {
+        size_t tmplen = len;
+
+        qemu_chr_write(so->extra, buf, &tmplen);
+        return len;
+    }
 
-	return send(so->s, buf, len, flags);
+    return send(so->s, buf, len, flags);
 }
 
 static struct socket *
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] char: unix write: Add some sleep to ease off spinning in a tight loop

[Amit Shah]

When the other end of a chardev connection isn't picking up data as
fast as we're sending, we just used to keep spinning in a tight loop
till all the data was sent out.

Polling for POLLOUT indefinitely gives the other end a chance to catch
up and also saves us CPU cycles.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index d5b1662..48e1e57 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -36,6 +36,7 @@
 
 #include <unistd.h>
 #include <fcntl.h>
+#include <poll.h>
 #include <signal.h>
 #include <time.h>
 #include <errno.h>
@@ -513,15 +514,27 @@ int send_all(int fd, const void *buf, size_t *len)
 
 static int unix_write(int fd, const uint8_t *buf, size_t *len)
 {
+    struct pollfd pollfds[1];
     ssize_t tmplen, ret;
 
+    pollfds[0].fd = fd;
+    pollfds[0].events = POLLOUT;
+
     tmplen = *len;
     *len = 0;
     while (tmplen > 0) {
+        ret = poll(pollfds, 1, -1);
+        if (ret == -1) {
+            if (errno == EINTR) {
+                continue;
+            }
+            return -1;
+        }
         ret = write(fd, buf, tmplen);
         if (ret < 0) {
-            if (errno != EINTR && errno != EAGAIN)
-                return -1;
+            if (errno != EINTR && errno != EAGAIN) {
+                 return -1;
+            }
         } else if (ret == 0) {
             break;
         } else {
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/5] char: unix: For files that are nonblocking, report -EAGAIN to calling functions

[Amit Shah]

If the chardev we're writing to is nonblocking, just report -EAGAIN to
the caller so that the caller can take any further action as it may see
fit.

Modify poll call for polling for a timeout of 10ms instead of waiting
indefinitely for POLLOUT to get set.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 48e1e57..b565872 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -516,6 +516,15 @@ static int unix_write(int fd, const uint8_t *buf, size_t *len)
 {
     struct pollfd pollfds[1];
     ssize_t tmplen, ret;
+    int flags;
+    bool nonblock;
+
+    flags = fcntl(fd, F_GETFL);
+    if (flags == -1) {
+        flags = 0;
+    }
+
+    nonblock = flags & O_NONBLOCK;
 
     pollfds[0].fd = fd;
     pollfds[0].events = POLLOUT;
@@ -523,13 +532,16 @@ static int unix_write(int fd, const uint8_t *buf, size_t *len)
     tmplen = *len;
     *len = 0;
     while (tmplen > 0) {
-        ret = poll(pollfds, 1, -1);
+        ret = poll(pollfds, 1, 10);
         if (ret == -1) {
             if (errno == EINTR) {
                 continue;
             }
             return -1;
         }
+        if (ret == 0 && nonblock) {
+            return -EAGAIN;
+        }
         ret = write(fd, buf, tmplen);
         if (ret < 0) {
             if (errno != EINTR && errno != EAGAIN) {
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] virtio-console: Factor out common init between console and generic ports

[Amit Shah]

The initialisation for generic ports and console ports is similar.
Factor out the parts that are the same in a different function that can
be called from each of the initfns.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 144efce..dc1f75f 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -58,24 +58,28 @@ static void chr_event(void *opaque, int event)
     }
 }
 
-/* Virtio Console Ports */
-static int virtconsole_initfn(VirtIOSerialDevice *dev)
+static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
-    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    port->info = dev->info;
-
-    port->is_console = true;
+    vcon->port.info = dev->info;
 
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
-        port->info->have_data = flush_buf;
+        vcon->port.info->have_data = flush_buf;
     }
     return 0;
 }
 
+/* Virtio Console Ports */
+static int virtconsole_initfn(VirtIOSerialDevice *dev)
+{
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    port->is_console = true;
+    return generic_port_init(vcon, dev);
+}
+
 static int virtconsole_exitfn(VirtIOSerialDevice *dev)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
@@ -115,14 +119,7 @@ static int virtserialport_initfn(VirtIOSerialDevice *dev)
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    port->info = dev->info;
-
-    if (vcon->chr) {
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
-        port->info->have_data = flush_buf;
-    }
-    return 0;
+    return generic_port_init(vcon, dev);
 }
 
 static VirtIOSerialPortInfo virtserialport_info = {
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/5] virtio-console: Throttle virtio-serial-bus if we can't consume any more guest data

[Amit Shah]

If the char device we're connected to is overwhelmed with data and it
can't accept any more, signal to the virtio-serial-bus to stop sending
us more data till we tell otherwise.

If the current buffer being processed hasn't been completely written out
to the char device, we have to keep it around and re-try sending it
since the virtio-serial-bus code assumes we consume the entire buffer.

Also register with savevm so that we save/restore such a buffer across
migration.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |  119 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 116 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index dc1f75f..91c5171 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -13,18 +13,84 @@
 #include "qemu-char.h"
 #include "virtio-serial.h"
 
+typedef struct Buffer {
+    uint8_t *buf;
+    size_t rem_len;
+    size_t offset;
+} Buffer;
+
 typedef struct VirtConsole {
     VirtIOSerialPort port;
     CharDriverState *chr;
+    QEMUTimer *host_timer;
+    Buffer *unflushed_buf;
 } VirtConsole;
 
+static void add_unflushed_buf(VirtConsole *vcon, const uint8_t *buf, size_t len)
+{
+    vcon->unflushed_buf = qemu_malloc(sizeof(Buffer));
+    vcon->unflushed_buf->buf = qemu_malloc(len);
+
+    memcpy(vcon->unflushed_buf->buf, buf, len);
+    vcon->unflushed_buf->rem_len = len;
+    vcon->unflushed_buf->offset = 0;
+}
+
+static void free_unflushed_buf(VirtConsole *vcon)
+{
+    qemu_free(vcon->unflushed_buf->buf);
+    qemu_free(vcon->unflushed_buf);
+    vcon->unflushed_buf = NULL;
+}
+
+/* Callback function when the timer expires */
+static void unthrottle_port(VirtConsole *vcon)
+{
+    if (vcon->unflushed_buf) {
+        ssize_t ret;
+        size_t len;
+
+        len = vcon->unflushed_buf->rem_len;
+        ret = qemu_chr_write(vcon->chr,
+                             vcon->unflushed_buf->buf
+                             + vcon->unflushed_buf->offset, &len);
+
+        if (vcon->unflushed_buf->rem_len) {
+            vcon->unflushed_buf->offset += len;
+            vcon->unflushed_buf->rem_len -= len;
+
+            qemu_mod_timer(vcon->host_timer,
+                           qemu_get_clock(host_clock) + (int64_t) 100000);
+            return;
+        }
+        free_unflushed_buf(vcon);
+    }
+    virtio_serial_throttle_port(&vcon->port, false);
+}
 
 /* Callback function that's called when the guest sends us data */
 static void flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    qemu_chr_write(vcon->chr, buf, &len);
+    ssize_t ret;
+    size_t written;
+
+    written = len;
+    ret = qemu_chr_write(vcon->chr, buf, &written);
+    if (len > written) {
+        virtio_serial_throttle_port(port, true);
+
+        /*
+         * We shouldn't be called while there are pending buffers to
+         * be flushed.
+         */
+        assert(!vcon->unflushed_buf);
+
+        add_unflushed_buf(vcon, buf + written, len - written);
+        qemu_mod_timer(vcon->host_timer,
+                       qemu_get_clock(host_clock) + (int64_t) 100000);
+    }
 }
 
 /* Readiness of the guest to accept data on a port */
@@ -48,16 +114,59 @@ static void chr_event(void *opaque, int event)
     VirtConsole *vcon = opaque;
 
     switch (event) {
-    case CHR_EVENT_OPENED: {
+    case CHR_EVENT_OPENED:
         virtio_serial_open(&vcon->port);
         break;
-    }
+
     case CHR_EVENT_CLOSED:
+        if (vcon->unflushed_buf) {
+            qemu_del_timer(vcon->host_timer);
+            free_unflushed_buf(vcon);
+        }
         virtio_serial_close(&vcon->port);
         break;
     }
 }
 
+static void virtio_console_port_save(QEMUFile *f, void *opaque)
+{
+    VirtConsole *vcon = opaque;
+    uint32_t have_buffer;
+
+    have_buffer = vcon->unflushed_buf ? true : false;
+
+    qemu_put_be32s(f, &have_buffer);
+    if (have_buffer) {
+        qemu_put_be64s(f, &vcon->unflushed_buf->rem_len);
+        qemu_put_buffer(f, vcon->unflushed_buf->buf
+                           + vcon->unflushed_buf->offset,
+                        vcon->unflushed_buf->rem_len);
+    }
+}
+
+static int virtio_console_port_load(QEMUFile *f, void *opaque, int version_id)
+{
+    VirtConsole *vcon = opaque;
+    uint32_t have_buffer;
+
+    if (version_id > 1) {
+        return -EINVAL;
+    }
+
+    qemu_get_be32s(f, &have_buffer);
+    if (have_buffer) {
+        vcon->unflushed_buf = qemu_mallocz(sizeof(Buffer));
+
+        qemu_get_be64s(f, &vcon->unflushed_buf->rem_len);
+        vcon->unflushed_buf->buf = qemu_malloc(vcon->unflushed_buf->rem_len);
+        vcon->unflushed_buf->offset = 0;
+
+        qemu_get_buffer(f, vcon->unflushed_buf->buf,
+                        vcon->unflushed_buf->rem_len);
+    }
+    return 0;
+}
+
 static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
     vcon->port.info = dev->info;
@@ -67,6 +176,9 @@ static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
                               vcon);
         vcon->port.info->have_data = flush_buf;
     }
+    vcon->host_timer = qemu_new_timer(host_clock, (void *)unthrottle_port, vcon);
+    register_savevm("virtio-console-ports", -1, 1, virtio_console_port_save,
+		    virtio_console_port_load, vcon);
     return 0;
 }
 
@@ -89,6 +201,7 @@ static int virtconsole_exitfn(VirtIOSerialDevice *dev)
         port->info->have_data = NULL;
         qemu_chr_close(vcon->chr);
     }
+    qemu_free_timer(vcon->host_timer);
 
     return 0;
 }
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 5/5] virtio-console: Throttle virtio-serial-bus if we can't consume any more guest data

[Gerd Hoffmann]

   Hi,

> +/* Callback function when the timer expires */
> +static void unthrottle_port(VirtConsole *vcon)
> +{

Why a timer?

qemu_set_fd_handler(fd, NULL, write_callback, opaque) ?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Paul Brook]

> There might be cases where a few bytes would have been sent out to char
> devices and some not. Currently the return values from qemu_chr_write()
> to char devs are only -1, indicating an error, or the complete length
> of the string passed.
> 
> Make 'len' a pointer instead, and indicate how much of the string was
> written. The return value will either be the same as 'len' or a negative
> number indicating an error condition.

This seems wrong. We should not be getting recoverable errors.

Paul

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Amit Shah]

On (Mon) Apr 05 2010 [17:33:38], Paul Brook wrote:
> > There might be cases where a few bytes would have been sent out to char
> > devices and some not. Currently the return values from qemu_chr_write()
> > to char devs are only -1, indicating an error, or the complete length
> > of the string passed.
> > 
> > Make 'len' a pointer instead, and indicate how much of the string was
> > written. The return value will either be the same as 'len' or a negative
> > number indicating an error condition.
> 
> This seems wrong. We should not be getting recoverable errors.

I was thinking of adding a bool to CharDriverState to indicate if EAGAIN
should be reported to the writer. This can be conveyed at the time of
doing qemu_chr_add_handlers().

It would certainly be beneficial for consumers of virtio-serial to be
notified of -EAGAIN so that the guest can be throttled till the chardev
catches up with the data being sent.

		Amit

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Paul Brook]

> On (Mon) Apr 05 2010 [17:33:38], Paul Brook wrote:
> > > There might be cases where a few bytes would have been sent out to char
> > > devices and some not. Currently the return values from qemu_chr_write()
> > > to char devs are only -1, indicating an error, or the complete length
> > > of the string passed.
> > >
> > > Make 'len' a pointer instead, and indicate how much of the string was
> > > written. The return value will either be the same as 'len' or a
> > > negative number indicating an error condition.
> >
> > This seems wrong. We should not be getting recoverable errors.
> 
> I was thinking of adding a bool to CharDriverState to indicate if EAGAIN
> should be reported to the writer. This can be conveyed at the time of
> doing qemu_chr_add_handlers().
> 
> It would certainly be beneficial for consumers of virtio-serial to be
> notified of -EAGAIN so that the guest can be throttled till the chardev
> catches up with the data being sent.

EAGAIN should only ever occur if no bytes are written. If a stall condition 
occurs after some data has been written (and we allow partial completion) then 
this is indicated by returning a short count.

Paul

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Amit Shah]

On (Tue) Apr 06 2010 [10:34:29], Paul Brook wrote:
> > On (Mon) Apr 05 2010 [17:33:38], Paul Brook wrote:
> > > > There might be cases where a few bytes would have been sent out to char
> > > > devices and some not. Currently the return values from qemu_chr_write()
> > > > to char devs are only -1, indicating an error, or the complete length
> > > > of the string passed.
> > > >
> > > > Make 'len' a pointer instead, and indicate how much of the string was
> > > > written. The return value will either be the same as 'len' or a
> > > > negative number indicating an error condition.
> > >
> > > This seems wrong. We should not be getting recoverable errors.
> > 
> > I was thinking of adding a bool to CharDriverState to indicate if EAGAIN
> > should be reported to the writer. This can be conveyed at the time of
> > doing qemu_chr_add_handlers().
> > 
> > It would certainly be beneficial for consumers of virtio-serial to be
> > notified of -EAGAIN so that the guest can be throttled till the chardev
> > catches up with the data being sent.
> 
> EAGAIN should only ever occur if no bytes are written.

Right. That, or just return 0 and let the caller handle the situation?

		Amit

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Gerd Hoffmann]

On 04/06/10 11:58, Amit Shah wrote:
>>> It would certainly be beneficial for consumers of virtio-serial to be
>>> notified of -EAGAIN so that the guest can be throttled till the chardev
>>> catches up with the data being sent.
>>
>> EAGAIN should only ever occur if no bytes are written.
>
> Right. That, or just return 0 and let the caller handle the situation?

Go with the usual unix semantics instead of creating something new.

When something was written -- return the number of bytes.  Caller has to 
compare with the length passed in to figure whenever it was a partial 
write or not.

When nothing was written -- return the error.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Amit Shah]

On (Tue) Apr 06 2010 [12:21:52], Gerd Hoffmann wrote:
> On 04/06/10 11:58, Amit Shah wrote:
>>>> It would certainly be beneficial for consumers of virtio-serial to be
>>>> notified of -EAGAIN so that the guest can be throttled till the chardev
>>>> catches up with the data being sent.
>>>
>>> EAGAIN should only ever occur if no bytes are written.
>>
>> Right. That, or just return 0 and let the caller handle the situation?
>
> Go with the usual unix semantics instead of creating something new.
>
> When something was written -- return the number of bytes.  Caller has to  
> compare with the length passed in to figure whenever it was a partial  
> write or not.
>
> When nothing was written -- return the error.

Unless Paul wants to never return recoverable error messages as he
mentioned in his first mail.

		Amit

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Gerd Hoffmann]

On 04/06/10 13:05, Amit Shah wrote:
> On (Tue) Apr 06 2010 [12:21:52], Gerd Hoffmann wrote:
>> On 04/06/10 11:58, Amit Shah wrote:
>>>>> It would certainly be beneficial for consumers of virtio-serial to be
>>>>> notified of -EAGAIN so that the guest can be throttled till the chardev
>>>>> catches up with the data being sent.
>>>>
>>>> EAGAIN should only ever occur if no bytes are written.
>>>
>>> Right. That, or just return 0 and let the caller handle the situation?
>>
>> Go with the usual unix semantics instead of creating something new.
>>
>> When something was written -- return the number of bytes.  Caller has to
>> compare with the length passed in to figure whenever it was a partial
>> write or not.
>>
>> When nothing was written -- return the error.
>
> Unless Paul wants to never return recoverable error messages as he
> mentioned in his first mail.

Return value "0" usually means end-of-file, I would not use that for 
something else too.  We have to agree on something though ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Jamie Lokier]

Gerd Hoffmann wrote:
> On 04/06/10 13:05, Amit Shah wrote:
> >On (Tue) Apr 06 2010 [12:21:52], Gerd Hoffmann wrote:
> >>On 04/06/10 11:58, Amit Shah wrote:
> >>>>>It would certainly be beneficial for consumers of virtio-serial to be
> >>>>>notified of -EAGAIN so that the guest can be throttled till the chardev
> >>>>>catches up with the data being sent.
> >>>>
> >>>>EAGAIN should only ever occur if no bytes are written.
> >>>
> >>>Right. That, or just return 0 and let the caller handle the situation?
> >>
> >>Go with the usual unix semantics instead of creating something new.
> >>
> >>When something was written -- return the number of bytes.  Caller has to
> >>compare with the length passed in to figure whenever it was a partial
> >>write or not.
> >>
> >>When nothing was written -- return the error.
> >
> >Unless Paul wants to never return recoverable error messages as he
> >mentioned in his first mail.
> 
> Return value "0" usually means end-of-file, I would not use that for 
> something else too.  We have to agree on something though ...

Which is why EAGAIN was invented.

-- Jamie

[Qemu-devel] Re: [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Gerd Hoffmann]

   Hi,

> Make 'len' a pointer instead, and indicate how much of the string was
> written. The return value will either be the same as 'len' or a negative
> number indicating an error condition.

I would follow the unix write syscall semantic here and report the 
number of written bytes using the return value.  Matter of taste though.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 1/5] char: Let the caller know how many bytes were written in case of incomplete writes

[Amit Shah]

On (Tue) Apr 06 2010 [09:40:55], Gerd Hoffmann wrote:
>   Hi,
>
>> Make 'len' a pointer instead, and indicate how much of the string was
>> written. The return value will either be the same as 'len' or a negative
>> number indicating an error condition.
>
> I would follow the unix write syscall semantic here and report the  
> number of written bytes using the return value.  Matter of taste though.

Yeah; some re-org in the write() calls in qemu-char would be needed to
do that, but yes, that can be done too.

		Amit

[Qemu-devel] Re: [PATCH 0/5] chardev, virtio-console: flow control, error handling

[Gerd Hoffmann]

   Hi,

> In case of EAGAIN, the unix_write() function just kept spinning while
> attempting to write to the chardev till it succeeded. This resulted in
> a stuck VM in case a chardev had opened connection but wasn't reading
> anything from qemu.

It spins only for non-blocking file handles.  In blocking mode the write 
syscall just goes sleep until it can write out data (or gets a signal).

> There are two fixes for that case:
> - Poll for POLLOUT instead of directly attempting write(), which helps
>    relax the CPU (and we become greener).

No need to do that, see above.  Having a poll() call in unix_write() is 
wrong IMHO.

> - If the file that we're writing to is nonblocking, return -EAGAIN to
>    the caller of qemu_chr_write() so that appropriate actions can be
>    taken higher up in the stack.

Good.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 0/5] chardev, virtio-console: flow control, error handling

[Amit Shah]

On (Tue) Apr 06 2010 [09:36:17], Gerd Hoffmann wrote:
>   Hi,
>
>> In case of EAGAIN, the unix_write() function just kept spinning while
>> attempting to write to the chardev till it succeeded. This resulted in
>> a stuck VM in case a chardev had opened connection but wasn't reading
>> anything from qemu.
>
> It spins only for non-blocking file handles.  In blocking mode the write  
> syscall just goes sleep until it can write out data (or gets a signal).

True. I'm interesting in non-blocking files for now.

>> There are two fixes for that case:
>> - Poll for POLLOUT instead of directly attempting write(), which helps
>>    relax the CPU (and we become greener).
>
> No need to do that, see above.  Having a poll() call in unix_write() is  
> wrong IMHO.

So you mean just return -EAGAIN in case write returns with -EAGAIN?

Yeah, makes sense.

>> - If the file that we're writing to is nonblocking, return -EAGAIN to
>>    the caller of qemu_chr_write() so that appropriate actions can be
>>    taken higher up in the stack.
>
> Good.

		Amit

[Qemu-devel] Re: [PATCH 0/5] chardev, virtio-console: flow control, error handling

[Gerd Hoffmann]

> So you mean just return -EAGAIN in case write returns with -EAGAIN?
>
> Yeah, makes sense.

Yes.

Note that for EINTR you want re-enter write() through so callers don't 
have to worry about signals.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 0/5] chardev, virtio-console: flow control, error handling

[Amit Shah]

On (Tue) Apr 06 2010 [10:17:12], Gerd Hoffmann wrote:
>> So you mean just return -EAGAIN in case write returns with -EAGAIN?
>>
>> Yeah, makes sense.
>
> Yes.
>
> Note that for EINTR you want re-enter write() through so callers don't  
> have to worry about signals.

Yes, of course.

		Amit