[Qemu-devel] [PATCH v7 0/6]

[Qemu-devel] [PATCH v7 0/6]

[Amit Shah]

Hello,

This series lets interested callers ask for an -EAGAIN return from the
chardev backends (only unix and tcp sockets as of now) to implement
their own flow control.

A new call, qemu_chr_write_nb() is added, that will fallback to
qemu_chr_write() if the backend file isn't non-blocking or if no
callback was supplied.

Support for other backend types is easy to add and will be done in
later patches.

Please apply.

v7:
- constify handlers (Blue Swirl)
- remove 'write_cb', a leftover from previous design (Juan Quintela)
- return ret instead of -EAGAIN in virtio-console (Juan)
- use pre-allocated meta-buffer instead of allocating one each time
  (Juan)

v6:
- Continue write operation on EINTR instead of returning partial
  writes (which was a change from prev. behaviour) (Gerd)

v5:
- Fix bug pointed out by Gerd
- Convert to using a struct for passing on handlers to
  qemu_chr_add_handlers() instead of passing each one
  individually. Simplifies patches. (Inspired by Juan's comment)
- Re-arranged patches

Amit Shah (6):
  virtio-console: Factor out common init between console and generic
    ports
  char: Add a QemuChrHandlers struct to initialise chardev handlers
  char: Let writers know how much data was written in case of errors
  char: Add qemu_chr_write_nb() for nonblocking writes
  char: unix/tcp: Add a non-blocking write handler
  virtio-console: Throttle virtio-serial-bus if we can't consume any
    more guest data

 gdbstub.c            |    9 ++-
 hw/debugcon.c        |    2 +-
 hw/escc.c            |    9 ++-
 hw/etraxfs_ser.c     |   13 +++-
 hw/mcf_uart.c        |    9 ++-
 hw/pl011.c           |    9 ++-
 hw/pxa2xx.c          |   13 +++-
 hw/serial.c          |    9 ++-
 hw/sh_serial.c       |   12 +++-
 hw/syborg_serial.c   |    9 ++-
 hw/usb-serial.c      |    9 ++-
 hw/virtio-console.c  |  156 +++++++++++++++++++++++++++++++++++++++++++-------
 hw/xen_console.c     |   16 ++++--
 hw/xilinx_uartlite.c |   11 +++-
 monitor.c            |   19 +++++-
 net/slirp.c          |    8 ++-
 net/socket.c         |    4 +-
 qemu-char.c          |  119 ++++++++++++++++++++++++++++++++------
 qemu-char.h          |   20 +++++-
 qemu_socket.h        |    3 +-
 20 files changed, 375 insertions(+), 84 deletions(-)

[Qemu-devel] [PATCH v7 1/6] 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 caea11f..d7fe68b 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 v7 2/6] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Amit Shah]

Instead of passing each handler in the qemu_add_handlers() function,
create a struct of handlers that can be passed to the function instead.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 gdbstub.c            |    9 +++++++--
 hw/debugcon.c        |    2 +-
 hw/escc.c            |    9 +++++++--
 hw/etraxfs_ser.c     |   13 +++++++++----
 hw/mcf_uart.c        |    9 +++++++--
 hw/pl011.c           |    9 +++++++--
 hw/pxa2xx.c          |   13 +++++++++----
 hw/serial.c          |    9 +++++++--
 hw/sh_serial.c       |   12 +++++++++---
 hw/syborg_serial.c   |    9 +++++++--
 hw/usb-serial.c      |    9 +++++++--
 hw/virtio-console.c  |    9 +++++++--
 hw/xen_console.c     |   16 +++++++++++-----
 hw/xilinx_uartlite.c |   11 +++++++++--
 monitor.c            |   19 +++++++++++++++----
 net/slirp.c          |    8 ++++++--
 qemu-char.c          |   27 ++++++++++++++++++---------
 qemu-char.h          |   12 ++++++++----
 18 files changed, 151 insertions(+), 54 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 93c4850..ae29db1 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2621,6 +2621,12 @@ static void gdb_sigterm_handler(int signal)
 }
 #endif
 
+static const QemuChrHandlers gdb_handlers = {
+    .fd_can_read = gdb_chr_can_receive,
+    .fd_read = gdb_chr_receive,
+    .fd_event = gdb_chr_event,
+};
+
 int gdbserver_start(const char *device)
 {
     GDBState *s;
@@ -2650,8 +2656,7 @@ int gdbserver_start(const char *device)
         if (!chr)
             return -1;
 
-        qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
-                              gdb_chr_event, NULL);
+        qemu_chr_add_handlers(chr, &gdb_handlers, NULL);
     }
 
     s = gdbserver_state;
diff --git a/hw/debugcon.c b/hw/debugcon.c
index 5ee6821..e79a595 100644
--- a/hw/debugcon.c
+++ b/hw/debugcon.c
@@ -73,7 +73,7 @@ static void debugcon_init_core(DebugconState *s)
         exit(1);
     }
 
-    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, s);
+    qemu_chr_add_handlers(s->chr, NULL, s);
 }
 
 static int debugcon_isa_initfn(ISADevice *dev)
diff --git a/hw/escc.c b/hw/escc.c
index 6d2fd36..4057bb4 100644
--- a/hw/escc.c
+++ b/hw/escc.c
@@ -890,6 +890,12 @@ void slavio_serial_ms_kbd_init(target_phys_addr_t base, qemu_irq irq,
     sysbus_mmio_map(s, 0, base);
 }
 
+static const QemuChrHandlers serial_handlers = {
+    .fd_can_read = serial_can_receive,
+    .fd_read = serial_receive1,
+    .fd_event = serial_event,
+};
+
 static int escc_init1(SysBusDevice *dev)
 {
     SerialState *s = FROM_SYSBUS(SerialState, dev);
@@ -903,8 +909,7 @@ static int escc_init1(SysBusDevice *dev)
         s->chn[i].chn = 1 - i;
         s->chn[i].clock = s->frequency / 2;
         if (s->chn[i].chr) {
-            qemu_chr_add_handlers(s->chn[i].chr, serial_can_receive,
-                                  serial_receive1, serial_event, &s->chn[i]);
+            qemu_chr_add_handlers(s->chn[i].chr, &serial_handlers, &s->chn[i]);
         }
     }
     s->chn[0].otherchn = &s->chn[1];
diff --git a/hw/etraxfs_ser.c b/hw/etraxfs_ser.c
index e1f9615..0c0c485 100644
--- a/hw/etraxfs_ser.c
+++ b/hw/etraxfs_ser.c
@@ -161,6 +161,12 @@ static void serial_event(void *opaque, int event)
 
 }
 
+static const QemuChrHandlers serial_handlers = {
+    .fd_can_read = serial_can_receive,
+    .fd_read = serial_receive,
+    .fd_event = serial_event,
+};
+
 static int etraxfs_ser_init(SysBusDevice *dev)
 {
     struct etrax_serial *s = FROM_SYSBUS(typeof (*s), dev);
@@ -174,10 +180,9 @@ static int etraxfs_ser_init(SysBusDevice *dev)
     ser_regs = cpu_register_io_memory(ser_read, ser_write, s);
     sysbus_init_mmio(dev, R_MAX * 4, ser_regs);
     s->chr = qdev_init_chardev(&dev->qdev);
-    if (s->chr)
-        qemu_chr_add_handlers(s->chr,
-                      serial_can_receive, serial_receive,
-                      serial_event, s);
+    if (s->chr) {
+        qemu_chr_add_handlers(s->chr, &serial_handlers, s);
+    }
     return 0;
 }
 
diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
index d16bac7..d2ce5f6 100644
--- a/hw/mcf_uart.c
+++ b/hw/mcf_uart.c
@@ -268,6 +268,12 @@ static void mcf_uart_receive(void *opaque, const uint8_t *buf, int size)
     mcf_uart_push_byte(s, buf[0]);
 }
 
+static const QemuChrHandlers mcf_uart_handlers = {
+    .fd_can_read = mcf_uart_can_receive,
+    .fd_read = mcf_uart_receive,
+    .fd_event = mcf_uart_event,
+};
+
 void *mcf_uart_init(qemu_irq irq, CharDriverState *chr)
 {
     mcf_uart_state *s;
@@ -276,8 +282,7 @@ void *mcf_uart_init(qemu_irq irq, CharDriverState *chr)
     s->chr = chr;
     s->irq = irq;
     if (chr) {
-        qemu_chr_add_handlers(chr, mcf_uart_can_receive, mcf_uart_receive,
-                              mcf_uart_event, s);
+        qemu_chr_add_handlers(chr, &mcf_uart_handlers, s);
     }
     mcf_uart_reset(s);
     return s;
diff --git a/hw/pl011.c b/hw/pl011.c
index 81de91e..24711c2 100644
--- a/hw/pl011.c
+++ b/hw/pl011.c
@@ -286,6 +286,12 @@ static int pl011_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static const QemuChrHandlers pl011_handlers = {
+    .fd_can_read = pl011_can_receive,
+    .fd_read = pl011_receive,
+    .fd_event = pl011_event,
+};
+
 static int pl011_init(SysBusDevice *dev, const unsigned char *id)
 {
     int iomemtype;
@@ -303,8 +309,7 @@ static int pl011_init(SysBusDevice *dev, const unsigned char *id)
     s->cr = 0x300;
     s->flags = 0x90;
     if (s->chr) {
-        qemu_chr_add_handlers(s->chr, pl011_can_receive, pl011_receive,
-                              pl011_event, s);
+        qemu_chr_add_handlers(s->chr, &pl011_handlers, s);
     }
     register_savevm("pl011_uart", -1, 1, pl011_save, pl011_load, s);
     return 0;
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index 9095386..e2adaaf 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1992,6 +1992,12 @@ static int pxa2xx_fir_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static const QemuChrHandlers pxa2xx_handlers = {
+    .fd_can_read = pxa2xx_fir_is_empty,
+    .fd_read = pxa2xx_fir_rx,
+    .fd_event = pxa2xx_fir_event,
+};
+
 static PXA2xxFIrState *pxa2xx_fir_init(target_phys_addr_t base,
                 qemu_irq irq, PXA2xxDMAState *dma,
                 CharDriverState *chr)
@@ -2010,10 +2016,9 @@ static PXA2xxFIrState *pxa2xx_fir_init(target_phys_addr_t base,
                     pxa2xx_fir_writefn, s);
     cpu_register_physical_memory(base, 0x1000, iomemtype);
 
-    if (chr)
-        qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
-                        pxa2xx_fir_rx, pxa2xx_fir_event, s);
-
+    if (chr) {
+        qemu_chr_add_handlers(chr, &pxa2xx_handlers, s);
+    }
     register_savevm("pxa2xx_fir", 0, 0, pxa2xx_fir_save, pxa2xx_fir_load, s);
 
     return s;
diff --git a/hw/serial.c b/hw/serial.c
index 90213c4..d4cc317 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -724,6 +724,12 @@ static void serial_reset(void *opaque)
     qemu_irq_lower(s->irq);
 }
 
+static const QemuChrHandlers serial_handlers = {
+    .fd_can_read = serial_can_receive1,
+    .fd_read = serial_receive1,
+    .fd_event = serial_event,
+};
+
 static void serial_init_core(SerialState *s)
 {
     if (!s->chr) {
@@ -738,8 +744,7 @@ static void serial_init_core(SerialState *s)
 
     qemu_register_reset(serial_reset, s);
 
-    qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
-                          serial_event, s);
+    qemu_chr_add_handlers(s->chr, &serial_handlers, s);
 }
 
 /* Change the main reference oscillator frequency. */
diff --git a/hw/sh_serial.c b/hw/sh_serial.c
index 93dc144..2201dba 100644
--- a/hw/sh_serial.c
+++ b/hw/sh_serial.c
@@ -363,6 +363,12 @@ static CPUWriteMemoryFunc * const sh_serial_writefn[] = {
     &sh_serial_write,
 };
 
+static const QemuChrHandlers sh_serial_handlers = {
+    .fd_can_read = sh_serial_can_receive1,
+    .fd_read = sh_serial_receive1,
+    .fd_event = sh_serial_event,
+};
+
 void sh_serial_init (target_phys_addr_t base, int feat,
 		     uint32_t freq, CharDriverState *chr,
 		     qemu_irq eri_source,
@@ -401,9 +407,9 @@ void sh_serial_init (target_phys_addr_t base, int feat,
 
     s->chr = chr;
 
-    if (chr)
-        qemu_chr_add_handlers(chr, sh_serial_can_receive1, sh_serial_receive1,
-			      sh_serial_event, s);
+    if (chr) {
+        qemu_chr_add_handlers(chr, &sh_serial_handlers, s);
+    }
 
     s->eri = eri_source;
     s->rxi = rxi_source;
diff --git a/hw/syborg_serial.c b/hw/syborg_serial.c
index cac00ea..9ad5ddc 100644
--- a/hw/syborg_serial.c
+++ b/hw/syborg_serial.c
@@ -315,6 +315,12 @@ static int syborg_serial_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static const QemuChrHandlers syborg_serial_handlers = {
+    .fd_can_read = syborg_serial_can_receive,
+    .fd_read = syborg_serial_receive,
+    .fd_event = syborg_serial_event,
+};
+
 static int syborg_serial_init(SysBusDevice *dev)
 {
     SyborgSerialState *s = FROM_SYSBUS(SyborgSerialState, dev);
@@ -326,8 +332,7 @@ static int syborg_serial_init(SysBusDevice *dev)
     sysbus_init_mmio(dev, 0x1000, iomemtype);
     s->chr = qdev_init_chardev(&dev->qdev);
     if (s->chr) {
-        qemu_chr_add_handlers(s->chr, syborg_serial_can_receive,
-                              syborg_serial_receive, syborg_serial_event, s);
+        qemu_chr_add_handlers(s->chr, &syborg_serial_handlers, s);
     }
     if (s->fifo_size <= 0) {
         fprintf(stderr, "syborg_serial: fifo too small\n");
diff --git a/hw/usb-serial.c b/hw/usb-serial.c
index 5b2483a..56e2b00 100644
--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -540,13 +540,18 @@ static void usb_serial_event(void *opaque, int event)
     }
 }
 
+static const QemuChrHandlers usb_serial_handlers = {
+    .fd_can_read = usb_serial_can_read,
+    .fd_read = usb_serial_read,
+    .fd_event = usb_serial_event,
+};
+
 static int usb_serial_initfn(USBDevice *dev)
 {
     USBSerialState *s = DO_UPCAST(USBSerialState, dev, dev);
     s->dev.speed = USB_SPEED_FULL;
 
-    qemu_chr_add_handlers(s->cs, usb_serial_can_read, usb_serial_read,
-                          usb_serial_event, s);
+    qemu_chr_add_handlers(s->cs, &usb_serial_handlers, s);
     usb_serial_handle_reset(dev);
     return 0;
 }
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index d7fe68b..862a431 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -58,13 +58,18 @@ static void chr_event(void *opaque, int event)
     }
 }
 
+static const QemuChrHandlers chr_handlers = {
+    .fd_can_read = chr_can_read,
+    .fd_read = chr_read,
+    .fd_event = chr_event,
+};
+
 static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
     vcon->port.info = dev->info;
 
     if (vcon->chr) {
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
+        qemu_chr_add_handlers(vcon->chr, &chr_handlers, vcon);
         vcon->port.info->have_data = flush_buf;
     }
     return 0;
diff --git a/hw/xen_console.c b/hw/xen_console.c
index d2261f4..8327e4e 100644
--- a/hw/xen_console.c
+++ b/hw/xen_console.c
@@ -202,6 +202,11 @@ static int con_init(struct XenDevice *xendev)
     return 0;
 }
 
+static const QemuChrHandlers xencons_handlers = {
+    .fd_can_read = xencons_can_receive,
+    .fd_read = xencons_receive,
+};
+
 static int con_connect(struct XenDevice *xendev)
 {
     struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
@@ -222,9 +227,9 @@ static int con_connect(struct XenDevice *xendev)
 	return -1;
 
     xen_be_bind_evtchn(&con->xendev);
-    if (con->chr)
-        qemu_chr_add_handlers(con->chr, xencons_can_receive, xencons_receive,
-                              NULL, con);
+    if (con->chr) {
+        qemu_chr_add_handlers(con->chr, &xencons_handlers, con);
+    }
 
     xen_be_printf(xendev, 1, "ring mfn %d, remote port %d, local port %d, limit %zd\n",
 		  con->ring_ref,
@@ -238,8 +243,9 @@ static void con_disconnect(struct XenDevice *xendev)
 {
     struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
 
-    if (con->chr)
-        qemu_chr_add_handlers(con->chr, NULL, NULL, NULL, NULL);
+    if (con->chr) {
+        qemu_chr_add_handlers(con->chr, NULL, NULL);
+    }
     xen_be_unbind_evtchn(&con->xendev);
 
     if (con->sring) {
diff --git a/hw/xilinx_uartlite.c b/hw/xilinx_uartlite.c
index adab759..66f0396 100644
--- a/hw/xilinx_uartlite.c
+++ b/hw/xilinx_uartlite.c
@@ -193,6 +193,12 @@ static void uart_event(void *opaque, int event)
 
 }
 
+static const QemuChrHandlers uart_handlers = {
+    .fd_can_read = uart_can_rx,
+    .fd_read = uart_rx,
+    .fd_event = uart_event,
+};
+
 static int xilinx_uartlite_init(SysBusDevice *dev)
 {
     struct xlx_uartlite *s = FROM_SYSBUS(typeof (*s), dev);
@@ -205,8 +211,9 @@ static int xilinx_uartlite_init(SysBusDevice *dev)
     sysbus_init_mmio(dev, R_MAX * 4, uart_regs);
 
     s->chr = qdev_init_chardev(&dev->qdev);
-    if (s->chr)
-        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
+    if (s->chr) {
+        qemu_chr_add_handlers(s->chr, &uart_handlers, s);
+    }
     return 0;
 }
 
diff --git a/monitor.c b/monitor.c
index 46d0b47..22a61d4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4600,6 +4600,18 @@ static void monitor_event(void *opaque, int event)
  * End:
  */
 
+static const QemuChrHandlers monitor_handlers = {
+    .fd_can_read = monitor_can_read,
+    .fd_read = monitor_read,
+    .fd_event = monitor_event,
+};
+
+static const QemuChrHandlers monitor_control_handlers = {
+    .fd_can_read = monitor_can_read,
+    .fd_read = monitor_control_read,
+    .fd_event = monitor_control_event,
+};
+
 void monitor_init(CharDriverState *chr, int flags)
 {
     static int is_first_init = 1;
@@ -4621,12 +4633,11 @@ void monitor_init(CharDriverState *chr, int flags)
 
     if (monitor_ctrl_mode(mon)) {
         mon->mc = qemu_mallocz(sizeof(MonitorControl));
+
         /* Control mode requires special handlers */
-        qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
-                              monitor_control_event, mon);
+        qemu_chr_add_handlers(chr, &monitor_control_handlers, mon);
     } else {
-        qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
-                              monitor_event, mon);
+        qemu_chr_add_handlers(chr, &monitor_handlers, mon);
     }
 
     QLIST_INSERT_HEAD(&mon_list, mon, entry);
diff --git a/net/slirp.c b/net/slirp.c
index b41c60a..437be46 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -577,6 +577,11 @@ static void guestfwd_read(void *opaque, const uint8_t *buf, int size)
     slirp_socket_recv(fwd->slirp, fwd->server, fwd->port, buf, size);
 }
 
+static const QemuChrHandlers guestfwd_handlers = {
+    .fd_can_read = guestfwd_can_read,
+    .fd_read = guestfwd_read,
+};
+
 static int slirp_guestfwd(SlirpState *s, const char *config_str,
                           int legacy_format)
 {
@@ -633,8 +638,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
     fwd->port = port;
     fwd->slirp = s->slirp;
 
-    qemu_chr_add_handlers(fwd->hd, guestfwd_can_read, guestfwd_read,
-                          NULL, fwd);
+    qemu_chr_add_handlers(fwd->hd, &guestfwd_handlers, fwd);
     return 0;
 
  fail_syntax:
diff --git a/qemu-char.c b/qemu-char.c
index ac65a1c..76ad12c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -191,15 +191,19 @@ void qemu_chr_send_event(CharDriverState *s, int event)
         s->chr_send_event(s, event);
 }
 
+static const QemuChrHandlers null_handlers = {
+    /* All handlers are initialised to NULL */
+};
+
 void qemu_chr_add_handlers(CharDriverState *s,
-                           IOCanReadHandler *fd_can_read,
-                           IOReadHandler *fd_read,
-                           IOEventHandler *fd_event,
-                           void *opaque)
-{
-    s->chr_can_read = fd_can_read;
-    s->chr_read = fd_read;
-    s->chr_event = fd_event;
+                           const QemuChrHandlers *handlers, void *opaque)
+{
+    if (!handlers) {
+        handlers = &null_handlers;
+    }
+    s->chr_can_read = handlers->fd_can_read;
+    s->chr_read = handlers->fd_read;
+    s->chr_event = handlers->fd_event;
     s->handler_opaque = opaque;
     if (s->chr_update_read_handler)
         s->chr_update_read_handler(s);
@@ -442,6 +446,12 @@ static void mux_chr_event(void *opaque, int event)
         mux_chr_send_event(d, i, event);
 }
 
+static const QemuChrHandlers mux_chr_handlers = {
+    .fd_can_read = mux_chr_can_read,
+    .fd_read = mux_chr_read,
+    .fd_event = mux_chr_event,
+};
+
 static void mux_chr_update_read_handler(CharDriverState *chr)
 {
     MuxDriver *d = chr->opaque;
@@ -456,8 +466,7 @@ static void mux_chr_update_read_handler(CharDriverState *chr)
     d->chr_event[d->mux_cnt] = chr->chr_event;
     /* Fix up the real driver with mux routines */
     if (d->mux_cnt == 0) {
-        qemu_chr_add_handlers(d->drv, mux_chr_can_read, mux_chr_read,
-                              mux_chr_event, chr);
+        qemu_chr_add_handlers(d->drv, &mux_chr_handlers, chr);
     }
     if (d->focus != -1) {
         mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
diff --git a/qemu-char.h b/qemu-char.h
index e3a0783..eacb3b9 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -71,6 +71,13 @@ struct CharDriverState {
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
+typedef struct QemuChrHandlers {
+    IOCanReadHandler *fd_can_read;
+    IOReadHandler *fd_read;
+    IOHandler *fd_write_unblocked;
+    IOEventHandler *fd_event;
+} QemuChrHandlers;
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
 CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
                                     void (*init)(struct CharDriverState *s));
@@ -79,10 +86,7 @@ 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);
 void qemu_chr_send_event(CharDriverState *s, int event);
-void qemu_chr_add_handlers(CharDriverState *s,
-                           IOCanReadHandler *fd_can_read,
-                           IOReadHandler *fd_read,
-                           IOEventHandler *fd_event,
+void qemu_chr_add_handlers(CharDriverState *s, const QemuChrHandlers *handlers,
                            void *opaque);
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
 void qemu_chr_generic_open(CharDriverState *s);
-- 
1.6.2.5

[Qemu-devel] [PATCH v7 3/6] char: Let writers know how much data was written in case of errors

[Amit Shah]

On writing errors, we just returned -1 even if some bytes were already
written out. Ensure we return the number of bytes written before we
return the error (on a subsequent call to qemu_chr_write()).

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

diff --git a/qemu-char.c b/qemu-char.c
index 76ad12c..decf687 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -507,6 +507,9 @@ int send_all(int fd, const void *buf, int len1)
     while (len > 0) {
         ret = send(fd, buf, len, 0);
         if (ret < 0) {
+            if (len1 - len) {
+                return len1 - len;
+            }
             errno = WSAGetLastError();
             if (errno != WSAEWOULDBLOCK) {
                 return -1;
@@ -531,8 +534,15 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
     while (len > 0) {
         ret = write(fd, buf, len);
         if (ret < 0) {
-            if (errno != EINTR && errno != EAGAIN)
+            if (errno == EINTR) {
+                continue;
+            }
+            if (len1 - len) {
+                return len1 - len;
+            }
+            if (errno != EAGAIN) {
                 return -1;
+            }
         } else if (ret == 0) {
             break;
         } else {
-- 
1.6.2.5

[Qemu-devel] [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Amit Shah]

For char devices whose backing files are open in non-blocking mode,
non-blocking writes can now be made using qemu_chr_write_nb().

For non-blocking chardevs, we can return -EAGAIN to callers of
qemu_chr_write_nb(). When the backend is ready to accept more data,
we can let the caller know via a callback.

-EAGAIN is returned only when the backend's file is non-blocking
and a callback is registered by the caller when invoking
qemu_chr_add_handlers().

In case a backend doesn't support non-blocking writes,
qemu_chr_write_nb() invokes qemu_chr_write().

Individual callers can update their code to add a callback handler,
call qemu_chr_write_nb() instead of qemu_chr_write() if they wish to
receive -EAGAIN notifications.

No backend currently supports non-blocking writes.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 net/socket.c  |    4 ++--
 qemu-char.c   |   31 ++++++++++++++++++++++++-------
 qemu-char.h   |    8 ++++++++
 qemu_socket.h |    3 ++-
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 1c4e153..8a401e6 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -56,8 +56,8 @@ static ssize_t net_socket_receive(VLANClientState *nc, const uint8_t *buf, size_
     uint32_t len;
     len = htonl(size);
 
-    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, sizeof(len), false);
+    return send_all(s->fd, buf, size, false);
 }
 
 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 decf687..5e4dec3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -145,6 +145,16 @@ int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len)
     return s->chr_write(s, buf, len);
 }
 
+ssize_t qemu_chr_write_nb(CharDriverState *s, const uint8_t *buf, size_t len)
+{
+    if (!s->nonblock) {
+        /* Fallback to blocking write if no callback registered */
+        return qemu_chr_write(s, buf, len);
+    }
+
+    return s->chr_write_nb(s, buf, len);
+}
+
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
 {
     if (!s->chr_ioctl)
@@ -203,11 +213,15 @@ void qemu_chr_add_handlers(CharDriverState *s,
     }
     s->chr_can_read = handlers->fd_can_read;
     s->chr_read = handlers->fd_read;
+    s->chr_write_unblocked = handlers->fd_write_unblocked;
     s->chr_event = handlers->fd_event;
     s->handler_opaque = opaque;
     if (s->chr_update_read_handler)
         s->chr_update_read_handler(s);
 
+    /* We'll set this at connect-time */
+    s->nonblock = false;
+
     /* We're connecting to an already opened device, so let's make sure we
        also get the open event */
     if (s->opened) {
@@ -499,7 +513,7 @@ 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, int len1, bool nonblock)
 {
     int ret, len;
 
@@ -526,7 +540,7 @@ 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, int len1, bool nonblock)
 {
     int ret, len;
 
@@ -540,6 +554,9 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
             if (len1 - len) {
                 return len1 - len;
             }
+            if (errno == EAGAIN && nonblock) {
+                return -EAGAIN;
+            }
             if (errno != EAGAIN) {
                 return -1;
             }
@@ -553,9 +570,9 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
     return len1 - len;
 }
 
-int send_all(int fd, const void *buf, int len1)
+int send_all(int fd, const void *buf, int len1, bool nonblock)
 {
-    return unix_write(fd, buf, len1);
+    return unix_write(fd, buf, len1, nonblock);
 }
 #endif /* !_WIN32 */
 
@@ -572,7 +589,7 @@ static int stdio_nb_clients = 0;
 static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     FDCharDriver *s = chr->opaque;
-    return send_all(s->fd_out, buf, len);
+    return send_all(s->fd_out, buf, len, false);
 }
 
 static int fd_chr_read_poll(void *opaque)
@@ -884,7 +901,7 @@ static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
         pty_chr_update_read_handler(chr);
         return 0;
     }
-    return send_all(s->fd, buf, len);
+    return send_all(s->fd, buf, len, false);
 }
 
 static int pty_chr_read_poll(void *opaque)
@@ -1949,7 +1966,7 @@ static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     TCPCharDriver *s = chr->opaque;
     if (s->connected) {
-        return send_all(s->fd, buf, len);
+        return send_all(s->fd, buf, len, false);
     } else {
         /* XXX: indicate an error ? */
         return len;
diff --git a/qemu-char.h b/qemu-char.h
index eacb3b9..52f9ef1 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"
@@ -53,12 +55,15 @@ 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);
+    ssize_t (*chr_write_nb)(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);
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
+    IOHandler *chr_write_unblocked;
     void *handler_opaque;
     void (*chr_send_event)(struct CharDriverState *chr, int event);
     void (*chr_close)(struct CharDriverState *chr);
@@ -68,6 +73,8 @@ struct CharDriverState {
     char *label;
     char *filename;
     int opened;
+    bool nonblock;
+    bool write_blocked;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
@@ -85,6 +92,7 @@ CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*i
 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);
+ssize_t qemu_chr_write_nb(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, const QemuChrHandlers *handlers,
                            void *opaque);
diff --git a/qemu_socket.h b/qemu_socket.h
index 164ae3e..bdf878b 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -23,6 +23,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include <arpa/inet.h>
 #include <netdb.h>
 #include <sys/un.h>
+#include <stdbool.h>
 
 #define socket_error() errno
 #define closesocket(s) close(s)
@@ -35,7 +36,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, int len1, bool nonblock);
 
 /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
 int inet_listen_opts(QemuOpts *opts, int port_offset);
-- 
1.6.2.5

[Qemu-devel] [PATCH v7 5/6] char: unix/tcp: Add a non-blocking write handler

[Amit Shah]

Add a non-blocking write handler that can return with -EAGAIN to the
caller and also callback when the socket becomes writable.

Non-blocking writes are only enabled for sockets that are opened in
non-blocking mode and only for callers that have registered a callback
handler for resuming writes.

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

diff --git a/qemu-char.c b/qemu-char.c
index 5e4dec3..9aa3401 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2129,11 +2129,57 @@ static void tcp_chr_read(void *opaque)
     }
 }
 
+static void tcp_chr_write_unblocked(void *opaque)
+{
+    CharDriverState *chr = opaque;
+    TCPCharDriver *s = chr->opaque;
+
+    assert(chr->write_blocked && chr->chr_write_unblocked);
+
+    chr->write_blocked = false;
+    qemu_set_fd_handler2(s->fd, tcp_chr_read_poll, tcp_chr_read, NULL, chr);
+    chr->chr_write_unblocked(chr->handler_opaque);
+}
+
+static ssize_t tcp_chr_write_nb(CharDriverState *chr, const uint8_t *buf,
+                                size_t len)
+{
+    TCPCharDriver *s = chr->opaque;
+    ssize_t ret;
+
+    if (!s->connected) {
+        /* XXX: indicate an error? */
+        return len;
+    }
+
+    ret = send_all(s->fd, buf, len, true);
+    if (ret == -EAGAIN) {
+        chr->write_blocked = true;
+        qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
+                             tcp_chr_read, tcp_chr_write_unblocked, chr);
+    }
+    return ret;
+}
+
 static void tcp_chr_connect(void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
+    int flags;
+    bool nonblock;
+
+    flags = fcntl(s->fd, F_GETFL);
+    if (flags == -1) {
+        flags = 0;
+    }
+    nonblock = flags & O_NONBLOCK;
+
+    chr->nonblock = false;
+    if (nonblock && chr->chr_write_unblocked) {
+        chr->nonblock = true;
+    }
 
+    chr->write_blocked = false;
     s->connected = 1;
     qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
                          tcp_chr_read, NULL, chr);
@@ -2266,6 +2312,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 
     chr->opaque = s;
     chr->chr_write = tcp_chr_write;
+    chr->chr_write_nb = tcp_chr_write_nb;
     chr->chr_close = tcp_chr_close;
     chr->get_msgfd = tcp_get_msgfd;
 
-- 
1.6.2.5

[Qemu-devel] [PATCH v7 6/6] 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.

Allow the chardev backends to return -EAGAIN; we're ready with a
callback handler that will flush the remainder of the 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 |  120 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 117 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 862a431..b0b4351 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -13,18 +13,90 @@
 #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;
+    Buffer unflushed_buf;
 } VirtConsole;
 
+static void add_unflushed_buf(VirtConsole *vcon, const uint8_t *buf, size_t len)
+{
+    assert(!vcon->unflushed_buf.buf);
+
+    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)
+{
+    if (vcon->unflushed_buf.buf) {
+        qemu_free(vcon->unflushed_buf.buf);
+        vcon->unflushed_buf.buf = NULL;
+    }
+}
+
+static ssize_t buffered_write_to_chardev(VirtConsole *vcon, const uint8_t *buf,
+                                         size_t len)
+{
+    size_t written;
+    ssize_t ret;
+
+    written = 0;
+    do {
+        ret = qemu_chr_write_nb(vcon->chr, buf + written, len - written);
+        if (ret < 0) {
+            if (vcon->unflushed_buf.buf) {
+                vcon->unflushed_buf.offset += written;
+                vcon->unflushed_buf.rem_len -= written;
+            } else {
+                virtio_serial_throttle_port(&vcon->port, true);
+                add_unflushed_buf(vcon, buf + written, len - written);
+            }
+            return ret;
+        }
+        written += ret;
+    } while (written != len);
+
+    return 0;
+}
+
+/* Callback function called when the chardev can accept more data */
+static void chr_write_unblocked(void *opaque)
+{
+    VirtConsole *vcon = opaque;
+
+    if (vcon->unflushed_buf.buf) {
+        ssize_t ret;
+
+        ret = buffered_write_to_chardev(vcon, vcon->unflushed_buf.buf
+                                              + vcon->unflushed_buf.offset,
+                                        vcon->unflushed_buf.rem_len);
+        if (ret < 0) {
+            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);
+    /* If a previous write was incomplete, we should've been throttled. */
+    assert(!vcon->unflushed_buf.buf);
+
+    buffered_write_to_chardev(vcon, buf, len);
 }
 
 /* Readiness of the guest to accept data on a port */
@@ -48,19 +120,58 @@ 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:
+        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.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) {
+        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 const QemuChrHandlers chr_handlers = {
     .fd_can_read = chr_can_read,
     .fd_read = chr_read,
+    .fd_write_unblocked = chr_write_unblocked,
     .fd_event = chr_event,
 };
 
@@ -72,6 +183,8 @@ static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
         qemu_chr_add_handlers(vcon->chr, &chr_handlers, vcon);
         vcon->port.info->have_data = flush_buf;
     }
+    register_savevm("virtio-console-ports", -1, 1, virtio_console_port_save,
+		    virtio_console_port_load, vcon);
     return 0;
 }
 
@@ -93,6 +206,7 @@ static int virtconsole_exitfn(VirtIOSerialDevice *dev)
     if (vcon->chr) {
         port->info->have_data = NULL;
         qemu_chr_close(vcon->chr);
+        free_unflushed_buf(vcon);
     }
 
     return 0;
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH v7 0/6] char: non-blocking writes, virtio-console flow control

[Amit Shah]

[Fix subject]

		Amit

[Qemu-devel] Re: [PATCH v7 0/6]

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> Hello,
>
> This series lets interested callers ask for an -EAGAIN return from the
> chardev backends (only unix and tcp sockets as of now) to implement
> their own flow control.
>
> A new call, qemu_chr_write_nb() is added, that will fallback to
> qemu_chr_write() if the backend file isn't non-blocking or if no
> callback was supplied.
>
> Support for other backend types is easy to add and will be done in
> later patches.
>
> Please apply.
>
> v7:
> - constify handlers (Blue Swirl)
> - remove 'write_cb', a leftover from previous design (Juan Quintela)
> - return ret instead of -EAGAIN in virtio-console (Juan)
> - use pre-allocated meta-buffer instead of allocating one each time
>   (Juan)
>
> v6:
> - Continue write operation on EINTR instead of returning partial
>   writes (which was a change from prev. behaviour) (Gerd)
>
> v5:
> - Fix bug pointed out by Gerd
> - Convert to using a struct for passing on handlers to
>   qemu_chr_add_handlers() instead of passing each one
>   individually. Simplifies patches. (Inspired by Juan's comment)
> - Re-arranged patches
>
> Amit Shah (6):
>   virtio-console: Factor out common init between console and generic
>     ports
>   char: Add a QemuChrHandlers struct to initialise chardev handlers
>   char: Let writers know how much data was written in case of errors
>   char: Add qemu_chr_write_nb() for nonblocking writes
>   char: unix/tcp: Add a non-blocking write handler
>   virtio-console: Throttle virtio-serial-bus if we can't consume any
>     more guest data

Acked-by: Juan Quintela <quintela@redhat.com>

Testing the porting to vmstate of this.

Later, Juan.

[Qemu-devel] Re: [PATCH v7 2/6] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Anthony Liguori]

On 05/04/2010 04:39 PM, Amit Shah wrote:
> Instead of passing each handler in the qemu_add_handlers() function,
> create a struct of handlers that can be passed to the function instead.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   gdbstub.c            |    9 +++++++--
>   hw/debugcon.c        |    2 +-
>   hw/escc.c            |    9 +++++++--
>   hw/etraxfs_ser.c     |   13 +++++++++----
>   hw/mcf_uart.c        |    9 +++++++--
>   hw/pl011.c           |    9 +++++++--
>   hw/pxa2xx.c          |   13 +++++++++----
>   hw/serial.c          |    9 +++++++--
>   hw/sh_serial.c       |   12 +++++++++---
>   hw/syborg_serial.c   |    9 +++++++--
>   hw/usb-serial.c      |    9 +++++++--
>   hw/virtio-console.c  |    9 +++++++--
>   hw/xen_console.c     |   16 +++++++++++-----
>   hw/xilinx_uartlite.c |   11 +++++++++--
>   monitor.c            |   19 +++++++++++++++----
>   net/slirp.c          |    8 ++++++--
>   qemu-char.c          |   27 ++++++++++++++++++---------
>   qemu-char.h          |   12 ++++++++----
>   18 files changed, 151 insertions(+), 54 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 93c4850..ae29db1 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2621,6 +2621,12 @@ static void gdb_sigterm_handler(int signal)
>   }
>   #endif
>
> +static const QemuChrHandlers gdb_handlers = {
> +    .fd_can_read = gdb_chr_can_receive,
> +    .fd_read = gdb_chr_receive,
> +    .fd_event = gdb_chr_event,
> +};
> +
>   int gdbserver_start(const char *device)
>   {
>       GDBState *s;
> @@ -2650,8 +2656,7 @@ int gdbserver_start(const char *device)
>           if (!chr)
>               return -1;
>
> -        qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
> -                              gdb_chr_event, NULL);
> +        qemu_chr_add_handlers(chr,&gdb_handlers, NULL);
>       }
>    

This sort of change isn't a bad one but if we're making this change, we 
should change the signature of these functions to also take a 
QemuChrHandlers *.

Also, let's drop the qemu_ and Qemu prefixes while we're at it.

Regards,

Anthony Liguori

>       s = gdbserver_state;
> diff --git a/hw/debugcon.c b/hw/debugcon.c
> index 5ee6821..e79a595 100644
> --- a/hw/debugcon.c
> +++ b/hw/debugcon.c
> @@ -73,7 +73,7 @@ static void debugcon_init_core(DebugconState *s)
>           exit(1);
>       }
>
> -    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, s);
> +    qemu_chr_add_handlers(s->chr, NULL, s);
>   }
>
>   static int debugcon_isa_initfn(ISADevice *dev)
> diff --git a/hw/escc.c b/hw/escc.c
> index 6d2fd36..4057bb4 100644
> --- a/hw/escc.c
> +++ b/hw/escc.c
> @@ -890,6 +890,12 @@ void slavio_serial_ms_kbd_init(target_phys_addr_t base, qemu_irq irq,
>       sysbus_mmio_map(s, 0, base);
>   }
>
> +static const QemuChrHandlers serial_handlers = {
> +    .fd_can_read = serial_can_receive,
> +    .fd_read = serial_receive1,
> +    .fd_event = serial_event,
> +};
> +
>   static int escc_init1(SysBusDevice *dev)
>   {
>       SerialState *s = FROM_SYSBUS(SerialState, dev);
> @@ -903,8 +909,7 @@ static int escc_init1(SysBusDevice *dev)
>           s->chn[i].chn = 1 - i;
>           s->chn[i].clock = s->frequency / 2;
>           if (s->chn[i].chr) {
> -            qemu_chr_add_handlers(s->chn[i].chr, serial_can_receive,
> -                                  serial_receive1, serial_event,&s->chn[i]);
> +            qemu_chr_add_handlers(s->chn[i].chr,&serial_handlers,&s->chn[i]);
>           }
>       }
>       s->chn[0].otherchn =&s->chn[1];
> diff --git a/hw/etraxfs_ser.c b/hw/etraxfs_ser.c
> index e1f9615..0c0c485 100644
> --- a/hw/etraxfs_ser.c
> +++ b/hw/etraxfs_ser.c
> @@ -161,6 +161,12 @@ static void serial_event(void *opaque, int event)
>
>   }
>
> +static const QemuChrHandlers serial_handlers = {
> +    .fd_can_read = serial_can_receive,
> +    .fd_read = serial_receive,
> +    .fd_event = serial_event,
> +};
> +
>   static int etraxfs_ser_init(SysBusDevice *dev)
>   {
>       struct etrax_serial *s = FROM_SYSBUS(typeof (*s), dev);
> @@ -174,10 +180,9 @@ static int etraxfs_ser_init(SysBusDevice *dev)
>       ser_regs = cpu_register_io_memory(ser_read, ser_write, s);
>       sysbus_init_mmio(dev, R_MAX * 4, ser_regs);
>       s->chr = qdev_init_chardev(&dev->qdev);
> -    if (s->chr)
> -        qemu_chr_add_handlers(s->chr,
> -                      serial_can_receive, serial_receive,
> -                      serial_event, s);
> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr,&serial_handlers, s);
> +    }
>       return 0;
>   }
>
> diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
> index d16bac7..d2ce5f6 100644
> --- a/hw/mcf_uart.c
> +++ b/hw/mcf_uart.c
> @@ -268,6 +268,12 @@ static void mcf_uart_receive(void *opaque, const uint8_t *buf, int size)
>       mcf_uart_push_byte(s, buf[0]);
>   }
>
> +static const QemuChrHandlers mcf_uart_handlers = {
> +    .fd_can_read = mcf_uart_can_receive,
> +    .fd_read = mcf_uart_receive,
> +    .fd_event = mcf_uart_event,
> +};
> +
>   void *mcf_uart_init(qemu_irq irq, CharDriverState *chr)
>   {
>       mcf_uart_state *s;
> @@ -276,8 +282,7 @@ void *mcf_uart_init(qemu_irq irq, CharDriverState *chr)
>       s->chr = chr;
>       s->irq = irq;
>       if (chr) {
> -        qemu_chr_add_handlers(chr, mcf_uart_can_receive, mcf_uart_receive,
> -                              mcf_uart_event, s);
> +        qemu_chr_add_handlers(chr,&mcf_uart_handlers, s);
>       }
>       mcf_uart_reset(s);
>       return s;
> diff --git a/hw/pl011.c b/hw/pl011.c
> index 81de91e..24711c2 100644
> --- a/hw/pl011.c
> +++ b/hw/pl011.c
> @@ -286,6 +286,12 @@ static int pl011_load(QEMUFile *f, void *opaque, int version_id)
>       return 0;
>   }
>
> +static const QemuChrHandlers pl011_handlers = {
> +    .fd_can_read = pl011_can_receive,
> +    .fd_read = pl011_receive,
> +    .fd_event = pl011_event,
> +};
> +
>   static int pl011_init(SysBusDevice *dev, const unsigned char *id)
>   {
>       int iomemtype;
> @@ -303,8 +309,7 @@ static int pl011_init(SysBusDevice *dev, const unsigned char *id)
>       s->cr = 0x300;
>       s->flags = 0x90;
>       if (s->chr) {
> -        qemu_chr_add_handlers(s->chr, pl011_can_receive, pl011_receive,
> -                              pl011_event, s);
> +        qemu_chr_add_handlers(s->chr,&pl011_handlers, s);
>       }
>       register_savevm("pl011_uart", -1, 1, pl011_save, pl011_load, s);
>       return 0;
> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
> index 9095386..e2adaaf 100644
> --- a/hw/pxa2xx.c
> +++ b/hw/pxa2xx.c
> @@ -1992,6 +1992,12 @@ static int pxa2xx_fir_load(QEMUFile *f, void *opaque, int version_id)
>       return 0;
>   }
>
> +static const QemuChrHandlers pxa2xx_handlers = {
> +    .fd_can_read = pxa2xx_fir_is_empty,
> +    .fd_read = pxa2xx_fir_rx,
> +    .fd_event = pxa2xx_fir_event,
> +};
> +
>   static PXA2xxFIrState *pxa2xx_fir_init(target_phys_addr_t base,
>                   qemu_irq irq, PXA2xxDMAState *dma,
>                   CharDriverState *chr)
> @@ -2010,10 +2016,9 @@ static PXA2xxFIrState *pxa2xx_fir_init(target_phys_addr_t base,
>                       pxa2xx_fir_writefn, s);
>       cpu_register_physical_memory(base, 0x1000, iomemtype);
>
> -    if (chr)
> -        qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
> -                        pxa2xx_fir_rx, pxa2xx_fir_event, s);
> -
> +    if (chr) {
> +        qemu_chr_add_handlers(chr,&pxa2xx_handlers, s);
> +    }
>       register_savevm("pxa2xx_fir", 0, 0, pxa2xx_fir_save, pxa2xx_fir_load, s);
>
>       return s;
> diff --git a/hw/serial.c b/hw/serial.c
> index 90213c4..d4cc317 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -724,6 +724,12 @@ static void serial_reset(void *opaque)
>       qemu_irq_lower(s->irq);
>   }
>
> +static const QemuChrHandlers serial_handlers = {
> +    .fd_can_read = serial_can_receive1,
> +    .fd_read = serial_receive1,
> +    .fd_event = serial_event,
> +};
> +
>   static void serial_init_core(SerialState *s)
>   {
>       if (!s->chr) {
> @@ -738,8 +744,7 @@ static void serial_init_core(SerialState *s)
>
>       qemu_register_reset(serial_reset, s);
>
> -    qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
> -                          serial_event, s);
> +    qemu_chr_add_handlers(s->chr,&serial_handlers, s);
>   }
>
>   /* Change the main reference oscillator frequency. */
> diff --git a/hw/sh_serial.c b/hw/sh_serial.c
> index 93dc144..2201dba 100644
> --- a/hw/sh_serial.c
> +++ b/hw/sh_serial.c
> @@ -363,6 +363,12 @@ static CPUWriteMemoryFunc * const sh_serial_writefn[] = {
>       &sh_serial_write,
>   };
>
> +static const QemuChrHandlers sh_serial_handlers = {
> +    .fd_can_read = sh_serial_can_receive1,
> +    .fd_read = sh_serial_receive1,
> +    .fd_event = sh_serial_event,
> +};
> +
>   void sh_serial_init (target_phys_addr_t base, int feat,
>   		     uint32_t freq, CharDriverState *chr,
>   		     qemu_irq eri_source,
> @@ -401,9 +407,9 @@ void sh_serial_init (target_phys_addr_t base, int feat,
>
>       s->chr = chr;
>
> -    if (chr)
> -        qemu_chr_add_handlers(chr, sh_serial_can_receive1, sh_serial_receive1,
> -			      sh_serial_event, s);
> +    if (chr) {
> +        qemu_chr_add_handlers(chr,&sh_serial_handlers, s);
> +    }
>
>       s->eri = eri_source;
>       s->rxi = rxi_source;
> diff --git a/hw/syborg_serial.c b/hw/syborg_serial.c
> index cac00ea..9ad5ddc 100644
> --- a/hw/syborg_serial.c
> +++ b/hw/syborg_serial.c
> @@ -315,6 +315,12 @@ static int syborg_serial_load(QEMUFile *f, void *opaque, int version_id)
>       return 0;
>   }
>
> +static const QemuChrHandlers syborg_serial_handlers = {
> +    .fd_can_read = syborg_serial_can_receive,
> +    .fd_read = syborg_serial_receive,
> +    .fd_event = syborg_serial_event,
> +};
> +
>   static int syborg_serial_init(SysBusDevice *dev)
>   {
>       SyborgSerialState *s = FROM_SYSBUS(SyborgSerialState, dev);
> @@ -326,8 +332,7 @@ static int syborg_serial_init(SysBusDevice *dev)
>       sysbus_init_mmio(dev, 0x1000, iomemtype);
>       s->chr = qdev_init_chardev(&dev->qdev);
>       if (s->chr) {
> -        qemu_chr_add_handlers(s->chr, syborg_serial_can_receive,
> -                              syborg_serial_receive, syborg_serial_event, s);
> +        qemu_chr_add_handlers(s->chr,&syborg_serial_handlers, s);
>       }
>       if (s->fifo_size<= 0) {
>           fprintf(stderr, "syborg_serial: fifo too small\n");
> diff --git a/hw/usb-serial.c b/hw/usb-serial.c
> index 5b2483a..56e2b00 100644
> --- a/hw/usb-serial.c
> +++ b/hw/usb-serial.c
> @@ -540,13 +540,18 @@ static void usb_serial_event(void *opaque, int event)
>       }
>   }
>
> +static const QemuChrHandlers usb_serial_handlers = {
> +    .fd_can_read = usb_serial_can_read,
> +    .fd_read = usb_serial_read,
> +    .fd_event = usb_serial_event,
> +};
> +
>   static int usb_serial_initfn(USBDevice *dev)
>   {
>       USBSerialState *s = DO_UPCAST(USBSerialState, dev, dev);
>       s->dev.speed = USB_SPEED_FULL;
>
> -    qemu_chr_add_handlers(s->cs, usb_serial_can_read, usb_serial_read,
> -                          usb_serial_event, s);
> +    qemu_chr_add_handlers(s->cs,&usb_serial_handlers, s);
>       usb_serial_handle_reset(dev);
>       return 0;
>   }
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index d7fe68b..862a431 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -58,13 +58,18 @@ static void chr_event(void *opaque, int event)
>       }
>   }
>
> +static const QemuChrHandlers chr_handlers = {
> +    .fd_can_read = chr_can_read,
> +    .fd_read = chr_read,
> +    .fd_event = chr_event,
> +};
> +
>   static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
>   {
>       vcon->port.info = dev->info;
>
>       if (vcon->chr) {
> -        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
> -                              vcon);
> +        qemu_chr_add_handlers(vcon->chr,&chr_handlers, vcon);
>           vcon->port.info->have_data = flush_buf;
>       }
>       return 0;
> diff --git a/hw/xen_console.c b/hw/xen_console.c
> index d2261f4..8327e4e 100644
> --- a/hw/xen_console.c
> +++ b/hw/xen_console.c
> @@ -202,6 +202,11 @@ static int con_init(struct XenDevice *xendev)
>       return 0;
>   }
>
> +static const QemuChrHandlers xencons_handlers = {
> +    .fd_can_read = xencons_can_receive,
> +    .fd_read = xencons_receive,
> +};
> +
>   static int con_connect(struct XenDevice *xendev)
>   {
>       struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
> @@ -222,9 +227,9 @@ static int con_connect(struct XenDevice *xendev)
>   	return -1;
>
>       xen_be_bind_evtchn(&con->xendev);
> -    if (con->chr)
> -        qemu_chr_add_handlers(con->chr, xencons_can_receive, xencons_receive,
> -                              NULL, con);
> +    if (con->chr) {
> +        qemu_chr_add_handlers(con->chr,&xencons_handlers, con);
> +    }
>
>       xen_be_printf(xendev, 1, "ring mfn %d, remote port %d, local port %d, limit %zd\n",
>   		  con->ring_ref,
> @@ -238,8 +243,9 @@ static void con_disconnect(struct XenDevice *xendev)
>   {
>       struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
>
> -    if (con->chr)
> -        qemu_chr_add_handlers(con->chr, NULL, NULL, NULL, NULL);
> +    if (con->chr) {
> +        qemu_chr_add_handlers(con->chr, NULL, NULL);
> +    }
>       xen_be_unbind_evtchn(&con->xendev);
>
>       if (con->sring) {
> diff --git a/hw/xilinx_uartlite.c b/hw/xilinx_uartlite.c
> index adab759..66f0396 100644
> --- a/hw/xilinx_uartlite.c
> +++ b/hw/xilinx_uartlite.c
> @@ -193,6 +193,12 @@ static void uart_event(void *opaque, int event)
>
>   }
>
> +static const QemuChrHandlers uart_handlers = {
> +    .fd_can_read = uart_can_rx,
> +    .fd_read = uart_rx,
> +    .fd_event = uart_event,
> +};
> +
>   static int xilinx_uartlite_init(SysBusDevice *dev)
>   {
>       struct xlx_uartlite *s = FROM_SYSBUS(typeof (*s), dev);
> @@ -205,8 +211,9 @@ static int xilinx_uartlite_init(SysBusDevice *dev)
>       sysbus_init_mmio(dev, R_MAX * 4, uart_regs);
>
>       s->chr = qdev_init_chardev(&dev->qdev);
> -    if (s->chr)
> -        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr,&uart_handlers, s);
> +    }
>       return 0;
>   }
>
> diff --git a/monitor.c b/monitor.c
> index 46d0b47..22a61d4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4600,6 +4600,18 @@ static void monitor_event(void *opaque, int event)
>    * End:
>    */
>
> +static const QemuChrHandlers monitor_handlers = {
> +    .fd_can_read = monitor_can_read,
> +    .fd_read = monitor_read,
> +    .fd_event = monitor_event,
> +};
> +
> +static const QemuChrHandlers monitor_control_handlers = {
> +    .fd_can_read = monitor_can_read,
> +    .fd_read = monitor_control_read,
> +    .fd_event = monitor_control_event,
> +};
> +
>   void monitor_init(CharDriverState *chr, int flags)
>   {
>       static int is_first_init = 1;
> @@ -4621,12 +4633,11 @@ void monitor_init(CharDriverState *chr, int flags)
>
>       if (monitor_ctrl_mode(mon)) {
>           mon->mc = qemu_mallocz(sizeof(MonitorControl));
> +
>           /* Control mode requires special handlers */
> -        qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
> -                              monitor_control_event, mon);
> +        qemu_chr_add_handlers(chr,&monitor_control_handlers, mon);
>       } else {
> -        qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
> -                              monitor_event, mon);
> +        qemu_chr_add_handlers(chr,&monitor_handlers, mon);
>       }
>
>       QLIST_INSERT_HEAD(&mon_list, mon, entry);
> diff --git a/net/slirp.c b/net/slirp.c
> index b41c60a..437be46 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -577,6 +577,11 @@ static void guestfwd_read(void *opaque, const uint8_t *buf, int size)
>       slirp_socket_recv(fwd->slirp, fwd->server, fwd->port, buf, size);
>   }
>
> +static const QemuChrHandlers guestfwd_handlers = {
> +    .fd_can_read = guestfwd_can_read,
> +    .fd_read = guestfwd_read,
> +};
> +
>   static int slirp_guestfwd(SlirpState *s, const char *config_str,
>                             int legacy_format)
>   {
> @@ -633,8 +638,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
>       fwd->port = port;
>       fwd->slirp = s->slirp;
>
> -    qemu_chr_add_handlers(fwd->hd, guestfwd_can_read, guestfwd_read,
> -                          NULL, fwd);
> +    qemu_chr_add_handlers(fwd->hd,&guestfwd_handlers, fwd);
>       return 0;
>
>    fail_syntax:
> diff --git a/qemu-char.c b/qemu-char.c
> index ac65a1c..76ad12c 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -191,15 +191,19 @@ void qemu_chr_send_event(CharDriverState *s, int event)
>           s->chr_send_event(s, event);
>   }
>
> +static const QemuChrHandlers null_handlers = {
> +    /* All handlers are initialised to NULL */
> +};
> +
>   void qemu_chr_add_handlers(CharDriverState *s,
> -                           IOCanReadHandler *fd_can_read,
> -                           IOReadHandler *fd_read,
> -                           IOEventHandler *fd_event,
> -                           void *opaque)
> -{
> -    s->chr_can_read = fd_can_read;
> -    s->chr_read = fd_read;
> -    s->chr_event = fd_event;
> +                           const QemuChrHandlers *handlers, void *opaque)
> +{
> +    if (!handlers) {
> +        handlers =&null_handlers;
> +    }
> +    s->chr_can_read = handlers->fd_can_read;
> +    s->chr_read = handlers->fd_read;
> +    s->chr_event = handlers->fd_event;
>       s->handler_opaque = opaque;
>       if (s->chr_update_read_handler)
>           s->chr_update_read_handler(s);
> @@ -442,6 +446,12 @@ static void mux_chr_event(void *opaque, int event)
>           mux_chr_send_event(d, i, event);
>   }
>
> +static const QemuChrHandlers mux_chr_handlers = {
> +    .fd_can_read = mux_chr_can_read,
> +    .fd_read = mux_chr_read,
> +    .fd_event = mux_chr_event,
> +};
> +
>   static void mux_chr_update_read_handler(CharDriverState *chr)
>   {
>       MuxDriver *d = chr->opaque;
> @@ -456,8 +466,7 @@ static void mux_chr_update_read_handler(CharDriverState *chr)
>       d->chr_event[d->mux_cnt] = chr->chr_event;
>       /* Fix up the real driver with mux routines */
>       if (d->mux_cnt == 0) {
> -        qemu_chr_add_handlers(d->drv, mux_chr_can_read, mux_chr_read,
> -                              mux_chr_event, chr);
> +        qemu_chr_add_handlers(d->drv,&mux_chr_handlers, chr);
>       }
>       if (d->focus != -1) {
>           mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> diff --git a/qemu-char.h b/qemu-char.h
> index e3a0783..eacb3b9 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -71,6 +71,13 @@ struct CharDriverState {
>       QTAILQ_ENTRY(CharDriverState) next;
>   };
>
> +typedef struct QemuChrHandlers {
> +    IOCanReadHandler *fd_can_read;
> +    IOReadHandler *fd_read;
> +    IOHandler *fd_write_unblocked;
> +    IOEventHandler *fd_event;
> +} QemuChrHandlers;
> +
>   QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
>   CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
>                                       void (*init)(struct CharDriverState *s));
> @@ -79,10 +86,7 @@ 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);
>   void qemu_chr_send_event(CharDriverState *s, int event);
> -void qemu_chr_add_handlers(CharDriverState *s,
> -                           IOCanReadHandler *fd_can_read,
> -                           IOReadHandler *fd_read,
> -                           IOEventHandler *fd_event,
> +void qemu_chr_add_handlers(CharDriverState *s, const QemuChrHandlers *handlers,
>                              void *opaque);
>   int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
>   void qemu_chr_generic_open(CharDriverState *s);
>    

[Qemu-devel] Re: [PATCH v7 3/6] char: Let writers know how much data was written in case of errors

[Anthony Liguori]

On 05/04/2010 04:39 PM, Amit Shah wrote:
> On writing errors, we just returned -1 even if some bytes were already
> written out. Ensure we return the number of bytes written before we
> return the error (on a subsequent call to qemu_chr_write()).
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   qemu-char.c |   12 +++++++++++-
>   1 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 76ad12c..decf687 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -507,6 +507,9 @@ int send_all(int fd, const void *buf, int len1)
>       while (len>  0) {
>           ret = send(fd, buf, len, 0);
>           if (ret<  0) {
> +            if (len1 - len) {
> +                return len1 - len;
> +            }
>               errno = WSAGetLastError();
>               if (errno != WSAEWOULDBLOCK) {
>                   return -1;
> @@ -531,8 +534,15 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>       while (len>  0) {
>           ret = write(fd, buf, len);
>           if (ret<  0) {
> -            if (errno != EINTR&&  errno != EAGAIN)
> +            if (errno == EINTR) {
> +                continue;
> +            }
> +            if (len1 - len) {
> +                return len1 - len;
> +            }
> +            if (errno != EAGAIN) {
>                   return -1;
> +            }
>           } else if (ret == 0) {
>               break;
>           } else {
>    

This will break lots of things.  The contract for send_all and 
unix_write is that the transmit all data.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Anthony Liguori]

On 05/04/2010 04:39 PM, Amit Shah wrote:
> For char devices whose backing files are open in non-blocking mode,
> non-blocking writes can now be made using qemu_chr_write_nb().
>
> For non-blocking chardevs, we can return -EAGAIN to callers of
> qemu_chr_write_nb(). When the backend is ready to accept more data,
> we can let the caller know via a callback.
>
> -EAGAIN is returned only when the backend's file is non-blocking
> and a callback is registered by the caller when invoking
> qemu_chr_add_handlers().
>
> In case a backend doesn't support non-blocking writes,
> qemu_chr_write_nb() invokes qemu_chr_write().
>
> Individual callers can update their code to add a callback handler,
> call qemu_chr_write_nb() instead of qemu_chr_write() if they wish to
> receive -EAGAIN notifications.
>
> No backend currently supports non-blocking writes.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   net/socket.c  |    4 ++--
>   qemu-char.c   |   31 ++++++++++++++++++++++++-------
>   qemu-char.h   |    8 ++++++++
>   qemu_socket.h |    3 ++-
>   4 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 1c4e153..8a401e6 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -56,8 +56,8 @@ static ssize_t net_socket_receive(VLANClientState *nc, const uint8_t *buf, size_
>       uint32_t len;
>       len = htonl(size);
>
> -    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, sizeof(len), false);
> +    return send_all(s->fd, buf, size, false);
>   }
>
>   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 decf687..5e4dec3 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -145,6 +145,16 @@ int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len)
>       return s->chr_write(s, buf, len);
>   }
>
> +ssize_t qemu_chr_write_nb(CharDriverState *s, const uint8_t *buf, size_t len)
> +{
> +    if (!s->nonblock) {
> +        /* Fallback to blocking write if no callback registered */
> +        return qemu_chr_write(s, buf, len);
> +    }
> +
> +    return s->chr_write_nb(s, buf, len);
> +}
>    

I really dislike the idea of adding another function for this.  Can you 
explain why you need this functionality for virtio-console and why this 
functionality isn't needed for everything else?

Regards,

Anthony Liguori

>   int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
>   {
>       if (!s->chr_ioctl)
> @@ -203,11 +213,15 @@ void qemu_chr_add_handlers(CharDriverState *s,
>       }
>       s->chr_can_read = handlers->fd_can_read;
>       s->chr_read = handlers->fd_read;
> +    s->chr_write_unblocked = handlers->fd_write_unblocked;
>       s->chr_event = handlers->fd_event;
>       s->handler_opaque = opaque;
>       if (s->chr_update_read_handler)
>           s->chr_update_read_handler(s);
>
> +    /* We'll set this at connect-time */
> +    s->nonblock = false;
> +
>       /* We're connecting to an already opened device, so let's make sure we
>          also get the open event */
>       if (s->opened) {
> @@ -499,7 +513,7 @@ 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, int len1, bool nonblock)
>   {
>       int ret, len;
>
> @@ -526,7 +540,7 @@ 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, int len1, bool nonblock)
>   {
>       int ret, len;
>
> @@ -540,6 +554,9 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>               if (len1 - len) {
>                   return len1 - len;
>               }
> +            if (errno == EAGAIN&&  nonblock) {
> +                return -EAGAIN;
> +            }
>               if (errno != EAGAIN) {
>                   return -1;
>               }
> @@ -553,9 +570,9 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>       return len1 - len;
>   }
>
> -int send_all(int fd, const void *buf, int len1)
> +int send_all(int fd, const void *buf, int len1, bool nonblock)
>   {
> -    return unix_write(fd, buf, len1);
> +    return unix_write(fd, buf, len1, nonblock);
>   }
>   #endif /* !_WIN32 */
>
> @@ -572,7 +589,7 @@ static int stdio_nb_clients = 0;
>   static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>   {
>       FDCharDriver *s = chr->opaque;
> -    return send_all(s->fd_out, buf, len);
> +    return send_all(s->fd_out, buf, len, false);
>   }
>
>   static int fd_chr_read_poll(void *opaque)
> @@ -884,7 +901,7 @@ static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>           pty_chr_update_read_handler(chr);
>           return 0;
>       }
> -    return send_all(s->fd, buf, len);
> +    return send_all(s->fd, buf, len, false);
>   }
>
>   static int pty_chr_read_poll(void *opaque)
> @@ -1949,7 +1966,7 @@ static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>   {
>       TCPCharDriver *s = chr->opaque;
>       if (s->connected) {
> -        return send_all(s->fd, buf, len);
> +        return send_all(s->fd, buf, len, false);
>       } else {
>           /* XXX: indicate an error ? */
>           return len;
> diff --git a/qemu-char.h b/qemu-char.h
> index eacb3b9..52f9ef1 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"
> @@ -53,12 +55,15 @@ 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);
> +    ssize_t (*chr_write_nb)(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);
>       IOEventHandler *chr_event;
>       IOCanReadHandler *chr_can_read;
>       IOReadHandler *chr_read;
> +    IOHandler *chr_write_unblocked;
>       void *handler_opaque;
>       void (*chr_send_event)(struct CharDriverState *chr, int event);
>       void (*chr_close)(struct CharDriverState *chr);
> @@ -68,6 +73,8 @@ struct CharDriverState {
>       char *label;
>       char *filename;
>       int opened;
> +    bool nonblock;
> +    bool write_blocked;
>       QTAILQ_ENTRY(CharDriverState) next;
>   };
>
> @@ -85,6 +92,7 @@ CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*i
>   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);
> +ssize_t qemu_chr_write_nb(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, const QemuChrHandlers *handlers,
>                              void *opaque);
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 164ae3e..bdf878b 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -23,6 +23,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
>   #include<arpa/inet.h>
>   #include<netdb.h>
>   #include<sys/un.h>
> +#include<stdbool.h>
>
>   #define socket_error() errno
>   #define closesocket(s) close(s)
> @@ -35,7 +36,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, int len1, bool nonblock);
>
>   /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
>   int inet_listen_opts(QemuOpts *opts, int port_offset);
>    

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Amit Shah]

On (Wed) May 05 2010 [08:16:37], Anthony Liguori wrote:
> On 05/04/2010 04:39 PM, Amit Shah wrote:
>> For char devices whose backing files are open in non-blocking mode,
>> non-blocking writes can now be made using qemu_chr_write_nb().
>>
>> For non-blocking chardevs, we can return -EAGAIN to callers of
>> qemu_chr_write_nb(). When the backend is ready to accept more data,
>> we can let the caller know via a callback.
>>
>> -EAGAIN is returned only when the backend's file is non-blocking
>> and a callback is registered by the caller when invoking
>> qemu_chr_add_handlers().
>>
>> In case a backend doesn't support non-blocking writes,
>> qemu_chr_write_nb() invokes qemu_chr_write().
>>
>> Individual callers can update their code to add a callback handler,
>> call qemu_chr_write_nb() instead of qemu_chr_write() if they wish to
>> receive -EAGAIN notifications.
>>
>> No backend currently supports non-blocking writes.
>>
>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>
>> +ssize_t qemu_chr_write_nb(CharDriverState *s, const uint8_t *buf, size_t len)
>> +{
>> +    if (!s->nonblock) {
>> +        /* Fallback to blocking write if no callback registered */
>> +        return qemu_chr_write(s, buf, len);
>> +    }
>> +
>> +    return s->chr_write_nb(s, buf, len);
>> +}
>>    
>
> I really dislike the idea of adding another function for this.

This suggestion came from Gerd to separate out a write() that blocks and
a write_nb() that doesn't block on -EAGAIN.

>  Can you  
> explain why you need this functionality for virtio-console and why this  
> functionality isn't needed for everything else?

I don't know about everything else; but for virtio-console, a fast guest
could swamp a host chardev with data and while the host chardev blocks
to flush out all the data, a notification to ask the guest to stop is
needed so that we don't lose any data.

(virtio-console asks virtio-serial to throttle any more data, and
virtio-serial doesn't send any more data to this port till it signals
otherwise. The guest, in the meantime, keeps filling its queue till the
queue is full. Once that happens, writes in guests return with -EAGAIN.)

		Amit

[Qemu-devel] Re: [PATCH v7 3/6] char: Let writers know how much data was written in case of errors

[Amit Shah]

On (Wed) May 05 2010 [08:15:19], Anthony Liguori wrote:
> On 05/04/2010 04:39 PM, Amit Shah wrote:
>> On writing errors, we just returned -1 even if some bytes were already
>> written out. Ensure we return the number of bytes written before we
>> return the error (on a subsequent call to qemu_chr_write()).
>>
>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>> ---
>>   qemu-char.c |   12 +++++++++++-
>>   1 files changed, 11 insertions(+), 1 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 76ad12c..decf687 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -507,6 +507,9 @@ int send_all(int fd, const void *buf, int len1)
>>       while (len>  0) {
>>           ret = send(fd, buf, len, 0);
>>           if (ret<  0) {
>> +            if (len1 - len) {
>> +                return len1 - len;
>> +            }
>>               errno = WSAGetLastError();
>>               if (errno != WSAEWOULDBLOCK) {
>>                   return -1;
>> @@ -531,8 +534,15 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>>       while (len>  0) {
>>           ret = write(fd, buf, len);
>>           if (ret<  0) {
>> -            if (errno != EINTR&&  errno != EAGAIN)
>> +            if (errno == EINTR) {
>> +                continue;
>> +            }
>> +            if (len1 - len) {
>> +                return len1 - len;
>> +            }
>> +            if (errno != EAGAIN) {
>>                   return -1;
>> +            }
>>           } else if (ret == 0) {
>>               break;
>>           } else {
>>    
>
> This will break lots of things.  The contract for send_all and  
> unix_write is that the transmit all data.

The current behaviour remains unchanged for all the users. Only callers
of qemu_chr_write_nb() will get an -EAGAIN return.

		Amit

[Qemu-devel] Re: [PATCH v7 2/6] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Amit Shah]

On (Wed) May 05 2010 [08:13:58], Anthony Liguori wrote:
> On 05/04/2010 04:39 PM, Amit Shah wrote:
>> Instead of passing each handler in the qemu_add_handlers() function,
>> create a struct of handlers that can be passed to the function instead.
>>
>> Signed-off-by: Amit Shah<amit.shah@redhat.com>


>> -        qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
>> -                              gdb_chr_event, NULL);
>> +        qemu_chr_add_handlers(chr,&gdb_handlers, NULL);
>>       }
>>    
>
> This sort of change isn't a bad one but if we're making this change, we  
> should change the signature of these functions to also take a  
> QemuChrHandlers *.

What do you mean?

> Also, let's drop the qemu_ and Qemu prefixes while we're at it.

In a later series, please. -ETOOMANYRESENDS already..

		Amit

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Paul Brook]

> I really dislike the idea of adding another function for this.  Can you
> explain why you need this functionality for virtio-console and why this
> functionality isn't needed for everything else?

This functionality should (in principle) be used by all serial port 
implementations.

Physical serial ports are sufficiently crufty and low-performance that noone 
actually uses them nowadays.  I expect that the only significant real-world 
use is for serial consoles, which never send enough data to care that writes 
stall the whole machine.

With virtio-serial we've made serial ports a viable solution to a whole range 
of problems.  It's likely that applications that may send nontrivial amounts 
of data, or clients will not be ready to process the data immediately.

Paul

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Anthony Liguori]

On 05/05/2010 08:34 AM, Paul Brook wrote:
>> I really dislike the idea of adding another function for this.  Can you
>> explain why you need this functionality for virtio-console and why this
>> functionality isn't needed for everything else?
>>      
> This functionality should (in principle) be used by all serial port
> implementations.
>
> Physical serial ports are sufficiently crufty and low-performance that noone
> actually uses them nowadays.  I expect that the only significant real-world
> use is for serial consoles, which never send enough data to care that writes
> stall the whole machine.
>    

We don't implement control flow in the character driver layer today.  
Different backends use different policies.  Some drop data (like pty) 
while other block (like tcp).

This patch adds optional control flow in a pretty crufty way to *some* 
backends but not all.  This just adds a bunch of complexity that will 
certainly introduce bugs.

If we're going to make the char drivers implement control flow, then I 
think we should do it universally--not as an optional feature.  For 
devices that can't participate in control flow, we should decide where 
the policy should be implemented (front-end or back-end) and in either 
approach, it's easy enough to make dropping data or blocking a choice.

Regards,

Anthony Liguori

> With virtio-serial we've made serial ports a viable solution to a whole range
> of problems.  It's likely that applications that may send nontrivial amounts
> of data, or clients will not be ready to process the data immediately.
>
> Paul
>    

[Qemu-devel] Re: [PATCH v7 3/6] char: Let writers know how much data was written in case of errors

[Anthony Liguori]

On 05/05/2010 08:23 AM, Amit Shah wrote:
> On (Wed) May 05 2010 [08:15:19], Anthony Liguori wrote:
>    
>> On 05/04/2010 04:39 PM, Amit Shah wrote:
>>      
>>> On writing errors, we just returned -1 even if some bytes were already
>>> written out. Ensure we return the number of bytes written before we
>>> return the error (on a subsequent call to qemu_chr_write()).
>>>
>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>> ---
>>>    qemu-char.c |   12 +++++++++++-
>>>    1 files changed, 11 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/qemu-char.c b/qemu-char.c
>>> index 76ad12c..decf687 100644
>>> --- a/qemu-char.c
>>> +++ b/qemu-char.c
>>> @@ -507,6 +507,9 @@ int send_all(int fd, const void *buf, int len1)
>>>        while (len>   0) {
>>>            ret = send(fd, buf, len, 0);
>>>            if (ret<   0) {
>>> +            if (len1 - len) {
>>> +                return len1 - len;
>>> +            }
>>>                errno = WSAGetLastError();
>>>                if (errno != WSAEWOULDBLOCK) {
>>>                    return -1;
>>> @@ -531,8 +534,15 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>>>        while (len>   0) {
>>>            ret = write(fd, buf, len);
>>>            if (ret<   0) {
>>> -            if (errno != EINTR&&   errno != EAGAIN)
>>> +            if (errno == EINTR) {
>>> +                continue;
>>> +            }
>>> +            if (len1 - len) {
>>> +                return len1 - len;
>>> +            }
>>> +            if (errno != EAGAIN) {
>>>                    return -1;
>>> +            }
>>>            } else if (ret == 0) {
>>>                break;
>>>            } else {
>>>
>>>        
>> This will break lots of things.  The contract for send_all and
>> unix_write is that the transmit all data.
>>      
> The current behaviour remains unchanged for all the users. Only callers
> of qemu_chr_write_nb() will get an -EAGAIN return.
>    

No, send_all used to send all data.  After your change, it only sends 
what it can the first time.  The same with unix_write.

Regards,

Anthony Liguori

> 		Amit
>    

[Qemu-devel] Re: [PATCH v7 2/6] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Anthony Liguori]

On 05/05/2010 08:25 AM, Amit Shah wrote:
> On (Wed) May 05 2010 [08:13:58], Anthony Liguori wrote:
>    
>> On 05/04/2010 04:39 PM, Amit Shah wrote:
>>      
>>> Instead of passing each handler in the qemu_add_handlers() function,
>>> create a struct of handlers that can be passed to the function instead.
>>>
>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>        
>
>    
>>> -        qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
>>> -                              gdb_chr_event, NULL);
>>> +        qemu_chr_add_handlers(chr,&gdb_handlers, NULL);
>>>        }
>>>
>>>        
>> This sort of change isn't a bad one but if we're making this change, we
>> should change the signature of these functions to also take a
>> QemuChrHandlers *.
>>      
> What do you mean?
>    

-typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
+typedef void IOReadHandler(QemuChrHandlers *handler, const uint8_t 
*buf, int size);

Then you can embed the structure in the state structure and use 
container_of to get the state.  It's more type safe and it avoids adding 
a global variable.

>> Also, let's drop the qemu_ and Qemu prefixes while we're at it.
>>      
> In a later series, please. -ETOOMANYRESENDS already..
>    

It's part of CODING_STYLE...  I think it's important to change the 
signatures so a new series will be required.

Regards,

Anthony Liguori

> 		Amit
>    

[Qemu-devel] Re: [PATCH v7 3/6] char: Let writers know how much data was written in case of errors

[Amit Shah]

On (Wed) May 05 2010 [08:54:48], Anthony Liguori wrote:
> On 05/05/2010 08:23 AM, Amit Shah wrote:
>> On (Wed) May 05 2010 [08:15:19], Anthony Liguori wrote:
>>    
>>> On 05/04/2010 04:39 PM, Amit Shah wrote:
>>>      
>>>> On writing errors, we just returned -1 even if some bytes were already
>>>> written out. Ensure we return the number of bytes written before we
>>>> return the error (on a subsequent call to qemu_chr_write()).
>>>>
>>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>> ---
>>>>    qemu-char.c |   12 +++++++++++-
>>>>    1 files changed, 11 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/qemu-char.c b/qemu-char.c
>>>> index 76ad12c..decf687 100644
>>>> --- a/qemu-char.c
>>>> +++ b/qemu-char.c
>>>> @@ -507,6 +507,9 @@ int send_all(int fd, const void *buf, int len1)
>>>>        while (len>   0) {
>>>>            ret = send(fd, buf, len, 0);
>>>>            if (ret<   0) {
>>>> +            if (len1 - len) {
>>>> +                return len1 - len;
>>>> +            }
>>>>                errno = WSAGetLastError();
>>>>                if (errno != WSAEWOULDBLOCK) {
>>>>                    return -1;
>>>> @@ -531,8 +534,15 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>>>>        while (len>   0) {
>>>>            ret = write(fd, buf, len);
>>>>            if (ret<   0) {
>>>> -            if (errno != EINTR&&   errno != EAGAIN)
>>>> +            if (errno == EINTR) {
>>>> +                continue;
>>>> +            }
>>>> +            if (len1 - len) {
>>>> +                return len1 - len;
>>>> +            }
>>>> +            if (errno != EAGAIN) {
>>>>                    return -1;
>>>> +            }
>>>>            } else if (ret == 0) {
>>>>                break;
>>>>            } else {
>>>>
>>>>        
>>> This will break lots of things.  The contract for send_all and
>>> unix_write is that the transmit all data.
>>>      
>> The current behaviour remains unchanged for all the users. Only callers
>> of qemu_chr_write_nb() will get an -EAGAIN return.
>>    
>
> No, send_all used to send all data.  After your change, it only sends  
> what it can the first time.  The same with unix_write.

Where do you see this?

		Amit

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Paul Brook]

> On 05/05/2010 08:34 AM, Paul Brook wrote:
> >> I really dislike the idea of adding another function for this.  Can you
> >> explain why you need this functionality for virtio-console and why this
> >> functionality isn't needed for everything else?
> > 
> > This functionality should (in principle) be used by all serial port
> > implementations.
> > 
> > Physical serial ports are sufficiently crufty and low-performance that
> > noone actually uses them nowadays.  I expect that the only significant
> > real-world use is for serial consoles, which never send enough data to
> > care that writes stall the whole machine.
> 
> We don't implement control flow in the character driver layer today.
> Different backends use different policies.  Some drop data (like pty)
> while other block (like tcp).

Really? I thought data was only dropped when no client was connected, and that 
there was a user visible option to control this.  Either way, I agree that 
this should be done consistently.
 
> This patch adds optional control flow in a pretty crufty way to *some*
> backends but not all.  This just adds a bunch of complexity that will
> certainly introduce bugs.

I admit I've only really looked at the device emulation side of the interface, 
not the chardev backend implementation.

Paul

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Gerd Hoffmann]

>> + return s->chr_write_nb(s, buf, len);
>> +}
>
> I really dislike the idea of adding another function for this.

Having a explicit function for non-blocking mode IMHO is much better. 
It makes things more clear when reading the code.

Previous approach was to check for O_NONBLOCK (extra syscall) and behave 
differently depending on whenever it is set or not (too much hidden magic).

> Can you
> explain why you need this functionality for virtio-console and why this
> functionality isn't needed for everything else?

virtio-console is just the first user.  16550 emulation can use that 
too.  With the blocking interface you'll stall the guest (sitting in the 
blocking write syscall) in case the outgoing pipe is full.  With the 
non-blocking interface you can simply delay the "you can send more data" 
signal (update status register + raise IRQ) until you can actually 
accept more data.  Meanwhile the guest can continue to run and do 
something else.  Basically it allows to implement sane flow control.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Gerd Hoffmann]

On 05/05/10 15:53, Anthony Liguori wrote:
> This patch adds optional control flow in a pretty crufty way to *some*
> backends but not all. This just adds a bunch of complexity that will
> certainly introduce bugs.

Amit plans to add support to the others as well.  Beside that there is a 
clearly defined backup plan:  In case the non-blocking interface isn't 
supported by $chardev it will fallback to use the blocking mode.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Anthony Liguori]

On 05/05/2010 01:40 PM, Gerd Hoffmann wrote:
>>> + return s->chr_write_nb(s, buf, len);
>>> +}
>>
>> I really dislike the idea of adding another function for this.
>
> Having a explicit function for non-blocking mode IMHO is much better. 
> It makes things more clear when reading the code.
>
> Previous approach was to check for O_NONBLOCK (extra syscall) and 
> behave differently depending on whenever it is set or not (too much 
> hidden magic).
>
>> Can you
>> explain why you need this functionality for virtio-console and why this
>> functionality isn't needed for everything else?
>
> virtio-console is just the first user.  16550 emulation can use that 
> too.  With the blocking interface you'll stall the guest (sitting in 
> the blocking write syscall) in case the outgoing pipe is full.  With 
> the non-blocking interface you can simply delay the "you can send more 
> data" signal (update status register + raise IRQ) until you can 
> actually accept more data.  Meanwhile the guest can continue to run 
> and do something else.  Basically it allows to implement sane flow 
> control.

The use of char drivers is dominated by a small number of use cases 
which include serial ports, the monitor, and QMP.

In all of those cases, the devices can and should participate in control 
flow.  IMHO, we should make control flow part of the standard interface 
and fix all users appropriately.

I've seen blocking result in frozen guests many times in real 
deployments.  It's never the right thing to do as far as I can tell.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Anthony Liguori]

On 05/05/2010 01:43 PM, Gerd Hoffmann wrote:
> On 05/05/10 15:53, Anthony Liguori wrote:
>> This patch adds optional control flow in a pretty crufty way to *some*
>> backends but not all. This just adds a bunch of complexity that will
>> certainly introduce bugs.
>
> Amit plans to add support to the others as well.  Beside that there is 
> a clearly defined backup plan:  In case the non-blocking interface 
> isn't supported by $chardev it will fallback to use the blocking mode.

If we have a second interface, we'll have two interfaces forever.  I'd 
rather see us aggressive remove the blocking interface instead of 
introducing a second interface.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Gerd Hoffmann]

On 05/05/10 20:49, Anthony Liguori wrote:
> On 05/05/2010 01:43 PM, Gerd Hoffmann wrote:
>> On 05/05/10 15:53, Anthony Liguori wrote:
>>> This patch adds optional control flow in a pretty crufty way to *some*
>>> backends but not all. This just adds a bunch of complexity that will
>>> certainly introduce bugs.
>>
>> Amit plans to add support to the others as well. Beside that there is
>> a clearly defined backup plan: In case the non-blocking interface
>> isn't supported by $chardev it will fallback to use the blocking mode.
>
> If we have a second interface, we'll have two interfaces forever. I'd
> rather see us aggressive remove the blocking interface instead of
> introducing a second interface.

I'm all for killing the blocking interface.  Problem is that converting 
over all users isn't exactly trivial and we have plenty of them.  IMHO 
it isn't realistic to do the switch with a single patch series.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Anthony Liguori]

On 05/05/2010 02:16 PM, Gerd Hoffmann wrote:
> On 05/05/10 20:49, Anthony Liguori wrote:
>> On 05/05/2010 01:43 PM, Gerd Hoffmann wrote:
>>> On 05/05/10 15:53, Anthony Liguori wrote:
>>>> This patch adds optional control flow in a pretty crufty way to *some*
>>>> backends but not all. This just adds a bunch of complexity that will
>>>> certainly introduce bugs.
>>>
>>> Amit plans to add support to the others as well. Beside that there is
>>> a clearly defined backup plan: In case the non-blocking interface
>>> isn't supported by $chardev it will fallback to use the blocking mode.
>>
>> If we have a second interface, we'll have two interfaces forever. I'd
>> rather see us aggressive remove the blocking interface instead of
>> introducing a second interface.
>
> I'm all for killing the blocking interface.  Problem is that 
> converting over all users isn't exactly trivial and we have plenty of 
> them.  IMHO it isn't realistic to do the switch with a single patch 
> series.

If we're agreed we ought to kill the blocking interface, let's define a 
new proper interface, rename all previous users to something ugly and 
deprecated, and approach it that way.

Let's make it clear what interfaces are supported and which interfaces 
are scheduled to be removed.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Gerd Hoffmann]

>> I'm all for killing the blocking interface. Problem is that converting
>> over all users isn't exactly trivial and we have plenty of them. IMHO
>> it isn't realistic to do the switch with a single patch series.
>
> If we're agreed we ought to kill the blocking interface, let's define a
> new proper interface, rename all previous users to something ugly and
> deprecated, and approach it that way.

__attribute__ ((deprecated)) ?

cheers,
   Gerd