* [Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes
@ 2011-01-11 11:10 Amit Shah
2011-01-11 11:10 ` [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers Amit Shah
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Amit Shah @ 2011-01-11 11:10 UTC (permalink / raw)
To: qemu list; +Cc: Amit Shah, Paul Brook, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-11 11:10 [Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes Amit Shah @ 2011-01-11 11:10 ` Amit Shah 2011-01-11 14:17 ` [Qemu-devel] " Gerd Hoffmann 2011-01-11 17:13 ` [Qemu-devel] " Blue Swirl 2011-01-11 11:10 ` [Qemu-devel] [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() Amit Shah ` (3 subsequent siblings) 4 siblings, 2 replies; 21+ messages in thread From: Amit Shah @ 2011-01-11 11:10 UTC (permalink / raw) To: qemu list; +Cc: Amit Shah, Paul Brook, Gerd Hoffmann 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-11 11:10 ` [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers Amit Shah @ 2011-01-11 14:17 ` Gerd Hoffmann 2011-01-11 17:13 ` [Qemu-devel] " Blue Swirl 1 sibling, 0 replies; 21+ messages in thread From: Gerd Hoffmann @ 2011-01-11 14:17 UTC (permalink / raw) To: Amit Shah; +Cc: qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-11 11:10 ` [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers Amit Shah 2011-01-11 14:17 ` [Qemu-devel] " Gerd Hoffmann @ 2011-01-11 17:13 ` Blue Swirl 2011-01-12 6:07 ` Amit Shah 1 sibling, 1 reply; 21+ messages in thread From: Blue Swirl @ 2011-01-11 17:13 UTC (permalink / raw) To: Amit Shah; +Cc: Gerd Hoffmann, qemu list, Paul Brook 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-11 17:13 ` [Qemu-devel] " Blue Swirl @ 2011-01-12 6:07 ` Amit Shah 2011-01-12 18:01 ` Michael Roth 0 siblings, 1 reply; 21+ messages in thread From: Amit Shah @ 2011-01-12 6:07 UTC (permalink / raw) To: Blue Swirl; +Cc: Gerd Hoffmann, qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-12 6:07 ` Amit Shah @ 2011-01-12 18:01 ` Michael Roth 2011-01-12 19:03 ` Blue Swirl 0 siblings, 1 reply; 21+ messages in thread From: Michael Roth @ 2011-01-12 18:01 UTC (permalink / raw) To: Amit Shah; +Cc: Blue Swirl, Paul Brook, Gerd Hoffmann, qemu list 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 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-12 18:01 ` Michael Roth @ 2011-01-12 19:03 ` Blue Swirl 2011-01-13 6:14 ` Amit Shah 0 siblings, 1 reply; 21+ messages in thread From: Blue Swirl @ 2011-01-12 19:03 UTC (permalink / raw) To: Michael Roth; +Cc: Amit Shah, Paul Brook, Gerd Hoffmann, qemu list 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-12 19:03 ` Blue Swirl @ 2011-01-13 6:14 ` Amit Shah 2011-01-13 21:29 ` Blue Swirl 0 siblings, 1 reply; 21+ messages in thread From: Amit Shah @ 2011-01-13 6:14 UTC (permalink / raw) To: Blue Swirl; +Cc: qemu list, Paul Brook, Michael Roth, Gerd Hoffmann 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers 2011-01-13 6:14 ` Amit Shah @ 2011-01-13 21:29 ` Blue Swirl 0 siblings, 0 replies; 21+ messages in thread From: Blue Swirl @ 2011-01-13 21:29 UTC (permalink / raw) To: Amit Shah; +Cc: qemu list, Paul Brook, Michael Roth, Gerd Hoffmann 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() 2011-01-11 11:10 [Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes Amit Shah 2011-01-11 11:10 ` [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers Amit Shah @ 2011-01-11 11:10 ` Amit Shah 2011-01-11 14:39 ` [Qemu-devel] " Gerd Hoffmann 2011-01-11 11:10 ` [Qemu-devel] [PATCH 3/5] char: Add framework for a 'write unblocked' callback Amit Shah ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Amit Shah @ 2011-01-11 11:10 UTC (permalink / raw) To: qemu list; +Cc: Amit Shah, Paul Brook, Gerd Hoffmann 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() 2011-01-11 11:10 ` [Qemu-devel] [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() Amit Shah @ 2011-01-11 14:39 ` Gerd Hoffmann 2011-01-11 15:38 ` Amit Shah 0 siblings, 1 reply; 21+ messages in thread From: Gerd Hoffmann @ 2011-01-11 14:39 UTC (permalink / raw) To: Amit Shah; +Cc: qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() 2011-01-11 14:39 ` [Qemu-devel] " Gerd Hoffmann @ 2011-01-11 15:38 ` Amit Shah 2011-01-11 15:54 ` Gerd Hoffmann 0 siblings, 1 reply; 21+ messages in thread From: Amit Shah @ 2011-01-11 15:38 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() 2011-01-11 15:38 ` Amit Shah @ 2011-01-11 15:54 ` Gerd Hoffmann 2011-01-11 17:23 ` Amit Shah 0 siblings, 1 reply; 21+ messages in thread From: Gerd Hoffmann @ 2011-01-11 15:54 UTC (permalink / raw) To: Amit Shah; +Cc: qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() 2011-01-11 15:54 ` Gerd Hoffmann @ 2011-01-11 17:23 ` Amit Shah 2011-01-12 9:11 ` Gerd Hoffmann 0 siblings, 1 reply; 21+ messages in thread From: Amit Shah @ 2011-01-11 17:23 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() 2011-01-11 17:23 ` Amit Shah @ 2011-01-12 9:11 ` Gerd Hoffmann 0 siblings, 0 replies; 21+ messages in thread From: Gerd Hoffmann @ 2011-01-12 9:11 UTC (permalink / raw) To: Amit Shah; +Cc: qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/5] char: Add framework for a 'write unblocked' callback 2011-01-11 11:10 [Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes Amit Shah 2011-01-11 11:10 ` [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers Amit Shah 2011-01-11 11:10 ` [Qemu-devel] [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() Amit Shah @ 2011-01-11 11:10 ` Amit Shah 2011-01-11 14:43 ` [Qemu-devel] " Gerd Hoffmann 2011-01-11 11:10 ` [Qemu-devel] [PATCH 4/5] char: Update send_all() to handle nonblocking chardev write requests Amit Shah 2011-01-11 11:10 ` [Qemu-devel] [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes Amit Shah 4 siblings, 1 reply; 21+ messages in thread From: Amit Shah @ 2011-01-11 11:10 UTC (permalink / raw) To: qemu list; +Cc: Amit Shah, Paul Brook, Gerd Hoffmann 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 3/5] char: Add framework for a 'write unblocked' callback 2011-01-11 11:10 ` [Qemu-devel] [PATCH 3/5] char: Add framework for a 'write unblocked' callback Amit Shah @ 2011-01-11 14:43 ` Gerd Hoffmann 0 siblings, 0 replies; 21+ messages in thread From: Gerd Hoffmann @ 2011-01-11 14:43 UTC (permalink / raw) To: Amit Shah; +Cc: qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 4/5] char: Update send_all() to handle nonblocking chardev write requests 2011-01-11 11:10 [Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes Amit Shah ` (2 preceding siblings ...) 2011-01-11 11:10 ` [Qemu-devel] [PATCH 3/5] char: Add framework for a 'write unblocked' callback Amit Shah @ 2011-01-11 11:10 ` Amit Shah 2011-01-11 11:10 ` [Qemu-devel] [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes Amit Shah 4 siblings, 0 replies; 21+ messages in thread From: Amit Shah @ 2011-01-11 11:10 UTC (permalink / raw) To: qemu list; +Cc: Amit Shah, Paul Brook, Gerd Hoffmann 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes 2011-01-11 11:10 [Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes Amit Shah ` (3 preceding siblings ...) 2011-01-11 11:10 ` [Qemu-devel] [PATCH 4/5] char: Update send_all() to handle nonblocking chardev write requests Amit Shah @ 2011-01-11 11:10 ` Amit Shah 2011-01-11 13:38 ` [Qemu-devel] " Paolo Bonzini 4 siblings, 1 reply; 21+ messages in thread From: Amit Shah @ 2011-01-11 11:10 UTC (permalink / raw) To: qemu list; +Cc: Amit Shah, Paul Brook, Gerd Hoffmann 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes 2011-01-11 11:10 ` [Qemu-devel] [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes Amit Shah @ 2011-01-11 13:38 ` Paolo Bonzini 2011-01-12 6:16 ` Amit Shah 0 siblings, 1 reply; 21+ messages in thread From: Paolo Bonzini @ 2011-01-11 13:38 UTC (permalink / raw) To: Amit Shah; +Cc: Gerd Hoffmann, qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] Re: [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes 2011-01-11 13:38 ` [Qemu-devel] " Paolo Bonzini @ 2011-01-12 6:16 ` Amit Shah 0 siblings, 0 replies; 21+ messages in thread From: Amit Shah @ 2011-01-12 6:16 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Gerd Hoffmann, qemu list, Paul Brook 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2011-01-13 21:29 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-11 11:10 [Qemu-devel] [PATCH v9 0/5] char: Add support for nonblocking writes Amit Shah 2011-01-11 11:10 ` [Qemu-devel] [PATCH 1/5] char: Add a QemuChrHandlers struct to initialise chardev handlers Amit Shah 2011-01-11 14:17 ` [Qemu-devel] " Gerd Hoffmann 2011-01-11 17:13 ` [Qemu-devel] " Blue Swirl 2011-01-12 6:07 ` Amit Shah 2011-01-12 18:01 ` Michael Roth 2011-01-12 19:03 ` Blue Swirl 2011-01-13 6:14 ` Amit Shah 2011-01-13 21:29 ` Blue Swirl 2011-01-11 11:10 ` [Qemu-devel] [PATCH 2/5] char: Introduce char_set/remove_fd_handlers() Amit Shah 2011-01-11 14:39 ` [Qemu-devel] " Gerd Hoffmann 2011-01-11 15:38 ` Amit Shah 2011-01-11 15:54 ` Gerd Hoffmann 2011-01-11 17:23 ` Amit Shah 2011-01-12 9:11 ` Gerd Hoffmann 2011-01-11 11:10 ` [Qemu-devel] [PATCH 3/5] char: Add framework for a 'write unblocked' callback Amit Shah 2011-01-11 14:43 ` [Qemu-devel] " Gerd Hoffmann 2011-01-11 11:10 ` [Qemu-devel] [PATCH 4/5] char: Update send_all() to handle nonblocking chardev write requests Amit Shah 2011-01-11 11:10 ` [Qemu-devel] [PATCH 5/5] char: Equip the unix/tcp backend to handle nonblocking writes Amit Shah 2011-01-11 13:38 ` [Qemu-devel] " Paolo Bonzini 2011-01-12 6:16 ` Amit Shah
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.