[Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes

[Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes

[Amit Shah]

Hello,

This version of the series adds proper handling of nonblocking when
the backend managed to flush out some data but not all (ret > 0 && ret
< len).

Just the unix/tcp backends have been updated to be nonblocking, but
it's easy to add support to the other backends, as the last patch in
the series shows.

I'll work on that once this is applied.

The virtio-console code will be the first user of this work, that
series will be sent shortly.

This series depends on the virtio-serial fixes and enhancements series
that I sent out a short while back.

Please review and apply,

Amit Shah (5):
  char: Add a QemuChrHandlers struct to initialise chardev handlers
  char: Introduce char_set/remove_fd_handlers()
  char: Add framework for a 'write unblocked' callback
  char: Update send_all() to handle nonblocking chardev write requests
  char: Equip the unix/tcp backend to handle nonblocking writes

 gdbstub.c            |    9 ++-
 hw/debugcon.c        |    2 +-
 hw/escc.c            |    9 ++-
 hw/etraxfs_ser.c     |   13 +++-
 hw/ivshmem.c         |   28 ++++++--
 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 ++-
 net/socket.c         |    4 +-
 qemu-char.c          |  196 ++++++++++++++++++++++++++++++++++++++++----------
 qemu-char.h          |   17 ++++-
 qemu_socket.h        |    2 +-
 21 files changed, 321 insertions(+), 92 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/5] 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/ivshmem.c         |   28 ++++++++++++++++++++++------
 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 ++++++++----
 19 files changed, 173 insertions(+), 60 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 0aa081b..b5ba2ef 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2627,6 +2627,12 @@ static void gdb_sigterm_handler(int signal)
 }
 #endif
 
+static 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;
@@ -2656,8 +2662,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 ba60636..6be7b53 100644
--- a/hw/escc.c
+++ b/hw/escc.c
@@ -894,6 +894,12 @@ void slavio_serial_ms_kbd_init(target_phys_addr_t base, qemu_irq irq,
     sysbus_mmio_map(s, 0, base);
 }
 
+static 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);
@@ -907,8 +913,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 2787ebd..4612c2b 100644
--- a/hw/etraxfs_ser.c
+++ b/hw/etraxfs_ser.c
@@ -190,6 +190,12 @@ static void serial_event(void *opaque, int event)
 
 }
 
+static 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);
@@ -204,10 +210,9 @@ static int etraxfs_ser_init(SysBusDevice *dev)
                                       DEVICE_NATIVE_ENDIAN);
     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/ivshmem.c b/hw/ivshmem.c
index 7b19a81..2a47e98 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -312,6 +312,18 @@ static void fake_irqfd(void *opaque, const uint8_t *buf, int size) {
     msix_notify(pdev, entry->vector);
 }
 
+static QemuChrHandlers ivshmem_msi_handlers = {
+    .fd_can_read = ivshmem_can_receive,
+    .fd_read = fake_irqfd,
+    .fd_event = ivshmem_event,
+};
+
+static QemuChrHandlers ivshmem_nomsi_handlers = {
+    .fd_can_read = ivshmem_can_receive,
+    .fd_read = ivshmem_receive,
+    .fd_event = ivshmem_event,
+};
+
 static CharDriverState* create_eventfd_chr_device(void * opaque, int eventfd,
                                                                     int vector)
 {
@@ -331,11 +343,10 @@ static CharDriverState* create_eventfd_chr_device(void * opaque, int eventfd,
         s->eventfd_table[vector].pdev = &s->dev;
         s->eventfd_table[vector].vector = vector;
 
-        qemu_chr_add_handlers(chr, ivshmem_can_receive, fake_irqfd,
-                      ivshmem_event, &s->eventfd_table[vector]);
+        qemu_chr_add_handlers(chr, &ivshmem_msi_handlers,
+                              &s->eventfd_table[vector]);
     } else {
-        qemu_chr_add_handlers(chr, ivshmem_can_receive, ivshmem_receive,
-                      ivshmem_event, s);
+        qemu_chr_add_handlers(chr, &ivshmem_nomsi_handlers, s);
     }
 
     return chr;
@@ -666,6 +677,12 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }
 
+static QemuChrHandlers ivshmem_handlers = {
+    .fd_can_read = ivshmem_can_receive,
+    .fd_read = ivshmem_read,
+    .fd_event = ivshmem_event,
+};
+
 static int pci_ivshmem_init(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
@@ -754,8 +771,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
         s->eventfd_chr = qemu_mallocz(s->vectors * sizeof(CharDriverState *));
 
-        qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
-                     ivshmem_event, s);
+        qemu_chr_add_handlers(s->server_chr, &ivshmem_handlers, s);
     } else {
         /* just map the file immediately, we're not using a server */
         int fd;
diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
index db57096..f8ff526 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 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 77f0dbf..107601a 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 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;
@@ -304,8 +310,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(&dev->qdev, "pl011_uart", -1, 1, pl011_save, pl011_load, s);
     return 0;
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index ab524a7..952732a 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1995,6 +1995,12 @@ static int pxa2xx_fir_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static 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)
@@ -2013,10 +2019,9 @@ static PXA2xxFIrState *pxa2xx_fir_init(target_phys_addr_t base,
                     pxa2xx_fir_writefn, s, DEVICE_NATIVE_ENDIAN);
     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(NULL, "pxa2xx_fir", 0, 0, pxa2xx_fir_save,
                     pxa2xx_fir_load, s);
 
diff --git a/hw/serial.c b/hw/serial.c
index 2c4af61..5ed4b9f 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -727,6 +727,12 @@ static void serial_reset(void *opaque)
     qemu_irq_lower(s->irq);
 }
 
+static 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) {
@@ -741,8 +747,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 1bdc0a5..69ab6e3 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 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,
@@ -402,9 +408,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 34ce076..1e6b60b 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 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);
@@ -327,8 +333,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 c19580f..697cb89 100644
--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -540,6 +540,12 @@ static void usb_serial_event(void *opaque, int event)
     }
 }
 
+static 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);
@@ -550,8 +556,7 @@ static int usb_serial_initfn(USBDevice *dev)
         return -1;
     }
 
-    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 d0b9354..3bd2b79 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -57,13 +57,18 @@ static void chr_event(void *opaque, int event)
     }
 }
 
+static 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..f490a5a 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 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 9b94e98..9ae84f7 100644
--- a/hw/xilinx_uartlite.c
+++ b/hw/xilinx_uartlite.c
@@ -193,6 +193,12 @@ static void uart_event(void *opaque, int event)
 
 }
 
+static 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);
@@ -206,8 +212,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 f258000..0c16970 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5110,6 +5110,18 @@ static void monitor_event(void *opaque, int event)
  * End:
  */
 
+static QemuChrHandlers monitor_handlers = {
+    .fd_can_read = monitor_can_read,
+    .fd_read = monitor_read,
+    .fd_event = monitor_event,
+};
+
+static 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;
@@ -5131,12 +5143,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..52e9a5c 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 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 edc9ad6..d9a1df3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -190,15 +190,19 @@ void qemu_chr_send_event(CharDriverState *s, int event)
         s->chr_send_event(s, event);
 }
 
+static 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;
+                           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);
@@ -436,6 +440,12 @@ static void mux_chr_event(void *opaque, int event)
         mux_chr_send_event(d, i, event);
 }
 
+static 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;
@@ -450,8 +460,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 e6ee6c4..8ed0ffd 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -72,6 +72,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));
@@ -81,10 +88,7 @@ void qemu_chr_printf(CharDriverState *s, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 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, QemuChrHandlers *handlers,
                            void *opaque);
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
 void qemu_chr_generic_open(CharDriverState *s);
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Amit Shah]

Introduce a char-specific wrapper to qemu_set_fd_handler functions.
This wrapper is useful to add / remove a write handler easily.  Write
handlers are only used when the backend is blocked and cannot receive
any more input.

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

diff --git a/qemu-char.c b/qemu-char.c
index d9a1df3..2420b6b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -214,6 +214,24 @@ void qemu_chr_add_handlers(CharDriverState *s,
     }
 }
 
+static int char_set_fd_handlers(int fd,
+                                IOCanReadHandler *fd_read_poll,
+                                IOHandler *fd_read, IOHandler *fd_write,
+                                void *opaque, bool install_write_handler)
+{
+    if (install_write_handler) {
+        assert(fd_write);
+    } else {
+        fd_write = NULL;
+    }
+    return qemu_set_fd_handler2(fd, fd_read_poll, fd_read, fd_write, opaque);
+}
+
+static int char_remove_fd_handlers(int fd)
+{
+    return qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL);
+}
+
 static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     return len;
@@ -579,7 +597,7 @@ static void fd_chr_read(void *opaque)
     size = read(s->fd_in, buf, len);
     if (size == 0) {
         /* FD has been closed. Remove it from the active list.  */
-        qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL);
+        char_remove_fd_handlers(s->fd_in);
         qemu_chr_event(chr, CHR_EVENT_CLOSED);
         return;
     }
@@ -595,8 +613,8 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
     if (s->fd_in >= 0) {
         if (display_type == DT_NOGRAPHIC && s->fd_in == 0) {
         } else {
-            qemu_set_fd_handler2(s->fd_in, fd_chr_read_poll,
-                                 fd_chr_read, NULL, chr);
+            char_set_fd_handlers(s->fd_in, fd_chr_read_poll, fd_chr_read,
+                                 NULL, chr, false);
         }
     }
 }
@@ -608,7 +626,7 @@ static void fd_chr_close(struct CharDriverState *chr)
     if (s->fd_in >= 0) {
         if (display_type == DT_NOGRAPHIC && s->fd_in == 0) {
         } else {
-            qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL);
+            char_remove_fd_handlers(s->fd_in);
         }
     }
 
@@ -708,7 +726,7 @@ static void stdio_read(void *opaque)
     size = read(0, buf, 1);
     if (size == 0) {
         /* stdin has been closed. Remove it from the active list.  */
-        qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL);
+        char_remove_fd_handlers(0);
         qemu_chr_event(chr, CHR_EVENT_CLOSED);
         return;
     }
@@ -764,7 +782,7 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr)
 {
     term_exit();
     stdio_nb_clients--;
-    qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL);
+    char_remove_fd_handlers(0);
     fd_chr_close(chr);
 }
 
@@ -776,7 +794,7 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
         return NULL;
     chr = qemu_chr_open_fd(0, 1);
     chr->chr_close = qemu_chr_close_stdio;
-    qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr);
+    char_set_fd_handlers(0, stdio_read_poll, stdio_read, NULL, chr, false);
     stdio_nb_clients++;
     term_init(opts);
 
@@ -904,8 +922,8 @@ static void pty_chr_update_read_handler(CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
 
-    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
-                         pty_chr_read, NULL, chr);
+    char_set_fd_handlers(s->fd, pty_chr_read_poll, pty_chr_read, NULL,
+                         chr, false);
     s->polling = 1;
     /*
      * Short timeout here: just need wait long enougth that qemu makes
@@ -923,7 +941,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
     PtyCharDriver *s = chr->opaque;
 
     if (!connected) {
-        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+        char_remove_fd_handlers(s->fd);
         s->connected = 0;
         s->polling = 0;
         /* (re-)connect poll interval for idle guests: once per second.
@@ -959,7 +977,7 @@ static void pty_chr_close(struct CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
 
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    char_remove_fd_handlers(s->fd);
     close(s->fd);
     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
@@ -1860,8 +1878,8 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
     NetCharDriver *s = chr->opaque;
 
     if (s->fd >= 0) {
-        qemu_set_fd_handler2(s->fd, udp_chr_read_poll,
-                             udp_chr_read, NULL, chr);
+        char_set_fd_handlers(s->fd, udp_chr_read_poll, udp_chr_read, NULL,
+                             chr, false);
     }
 }
 
@@ -1869,7 +1887,7 @@ static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        char_remove_fd_handlers(s->fd);
         closesocket(s->fd);
     }
     qemu_free(s);
@@ -2078,9 +2096,10 @@ static void tcp_chr_read(void *opaque)
         /* connection closed */
         s->connected = 0;
         if (s->listen_fd >= 0) {
-            qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
+            char_set_fd_handlers(s->listen_fd, NULL, tcp_chr_accept, NULL,
+                                 chr, false);
         }
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        char_remove_fd_handlers(s->fd);
         closesocket(s->fd);
         s->fd = -1;
         qemu_chr_event(chr, CHR_EVENT_CLOSED);
@@ -2105,8 +2124,8 @@ static void tcp_chr_connect(void *opaque)
     TCPCharDriver *s = chr->opaque;
 
     s->connected = 1;
-    qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
-                         tcp_chr_read, NULL, chr);
+    char_set_fd_handlers(s->fd, tcp_chr_read_poll, tcp_chr_read, NULL,
+                         chr, false);
     qemu_chr_generic_open(chr);
 }
 
@@ -2167,7 +2186,7 @@ static void tcp_chr_accept(void *opaque)
     if (s->do_nodelay)
         socket_set_nodelay(fd);
     s->fd = fd;
-    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
+    char_remove_fd_handlers(s->listen_fd);
     tcp_chr_connect(chr);
 }
 
@@ -2175,11 +2194,11 @@ static void tcp_chr_close(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
-        qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
+        char_remove_fd_handlers(s->fd);
         closesocket(s->fd);
     }
     if (s->listen_fd >= 0) {
-        qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
+        char_remove_fd_handlers(s->listen_fd);
         closesocket(s->listen_fd);
     }
     qemu_free(s);
@@ -2241,7 +2260,8 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 
     if (is_listen) {
         s->listen_fd = fd;
-        qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr);
+        char_set_fd_handlers(s->listen_fd, NULL, tcp_chr_accept, NULL,
+                             chr, false);
         if (is_telnet)
             s->do_telnetopt = 1;
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/5] char: Add framework for a 'write unblocked' callback

[Amit Shah]

The char layer can let users know that the driver will block on further
input.  For users interested in not blocking, they can assign a function
pointer that will be called back when the driver becomes writable.  This
patch just adds the function pointers to the CharDriverState structure,
future patches will enable the nonblocking and callback functionality.

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

diff --git a/qemu-char.c b/qemu-char.c
index 2420b6b..9a132b6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -202,11 +202,14 @@ 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);
 
+    s->write_blocked = false;
+
     /* We're connecting to an already opened device, so let's make sure we
        also get the open event */
     if (s->opened) {
diff --git a/qemu-char.h b/qemu-char.h
index 8ed0ffd..0c2c445 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -60,6 +60,9 @@ struct CharDriverState {
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
+    IOHandler *chr_write_unblocked;
+    void (*update_fd_handlers)(struct CharDriverState *chr,
+                               bool poll_out);
     void *handler_opaque;
     void (*chr_send_event)(struct CharDriverState *chr, int event);
     void (*chr_close)(struct CharDriverState *chr);
@@ -69,6 +72,8 @@ struct CharDriverState {
     char *label;
     char *filename;
     int opened;
+    /* Are we in a blocked state? */
+    bool write_blocked;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 4/5] char: Update send_all() to handle nonblocking chardev write requests

[Amit Shah]

The send_all function is modified to return to the caller in case the
driver cannot handle any more data.  It returns -EAGAIN or
WSAEWOULDBLOCK on non-Windows and Windows platforms respectively.  This
is only done when the caller sets a callback function handler indicating
it's not interested in blocking till the driver has written out all the
data.

Currently there's no driver or caller that supports this.  Future
commits will add such capability.
---
 net/socket.c  |    4 +-
 qemu-char.c   |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 qemu_socket.h |    2 +-
 3 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 3182b37..5dedd78 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(NULL, s->fd, (const uint8_t *)&len, sizeof(len));
+    return send_all(NULL, s->fd, buf, size);
 }
 
 static ssize_t net_socket_receive_dgram(VLANClientState *nc, const uint8_t *buf, size_t size)
diff --git a/qemu-char.c b/qemu-char.c
index 9a132b6..6e02334 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -514,7 +514,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
 
 
 #ifdef _WIN32
-int send_all(int fd, const void *buf, int len1)
+static int do_send(int fd, const void *buf, int len1, bool nonblock)
 {
     int ret, len;
 
@@ -522,9 +522,14 @@ int send_all(int fd, const void *buf, int len1)
     while (len > 0) {
         ret = send(fd, buf, len, 0);
         if (ret < 0) {
+            if (nonblock && len1 - len) {
+                return len1 - len;
+            }
             errno = WSAGetLastError();
             if (errno != WSAEWOULDBLOCK) {
                 return -1;
+            } else if (errno == WSAEWOULDBLOCK && nonblock) {
+                return WSAEWOULDBLOCK;
             }
         } else if (ret == 0) {
             break;
@@ -538,7 +543,7 @@ int send_all(int fd, const void *buf, int len1)
 
 #else
 
-int send_all(int fd, const void *_buf, int len1)
+static int do_send(int fd, const void *_buf, int len1, bool nonblock)
 {
     int ret, len;
     const uint8_t *buf = _buf;
@@ -547,8 +552,15 @@ int send_all(int fd, const void *_buf, int len1)
     while (len > 0) {
         ret = write(fd, buf, len);
         if (ret < 0) {
-            if (errno != EINTR && errno != EAGAIN)
+            if (nonblock && len1 - len) {
+                return len1 - len;
+            }
+            if (errno == EAGAIN && nonblock) {
+                return -EAGAIN;
+            }
+            if (errno != EINTR && errno != EAGAIN) {
                 return -1;
+            }
         } else if (ret == 0) {
             break;
         } else {
@@ -560,6 +572,55 @@ int send_all(int fd, const void *_buf, int len1)
 }
 #endif /* !_WIN32 */
 
+int send_all(CharDriverState *chr, int fd, const void *_buf, int len1)
+{
+    int ret, eagain_errno;
+    bool nonblock;
+
+    if (chr->write_blocked) {
+        /*
+         * We don't handle this situation: the caller should not send
+         * us data while we're blocked.
+         *
+         * We could buffer this data here but that'll only encourage
+         * bad behaviour on part of the callers.
+         *
+         * Also, the data already in fd's buffers isn't easily
+         * migratable.  If we want full migration support, all the
+         * data landing here needs to be buffered and on migration,
+         * anything that's unsent needs to be transferred to the
+         * dest. machine (which again isn't a very good way of solving
+         * the problem, as the src may become writable just during
+         * migration and the reader could receive some data twice,
+         * essentially corrupting the data).
+         */
+        abort();
+    }
+
+    nonblock = false;
+    /*
+     * Ensure the char backend is able to receive and handle the
+     * 'write unblocked' event before we turn on nonblock support.
+     */
+    if (chr && chr->update_fd_handlers && chr->chr_write_unblocked) {
+        nonblock = true;
+    }
+    ret = do_send(fd, _buf, len1, nonblock);
+
+#ifdef _WIN32
+    eagain_errno = WSAEWOULDBLOCK;
+#else
+    eagain_errno = -EAGAIN;
+#endif
+
+    if (nonblock && (ret == eagain_errno || (ret >= 0 && ret < len1))) {
+        /* Update fd handler to wake up when chr becomes writable */
+        chr->update_fd_handlers(chr, true);
+        chr->write_blocked = true;
+    }
+    return ret;
+}
+
 #ifndef _WIN32
 
 typedef struct {
@@ -573,7 +634,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(chr, s->fd_out, buf, len);
 }
 
 static int fd_chr_read_poll(void *opaque)
@@ -885,7 +946,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(chr, s->fd, buf, len);
 }
 
 static int pty_chr_read_poll(void *opaque)
@@ -1950,7 +2011,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(chr, s->fd, buf, len);
     } else {
         /* XXX: indicate an error ? */
         return len;
diff --git a/qemu_socket.h b/qemu_socket.h
index 897a8ae..97dd24a 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -36,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(CharDriverState *chr, int fd, const void *buf, int len1);
 
 /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
 int inet_listen_opts(QemuOpts *opts, int port_offset);
-- 
1.7.3.4

[Qemu-devel] [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes

[Amit Shah]

Now that the infrastructure is in place to return -EAGAIN to callers,
individual char drivers can set their update_fd_handlers() function to
set or remove an fd's write handler.  This handler checks if the driver
became writable.

A generic callback routine is used for unblocking writes and letting
users of chardevs know that a driver became writable again.

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

diff --git a/qemu-char.c b/qemu-char.c
index 6e02334..55d442d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -235,6 +235,19 @@ static int char_remove_fd_handlers(int fd)
     return qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL);
 }
 
+/*
+ * Generic routine that gets called when chardev becomes writable.
+ * Lets chardev user know it's OK to send more data.
+ */
+static void char_write_unblocked(void *opaque)
+{
+    CharDriverState *chr = opaque;
+
+    chr->write_blocked = false;
+    chr->update_fd_handlers(chr, false);
+    chr->chr_write_unblocked(chr->handler_opaque);
+}
+
 static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     return len;
@@ -2269,6 +2282,19 @@ static void tcp_chr_close(CharDriverState *chr)
     qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
+static void tcp_update_fd_handlers(CharDriverState *chr, bool poll_out)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    /*
+     * This function is called only after tcp_chr_connect() is called
+     * (either in 'server' mode or client mode.  So we're sure of
+     * s->fd being initialised.
+     */
+    char_set_fd_handlers(s->fd, tcp_chr_read_poll, tcp_chr_read,
+                         char_write_unblocked, chr, poll_out);
+}
+
 static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 {
     CharDriverState *chr = NULL;
@@ -2321,6 +2347,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
     chr->get_msgfd = tcp_get_msgfd;
+    chr->update_fd_handlers = tcp_update_fd_handlers;
 
     if (is_listen) {
         s->listen_fd = fd;
-- 
1.7.3.4

[Qemu-devel] Re: [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes

[Paolo Bonzini]

On 01/11/2011 12:10 PM, Amit Shah wrote:
> +    char_set_fd_handlers(s->fd, tcp_chr_read_poll, tcp_chr_read,
> +                         char_write_unblocked, chr, poll_out);

Would the 4th parameter always be char_write_unblocked?  If so, what 
about making it hidden within char_set_fd_handlers (also because 
char_write_unblocked is static in qemu-char.c).

Paolo

[Qemu-devel] Re: [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Gerd Hoffmann]

On 01/11/11 12:10, 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.

Nice cleanup.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Gerd Hoffmann]

On 01/11/11 12:10, Amit Shah wrote:
> Introduce a char-specific wrapper to qemu_set_fd_handler functions.
> This wrapper is useful to add / remove a write handler easily.  Write
> handlers are only used when the backend is blocked and cannot receive
> any more input.

I'd suggest to add flags to enable/disable handlers to IOHandlerRecord 
instead.  And helper functions to set/clear them of course.

With that in place you also can move the handlers to a separate struct 
simliar to the new QemuChrHandlers struct from patch #1.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 3/5] char: Add framework for a 'write unblocked' callback

[Gerd Hoffmann]

   Hi,

> @@ -60,6 +60,9 @@ struct CharDriverState {
>       IOEventHandler *chr_event;
>       IOCanReadHandler *chr_can_read;
>       IOReadHandler *chr_read;
> +    IOHandler *chr_write_unblocked;
> +    void (*update_fd_handlers)(struct CharDriverState *chr,
> +                               bool poll_out);

I think you don't need that callback in the first place if you are able 
to just enable/disable the handlers for a file handle.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Amit Shah]

On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote:
> On 01/11/11 12:10, Amit Shah wrote:
> >Introduce a char-specific wrapper to qemu_set_fd_handler functions.
> >This wrapper is useful to add / remove a write handler easily.  Write
> >handlers are only used when the backend is blocked and cannot receive
> >any more input.
> 
> I'd suggest to add flags to enable/disable handlers to
> IOHandlerRecord instead.  And helper functions to set/clear them of
> course.
> 
> With that in place you also can move the handlers to a separate
> struct simliar to the new QemuChrHandlers struct from patch #1.

I'm planning to do that later -- when more backends get involved, which
have multiple fds (one for in, one for out).

Are you OK with this for now (to solve the immediate bugs of guests
freezing if host can't flush data) and doing this cleanup later as we
progress?

		Amit

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Gerd Hoffmann]

On 01/11/11 16:38, Amit Shah wrote:
> On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote:
>> On 01/11/11 12:10, Amit Shah wrote:
>>> Introduce a char-specific wrapper to qemu_set_fd_handler functions.
>>> This wrapper is useful to add / remove a write handler easily.  Write
>>> handlers are only used when the backend is blocked and cannot receive
>>> any more input.
>>
>> I'd suggest to add flags to enable/disable handlers to
>> IOHandlerRecord instead.  And helper functions to set/clear them of
>> course.
>>
>> With that in place you also can move the handlers to a separate
>> struct simliar to the new QemuChrHandlers struct from patch #1.
>
> I'm planning to do that later -- when more backends get involved, which
> have multiple fds (one for in, one for out).

Moving the handlers to a separate struct is clearly a incremental 
cleanup which can follow later.  Using enable/disable flags will 
probably simplify the interfaces for the non-blocking mode and thus 
simplify the whole patch series so I think this should be done now.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Blue Swirl]

On Tue, Jan 11, 2011 at 11:10 AM, Amit Shah <amit.shah@redhat.com> 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/ivshmem.c         |   28 ++++++++++++++++++++++------
>  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 ++++++++----
>  19 files changed, 173 insertions(+), 60 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 0aa081b..b5ba2ef 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2627,6 +2627,12 @@ static void gdb_sigterm_handler(int signal)
>  }
>  #endif
>
> +static QemuChrHandlers gdb_handlers = {
> +    .fd_can_read = gdb_chr_can_receive,
> +    .fd_read = gdb_chr_receive,
> +    .fd_event = gdb_chr_event,
> +};

These structures should be const.

> +
>  int gdbserver_start(const char *device)
>  {
>     GDBState *s;
> @@ -2656,8 +2662,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 ba60636..6be7b53 100644
> --- a/hw/escc.c
> +++ b/hw/escc.c
> @@ -894,6 +894,12 @@ void slavio_serial_ms_kbd_init(target_phys_addr_t base, qemu_irq irq,
>     sysbus_mmio_map(s, 0, base);
>  }
>
> +static 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);
> @@ -907,8 +913,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 2787ebd..4612c2b 100644
> --- a/hw/etraxfs_ser.c
> +++ b/hw/etraxfs_ser.c
> @@ -190,6 +190,12 @@ static void serial_event(void *opaque, int event)
>
>  }
>
> +static 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);
> @@ -204,10 +210,9 @@ static int etraxfs_ser_init(SysBusDevice *dev)
>                                       DEVICE_NATIVE_ENDIAN);
>     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/ivshmem.c b/hw/ivshmem.c
> index 7b19a81..2a47e98 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -312,6 +312,18 @@ static void fake_irqfd(void *opaque, const uint8_t *buf, int size) {
>     msix_notify(pdev, entry->vector);
>  }
>
> +static QemuChrHandlers ivshmem_msi_handlers = {
> +    .fd_can_read = ivshmem_can_receive,
> +    .fd_read = fake_irqfd,
> +    .fd_event = ivshmem_event,
> +};
> +
> +static QemuChrHandlers ivshmem_nomsi_handlers = {
> +    .fd_can_read = ivshmem_can_receive,
> +    .fd_read = ivshmem_receive,
> +    .fd_event = ivshmem_event,
> +};
> +
>  static CharDriverState* create_eventfd_chr_device(void * opaque, int eventfd,
>                                                                     int vector)
>  {
> @@ -331,11 +343,10 @@ static CharDriverState* create_eventfd_chr_device(void * opaque, int eventfd,
>         s->eventfd_table[vector].pdev = &s->dev;
>         s->eventfd_table[vector].vector = vector;
>
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, fake_irqfd,
> -                      ivshmem_event, &s->eventfd_table[vector]);
> +        qemu_chr_add_handlers(chr, &ivshmem_msi_handlers,
> +                              &s->eventfd_table[vector]);
>     } else {
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, ivshmem_receive,
> -                      ivshmem_event, s);
> +        qemu_chr_add_handlers(chr, &ivshmem_nomsi_handlers, s);
>     }
>
>     return chr;
> @@ -666,6 +677,12 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
>     return 0;
>  }
>
> +static QemuChrHandlers ivshmem_handlers = {
> +    .fd_can_read = ivshmem_can_receive,
> +    .fd_read = ivshmem_read,
> +    .fd_event = ivshmem_event,
> +};
> +
>  static int pci_ivshmem_init(PCIDevice *dev)
>  {
>     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
> @@ -754,8 +771,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
>         s->eventfd_chr = qemu_mallocz(s->vectors * sizeof(CharDriverState *));
>
> -        qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
> -                     ivshmem_event, s);
> +        qemu_chr_add_handlers(s->server_chr, &ivshmem_handlers, s);
>     } else {
>         /* just map the file immediately, we're not using a server */
>         int fd;
> diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
> index db57096..f8ff526 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 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 77f0dbf..107601a 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 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;
> @@ -304,8 +310,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(&dev->qdev, "pl011_uart", -1, 1, pl011_save, pl011_load, s);
>     return 0;
> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
> index ab524a7..952732a 100644
> --- a/hw/pxa2xx.c
> +++ b/hw/pxa2xx.c
> @@ -1995,6 +1995,12 @@ static int pxa2xx_fir_load(QEMUFile *f, void *opaque, int version_id)
>     return 0;
>  }
>
> +static 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)
> @@ -2013,10 +2019,9 @@ static PXA2xxFIrState *pxa2xx_fir_init(target_phys_addr_t base,
>                     pxa2xx_fir_writefn, s, DEVICE_NATIVE_ENDIAN);
>     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(NULL, "pxa2xx_fir", 0, 0, pxa2xx_fir_save,
>                     pxa2xx_fir_load, s);
>
> diff --git a/hw/serial.c b/hw/serial.c
> index 2c4af61..5ed4b9f 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -727,6 +727,12 @@ static void serial_reset(void *opaque)
>     qemu_irq_lower(s->irq);
>  }
>
> +static 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) {
> @@ -741,8 +747,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 1bdc0a5..69ab6e3 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 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,
> @@ -402,9 +408,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 34ce076..1e6b60b 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 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);
> @@ -327,8 +333,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 c19580f..697cb89 100644
> --- a/hw/usb-serial.c
> +++ b/hw/usb-serial.c
> @@ -540,6 +540,12 @@ static void usb_serial_event(void *opaque, int event)
>     }
>  }
>
> +static 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);
> @@ -550,8 +556,7 @@ static int usb_serial_initfn(USBDevice *dev)
>         return -1;
>     }
>
> -    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 d0b9354..3bd2b79 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -57,13 +57,18 @@ static void chr_event(void *opaque, int event)
>     }
>  }
>
> +static 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..f490a5a 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 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 9b94e98..9ae84f7 100644
> --- a/hw/xilinx_uartlite.c
> +++ b/hw/xilinx_uartlite.c
> @@ -193,6 +193,12 @@ static void uart_event(void *opaque, int event)
>
>  }
>
> +static 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);
> @@ -206,8 +212,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 f258000..0c16970 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -5110,6 +5110,18 @@ static void monitor_event(void *opaque, int event)
>  * End:
>  */
>
> +static QemuChrHandlers monitor_handlers = {
> +    .fd_can_read = monitor_can_read,
> +    .fd_read = monitor_read,
> +    .fd_event = monitor_event,
> +};
> +
> +static 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;
> @@ -5131,12 +5143,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..52e9a5c 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 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 edc9ad6..d9a1df3 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -190,15 +190,19 @@ void qemu_chr_send_event(CharDriverState *s, int event)
>         s->chr_send_event(s, event);
>  }
>
> +static 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;
> +                           QemuChrHandlers *handlers, void *opaque)
> +{

Here we could also check if (!s) and return if so. This would simplify
the callers a bit.

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Amit Shah]

On (Tue) Jan 11 2011 [16:54:48], Gerd Hoffmann wrote:
> On 01/11/11 16:38, Amit Shah wrote:
> >On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote:
> >>On 01/11/11 12:10, Amit Shah wrote:
> >>>Introduce a char-specific wrapper to qemu_set_fd_handler functions.
> >>>This wrapper is useful to add / remove a write handler easily.  Write
> >>>handlers are only used when the backend is blocked and cannot receive
> >>>any more input.
> >>
> >>I'd suggest to add flags to enable/disable handlers to
> >>IOHandlerRecord instead.  And helper functions to set/clear them of
> >>course.
> >>
> >>With that in place you also can move the handlers to a separate
> >>struct simliar to the new QemuChrHandlers struct from patch #1.
> >
> >I'm planning to do that later -- when more backends get involved, which
> >have multiple fds (one for in, one for out).
> 
> Moving the handlers to a separate struct is clearly a incremental
> cleanup which can follow later.  Using enable/disable flags will
> probably simplify the interfaces for the non-blocking mode and thus
> simplify the whole patch series so I think this should be done now.

Agree -- but it looks to be a big patch.  I have some initial work done,
and hence am not converting anything other than unix/tcp backends.  The
proposed interface here is local to this file, and just a couple of
callers.  No big deal to change it once this is in.

(The struct for that will look like:

struct fd_handler {
	int fd;

	IOHandler *read;
	IOHandler *write;
	IOCanReadHandler *read_poll;

	bool read_enabled, write_enabled, read_poll_enabled;

	void (*set_read_handler)(IOHandler *read_handler);
	void (*set_write_handler)(IOHandler *write_handler);
	void (*set_readpoll_handler)(IOCanReadHandler *read_poll_handler);
}

This has to be embedded in the CharDriverState for each fd for each
backend.

Also: we also want to be able to select() on all fds so that we can
detect disconnection events as they happen.  So we also need an array
somewhere.)

		Amit

Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Amit Shah]

On (Tue) Jan 11 2011 [17:13:15], Blue Swirl wrote:
> > +static QemuChrHandlers gdb_handlers = {
> > +    .fd_can_read = gdb_chr_can_receive,
> > +    .fd_read = gdb_chr_receive,
> > +    .fd_event = gdb_chr_event,
> > +};
> 
> These structures should be const.

Hm, I had that but looks like it got lost in some rebase.  Added again.

> > @@ -190,15 +190,19 @@ void qemu_chr_send_event(CharDriverState *s, int event)
> >         s->chr_send_event(s, event);
> >  }
> >
> > +static 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;
> > +                           QemuChrHandlers *handlers, void *opaque)
> > +{
> 
> Here we could also check if (!s) and return if so. This would simplify
> the callers a bit.

Simplified in what way?

		Amit

[Qemu-devel] Re: [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes

[Amit Shah]

On (Tue) Jan 11 2011 [14:38:00], Paolo Bonzini wrote:
> On 01/11/2011 12:10 PM, Amit Shah wrote:
> >+    char_set_fd_handlers(s->fd, tcp_chr_read_poll, tcp_chr_read,
> >+                         char_write_unblocked, chr, poll_out);
> 
> Would the 4th parameter always be char_write_unblocked?  If so, what
> about making it hidden within char_set_fd_handlers (also because
> char_write_unblocked is static in qemu-char.c).

It could change for other backends, but the plan is to move to a
struct-type declaration even for the fd handlers, like discussed in the
other thread with Gerd.

		Amit

[Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers()

[Gerd Hoffmann]

   Hi,

>> Moving the handlers to a separate struct is clearly a incremental
>> cleanup which can follow later.  Using enable/disable flags will
>> probably simplify the interfaces for the non-blocking mode and thus
>> simplify the whole patch series so I think this should be done now.
>
> Agree -- but it looks to be a big patch.  I have some initial work done,
> and hence am not converting anything other than unix/tcp backends.  The
> proposed interface here is local to this file, and just a couple of
> callers.  No big deal to change it once this is in.
>
> (The struct for that will look like:
>
> struct fd_handler {
> 	int fd;
>
> 	IOHandler *read;
> 	IOHandler *write;
> 	IOCanReadHandler *read_poll;
>
> 	bool read_enabled, write_enabled, read_poll_enabled;
>
> 	void (*set_read_handler)(IOHandler *read_handler);
> 	void (*set_write_handler)(IOHandler *write_handler);
> 	void (*set_readpoll_handler)(IOCanReadHandler *read_poll_handler);
> }

No, that isn't what I have in mind ...

I'm thinking more about this:

   (1) IOHandlerRecord gets variables to enable/disable the handlers
       (a bunch of bools, a bitmask, whatever).  They default to enabled
       to maintain backward compatibility.
   (2) One or more functions are added to enable/disable the handlers,
       something like qemu_fd_check_read(int fd, bool enable);
   (3) main_loop_wait() will check whenever the handler is actually
       enabled before adding it to the file handle set for the select()
       call.

This shouldn't be that big.  And with this initial stuff in place you 
can go forward with the non-blocking stuff.  No need to pass around the 
write handler or have callbacks just to enable/disable the checking for 
a writable fd, the non-blocking code can just call 
qemu_fd_check_write(fd, true/false) to do it.

Additional cleanups possible:

   (1) switch everybody who registers/unregisters handlers to the new
       enable/disable interface.
   (2) move the function pointers from IOHandlerRecord to a separate
       struct (say IOHandlerOps).

> Also: we also want to be able to select() on all fds so that we can
> detect disconnection events as they happen.  So we also need an array
> somewhere.)

I think you can't do that without switching from select() to poll().

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Michael Roth]

On 01/12/2011 12:07 AM, Amit Shah wrote:
> On (Tue) Jan 11 2011 [17:13:15], Blue Swirl wrote:
>>> +static QemuChrHandlers gdb_handlers = {
>>> +    .fd_can_read = gdb_chr_can_receive,
>>> +    .fd_read = gdb_chr_receive,
>>> +    .fd_event = gdb_chr_event,
>>> +};
>>
>> These structures should be const.
>
> Hm, I had that but looks like it got lost in some rebase.  Added again.
>
>>> @@ -190,15 +190,19 @@ void qemu_chr_send_event(CharDriverState *s, int event)
>>>          s->chr_send_event(s, event);
>>>   }
>>>
>>> +static 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;
>>> +                           QemuChrHandlers *handlers, void *opaque)
>>> +{
>>
>> Here we could also check if (!s) and return if so. This would simplify
>> the callers a bit.
>
> Simplified in what way?

I assume for reducing the need to have to check s->chr != NULL everytime 
beforehand. It's safer and would save a lot on repetitive code as well.

>
> 		Amit
>

Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Blue Swirl]

On Wed, Jan 12, 2011 at 6:01 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 01/12/2011 12:07 AM, Amit Shah wrote:
>>
>> On (Tue) Jan 11 2011 [17:13:15], Blue Swirl wrote:
>>>>
>>>> +static QemuChrHandlers gdb_handlers = {
>>>> +    .fd_can_read = gdb_chr_can_receive,
>>>> +    .fd_read = gdb_chr_receive,
>>>> +    .fd_event = gdb_chr_event,
>>>> +};
>>>
>>> These structures should be const.
>>
>> Hm, I had that but looks like it got lost in some rebase.  Added again.
>>
>>>> @@ -190,15 +190,19 @@ void qemu_chr_send_event(CharDriverState *s, int
>>>> event)
>>>>         s->chr_send_event(s, event);
>>>>  }
>>>>
>>>> +static 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;
>>>> +                           QemuChrHandlers *handlers, void *opaque)
>>>> +{
>>>
>>> Here we could also check if (!s) and return if so. This would simplify
>>> the callers a bit.
>>
>> Simplified in what way?
>
> I assume for reducing the need to have to check s->chr != NULL everytime
> beforehand. It's safer and would save a lot on repetitive code as well.

Yes, that's what I meant.

Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Amit Shah]

On (Wed) Jan 12 2011 [19:03:58], Blue Swirl wrote:
> >>>> +static 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;
> >>>> +                           QemuChrHandlers *handlers, void *opaque)
> >>>> +{
> >>>
> >>> Here we could also check if (!s) and return if so. This would simplify
> >>> the callers a bit.
> >>
> >> Simplified in what way?
> >
> > I assume for reducing the need to have to check s->chr != NULL everytime
> > beforehand. It's safer and would save a lot on repetitive code as well.
> 
> Yes, that's what I meant.

OK, can be added, but I'm wondering if it makes sense (i.e., if an
assert would be better than return).

		Amit

Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers

[Blue Swirl]

On Thu, Jan 13, 2011 at 6:14 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) Jan 12 2011 [19:03:58], Blue Swirl wrote:
>> >>>> +static 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;
>> >>>> +                           QemuChrHandlers *handlers, void *opaque)
>> >>>> +{
>> >>>
>> >>> Here we could also check if (!s) and return if so. This would simplify
>> >>> the callers a bit.
>> >>
>> >> Simplified in what way?
>> >
>> > I assume for reducing the need to have to check s->chr != NULL everytime
>> > beforehand. It's safer and would save a lot on repetitive code as well.
>>
>> Yes, that's what I meant.
>
> OK, can be added, but I'm wondering if it makes sense (i.e., if an
> assert would be better than return).

No, the idea is that the caller does not have to do the current NULL
checks anymore and can then possibly pass a NULL CharDriverState. This
is of course invalid, but to make things easier for the caller, such
calls are just to be ignored.