[Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Anthony Liguori]

The char layer has been growing some nasty warts for some time now as we ask it
to do things it was never intended on doing.  It's been long over due for an
overhaul and its become evident to me that we need to address this first before
adding any more features to the char layer.

This series is the start at sanitizing the char layer.  It effectively turns
the char layer into an internal pipe.  It supports flow control using an
intermediate ring queue for each direction.

This series is an RFC because I don't think we should merge the series until we
completely convert the old style flow control users to the new style.

One particularly nasty area is the mux device.  I'm not entirely sure yet how
to preceed there.

[Qemu-devel] [PATCH 01/12] char: rename qemu_chr_write() to qemu_chr_fe_write()

[Anthony Liguori]

The char layer is confusing.  There is a front-end, typically a device, that
can send and receive data.  The front-end sends data by calling
qemu_chr_write().

The back-end, typically created via -chardev, can also send and receive data.
Oddly, it sends data by calling qemu_chr_read().

Let's be explicit about which function is for which party.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 gdbstub.c               |    2 +-
 hw/ccid-card-passthru.c |    4 ++--
 hw/debugcon.c           |    2 +-
 hw/escc.c               |    2 +-
 hw/etraxfs_ser.c        |    2 +-
 hw/grlib_apbuart.c      |    2 +-
 hw/lm32_juart.c         |    2 +-
 hw/lm32_uart.c          |    2 +-
 hw/mcf_uart.c           |    2 +-
 hw/milkymist-uart.c     |    2 +-
 hw/omap2.c              |    6 +++---
 hw/parallel.c           |    2 +-
 hw/pl011.c              |    2 +-
 hw/pxa2xx.c             |    2 +-
 hw/serial.c             |    2 +-
 hw/sh_serial.c          |    2 +-
 hw/spapr_vty.c          |    4 ++--
 hw/strongarm.c          |    2 +-
 hw/syborg_serial.c      |    4 ++--
 hw/usb-serial.c         |    2 +-
 hw/virtio-console.c     |    2 +-
 hw/xen_console.c        |    2 +-
 hw/xilinx_uartlite.c    |    2 +-
 monitor.c               |    2 +-
 qemu-char.c             |    4 ++--
 qemu-char.h             |    3 ++-
 slirp/slirp.c           |    2 +-
 usb-redir.c             |    2 +-
 28 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 27b0cfa..d6c362e 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -382,7 +382,7 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int len)
         }
     }
 #else
-    qemu_chr_write(s->chr, buf, len);
+    qemu_chr_fe_write(s->chr, buf, len);
 #endif
 }
 
diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index 28eb9d1..082fd82 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -72,8 +72,8 @@ static void ccid_card_vscard_send_msg(PassthruState *s,
     scr_msg_header.type = htonl(type);
     scr_msg_header.reader_id = htonl(reader_id);
     scr_msg_header.length = htonl(length);
-    qemu_chr_write(s->cs, (uint8_t *)&scr_msg_header, sizeof(VSCMsgHeader));
-    qemu_chr_write(s->cs, payload, length);
+    qemu_chr_fe_write(s->cs, (uint8_t *)&scr_msg_header, sizeof(VSCMsgHeader));
+    qemu_chr_fe_write(s->cs, payload, length);
 }
 
 static void ccid_card_vscard_send_apdu(PassthruState *s,
diff --git a/hw/debugcon.c b/hw/debugcon.c
index 5ee6821..c9ee6d9 100644
--- a/hw/debugcon.c
+++ b/hw/debugcon.c
@@ -51,7 +51,7 @@ static void debugcon_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     printf("debugcon: write addr=0x%04x val=0x%02x\n", addr, val);
 #endif
 
-    qemu_chr_write(s->chr, &ch, 1);
+    qemu_chr_fe_write(s->chr, &ch, 1);
 }
 
 
diff --git a/hw/escc.c b/hw/escc.c
index f6fd919..c1460b7 100644
--- a/hw/escc.c
+++ b/hw/escc.c
@@ -578,7 +578,7 @@ static void escc_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
         s->tx = val;
         if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
             if (s->chr)
-                qemu_chr_write(s->chr, &s->tx, 1);
+                qemu_chr_fe_write(s->chr, &s->tx, 1);
             else if (s->type == kbd && !s->disabled) {
                 handle_kbd_command(s, val);
             }
diff --git a/hw/etraxfs_ser.c b/hw/etraxfs_ser.c
index b917d4d..35f8325 100644
--- a/hw/etraxfs_ser.c
+++ b/hw/etraxfs_ser.c
@@ -118,7 +118,7 @@ ser_writel (void *opaque, target_phys_addr_t addr, uint32_t value)
     switch (addr)
     {
         case RW_DOUT:
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
             s->regs[R_INTR] |= 3;
             s->pending_tx = 1;
             s->regs[addr] = value;
diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c
index 169a56e..c90b810 100644
--- a/hw/grlib_apbuart.c
+++ b/hw/grlib_apbuart.c
@@ -114,7 +114,7 @@ grlib_apbuart_writel(void *opaque, target_phys_addr_t addr, uint32_t value)
     switch (addr) {
     case DATA_OFFSET:
         c = value & 0xFF;
-        qemu_chr_write(uart->chr, &c, 1);
+        qemu_chr_fe_write(uart->chr, &c, 1);
         return;
 
     case STATUS_OFFSET:
diff --git a/hw/lm32_juart.c b/hw/lm32_juart.c
index fddcf7e..5454aa4 100644
--- a/hw/lm32_juart.c
+++ b/hw/lm32_juart.c
@@ -72,7 +72,7 @@ void lm32_juart_set_jtx(DeviceState *d, uint32_t jtx)
 
     s->jtx = jtx;
     if (s->chr) {
-        qemu_chr_write(s->chr, &ch, 1);
+        qemu_chr_fe_write(s->chr, &ch, 1);
     }
 }
 
diff --git a/hw/lm32_uart.c b/hw/lm32_uart.c
index 09090e9..3678545 100644
--- a/hw/lm32_uart.c
+++ b/hw/lm32_uart.c
@@ -169,7 +169,7 @@ static void uart_write(void *opaque, target_phys_addr_t addr, uint32_t value)
     switch (addr) {
     case R_RXTX:
         if (s->chr) {
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
         }
         break;
     case R_IER:
diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
index 905e116..747bc36 100644
--- a/hw/mcf_uart.c
+++ b/hw/mcf_uart.c
@@ -110,7 +110,7 @@ static void mcf_uart_do_tx(mcf_uart_state *s)
 {
     if (s->tx_enabled && (s->sr & MCF_UART_TxEMP) == 0) {
         if (s->chr)
-            qemu_chr_write(s->chr, (unsigned char *)&s->tb, 1);
+            qemu_chr_fe_write(s->chr, (unsigned char *)&s->tb, 1);
         s->sr |= MCF_UART_TxEMP;
     }
     if (s->tx_enabled) {
diff --git a/hw/milkymist-uart.c b/hw/milkymist-uart.c
index 56c90da..e8e309d 100644
--- a/hw/milkymist-uart.c
+++ b/hw/milkymist-uart.c
@@ -77,7 +77,7 @@ static void uart_write(void *opaque, target_phys_addr_t addr, uint32_t value)
     switch (addr) {
     case R_RXTX:
         if (s->chr) {
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
         }
         trace_milkymist_uart_pulse_irq_tx();
         qemu_irq_pulse(s->tx_irq);
diff --git a/hw/omap2.c b/hw/omap2.c
index c9b3540..919318f 100644
--- a/hw/omap2.c
+++ b/hw/omap2.c
@@ -748,14 +748,14 @@ static void omap_sti_fifo_write(void *opaque, target_phys_addr_t addr,
 
     if (ch == STI_TRACE_CONTROL_CHANNEL) {
         /* Flush channel <i>value</i>.  */
-        qemu_chr_write(s->chr, (const uint8_t *) "\r", 1);
+        qemu_chr_fe_write(s->chr, (const uint8_t *) "\r", 1);
     } else if (ch == STI_TRACE_CONSOLE_CHANNEL || 1) {
         if (value == 0xc0 || value == 0xc3) {
             /* Open channel <i>ch</i>.  */
         } else if (value == 0x00)
-            qemu_chr_write(s->chr, (const uint8_t *) "\n", 1);
+            qemu_chr_fe_write(s->chr, (const uint8_t *) "\n", 1);
         else
-            qemu_chr_write(s->chr, &byte, 1);
+            qemu_chr_fe_write(s->chr, &byte, 1);
     }
 }
 
diff --git a/hw/parallel.c b/hw/parallel.c
index cc853a5..f3e8219 100644
--- a/hw/parallel.c
+++ b/hw/parallel.c
@@ -120,7 +120,7 @@ parallel_ioport_write_sw(void *opaque, uint32_t addr, uint32_t val)
             if (val & PARA_CTR_STROBE) {
                 s->status &= ~PARA_STS_BUSY;
                 if ((s->control & PARA_CTR_STROBE) == 0)
-                    qemu_chr_write(s->chr, &s->dataw, 1);
+                    qemu_chr_fe_write(s->chr, &s->dataw, 1);
             } else {
                 if (s->control & PARA_CTR_INTEN) {
                     s->irq_pending = 1;
diff --git a/hw/pl011.c b/hw/pl011.c
index 997ce84..707a161 100644
--- a/hw/pl011.c
+++ b/hw/pl011.c
@@ -133,7 +133,7 @@ static void pl011_write(void *opaque, target_phys_addr_t offset,
         /* ??? Check if transmitter is enabled.  */
         ch = value;
         if (s->chr)
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
         s->int_level |= PL011_INT_TX;
         pl011_update(s);
         break;
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index cf93110..7516454 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1923,7 +1923,7 @@ static void pxa2xx_fir_write(void *opaque, target_phys_addr_t addr,
         else
             ch = ~value;
         if (s->chr && s->enable && (s->control[0] & (1 << 3)))	/* TXE */
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
         break;
     case ICSR0:
         s->status[0] &= ~(value & 0x66);
diff --git a/hw/serial.c b/hw/serial.c
index 0ee61dd..dc6d1f8 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -334,7 +334,7 @@ static void serial_xmit(void *opaque)
     if (s->mcr & UART_MCR_LOOP) {
         /* in loopback mode, say that we just received a char */
         serial_receive1(s, &s->tsr, 1);
-    } else if (qemu_chr_write(s->chr, &s->tsr, 1) != 1) {
+    } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
         if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
             s->tsr_retry++;
             qemu_mod_timer(s->transmit_timer,  new_xmit_ts + s->char_transmit_time);
diff --git a/hw/sh_serial.c b/hw/sh_serial.c
index 191f4a6..eabd726 100644
--- a/hw/sh_serial.c
+++ b/hw/sh_serial.c
@@ -105,7 +105,7 @@ static void sh_serial_write(void *opaque, uint32_t offs, uint32_t val)
     case 0x0c: /* FTDR / TDR */
         if (s->chr) {
             ch = val;
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
 	}
 	s->dr = val;
 	s->flags &= ~SH_SERIAL_FLAG_TDE;
diff --git a/hw/spapr_vty.c b/hw/spapr_vty.c
index 6fc0105..f5046d9 100644
--- a/hw/spapr_vty.c
+++ b/hw/spapr_vty.c
@@ -50,8 +50,8 @@ void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len)
 {
     VIOsPAPRVTYDevice *dev = (VIOsPAPRVTYDevice *)sdev;
 
-    /* FIXME: should check the qemu_chr_write() return value */
-    qemu_chr_write(dev->chardev, buf, len);
+    /* FIXME: should check the qemu_chr_fe_write() return value */
+    qemu_chr_fe_write(dev->chardev, buf, len);
 }
 
 static int spapr_vty_init(VIOsPAPRDevice *sdev)
diff --git a/hw/strongarm.c b/hw/strongarm.c
index 0e03d61..50fe0c0 100644
--- a/hw/strongarm.c
+++ b/hw/strongarm.c
@@ -1067,7 +1067,7 @@ static void strongarm_uart_tx(void *opaque)
     if (s->utcr3 & UTCR3_LBM) /* loopback */ {
         strongarm_uart_receive(s, &s->tx_fifo[s->tx_start], 1);
     } else if (s->chr) {
-        qemu_chr_write(s->chr, &s->tx_fifo[s->tx_start], 1);
+        qemu_chr_fe_write(s->chr, &s->tx_fifo[s->tx_start], 1);
     }
 
     s->tx_start = (s->tx_start + 1) % 8;
diff --git a/hw/syborg_serial.c b/hw/syborg_serial.c
index 2ef7175..25c6c9d 100644
--- a/hw/syborg_serial.c
+++ b/hw/syborg_serial.c
@@ -119,7 +119,7 @@ static void do_dma_tx(SyborgSerialState *s, uint32_t count)
         /* optimize later. Now, 1 byte per iteration */
         while (count--) {
             cpu_physical_memory_read(s->dma_tx_ptr, &ch, 1);
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
             s->dma_tx_ptr++;
         }
     } else {
@@ -203,7 +203,7 @@ static void syborg_serial_write(void *opaque, target_phys_addr_t offset,
     case SERIAL_DATA:
         ch = value;
         if (s->chr)
-            qemu_chr_write(s->chr, &ch, 1);
+            qemu_chr_fe_write(s->chr, &ch, 1);
         break;
     case SERIAL_INT_ENABLE:
         s->int_enable = value;
diff --git a/hw/usb-serial.c b/hw/usb-serial.c
index 298c1e9..f093d18 100644
--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -369,7 +369,7 @@ static int usb_serial_handle_data(USBDevice *dev, USBPacket *p)
     case USB_TOKEN_OUT:
         if (devep != 2)
             goto fail;
-        qemu_chr_write(s->cs, data, len);
+        qemu_chr_fe_write(s->cs, data, len);
         break;
 
     case USB_TOKEN_IN:
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index fe5e188..6c386fa 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -27,7 +27,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
     ssize_t ret;
 
-    ret = qemu_chr_write(vcon->chr, buf, len);
+    ret = qemu_chr_fe_write(vcon->chr, buf, len);
     trace_virtio_console_flush_buf(port->id, len, ret);
 
     if (ret < 0) {
diff --git a/hw/xen_console.c b/hw/xen_console.c
index 8ef104c..e218bb8 100644
--- a/hw/xen_console.c
+++ b/hw/xen_console.c
@@ -156,7 +156,7 @@ static void xencons_send(struct XenConsole *con)
 
     size = con->buffer.size - con->buffer.consumed;
     if (con->chr)
-        len = qemu_chr_write(con->chr, con->buffer.data + con->buffer.consumed,
+        len = qemu_chr_fe_write(con->chr, con->buffer.data + con->buffer.consumed,
                              size);
     else
         len = size;
diff --git a/hw/xilinx_uartlite.c b/hw/xilinx_uartlite.c
index 9b94e98..467a26c 100644
--- a/hw/xilinx_uartlite.c
+++ b/hw/xilinx_uartlite.c
@@ -129,7 +129,7 @@ uart_writel (void *opaque, target_phys_addr_t addr, uint32_t value)
 
         case R_TX:
             if (s->chr)
-                qemu_chr_write(s->chr, &ch, 1);
+                qemu_chr_fe_write(s->chr, &ch, 1);
 
             s->regs[addr] = value;
 
diff --git a/monitor.c b/monitor.c
index 1b8ba2c..38d4544 100644
--- a/monitor.c
+++ b/monitor.c
@@ -247,7 +247,7 @@ static int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
 void monitor_flush(Monitor *mon)
 {
     if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
-        qemu_chr_write(mon->chr, mon->outbuf, mon->outbuf_index);
+        qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index);
         mon->outbuf_index = 0;
     }
 }
diff --git a/qemu-char.c b/qemu-char.c
index 8e8cf31..fe5b28e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -139,7 +139,7 @@ void qemu_chr_generic_open(CharDriverState *s)
     }
 }
 
-int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len)
+int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
 {
     return s->chr_write(s, buf, len);
 }
@@ -185,7 +185,7 @@ void qemu_chr_printf(CharDriverState *s, const char *fmt, ...)
     va_list ap;
     va_start(ap, fmt);
     vsnprintf(buf, sizeof(buf), fmt, ap);
-    qemu_chr_write(s, (uint8_t *)buf, strlen(buf));
+    qemu_chr_fe_write(s, (uint8_t *)buf, strlen(buf));
     va_end(ap);
 }
 
diff --git a/qemu-char.h b/qemu-char.h
index f361c6d..90f0c41 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -74,6 +74,7 @@ struct CharDriverState {
     char *filename;
     int opened;
     int avail_connections;
+
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
@@ -87,7 +88,7 @@ void qemu_chr_guest_close(struct CharDriverState *chr);
 void qemu_chr_close(CharDriverState *chr);
 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);
+int qemu_chr_fe_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,
diff --git a/slirp/slirp.c b/slirp/slirp.c
index df787ea..b2b04fd 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -838,7 +838,7 @@ int slirp_add_exec(Slirp *slirp, int do_pty, const void *args,
 ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags)
 {
 	if (so->s == -1 && so->extra) {
-		qemu_chr_write(so->extra, buf, len);
+		qemu_chr_fe_write(so->extra, buf, len);
 		return len;
 	}
 
diff --git a/usb-redir.c b/usb-redir.c
index e212993..606bc98 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -225,7 +225,7 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
 {
     USBRedirDevice *dev = priv;
 
-    return qemu_chr_write(dev->cs, data, count);
+    return qemu_chr_fe_write(dev->cs, data, count);
 }
 
 /*
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 01/12] char: rename qemu_chr_write() to qemu_chr_fe_write()

[Avi Kivity]

On 08/01/2011 05:22 PM, Anthony Liguori wrote:
> The char layer is confusing.  There is a front-end, typically a device, that
> can send and receive data.  The front-end sends data by calling
> qemu_chr_write().
>
> The back-end, typically created via -chardev, can also send and receive data.
> Oddly, it sends data by calling qemu_chr_read().
>
> Let's be explicit about which function is for which party.

A different way to accomplish this would be to have each pipe expose two 
interfaces (a front end and a back end), and use the same functions for 
both.  Just like a unix pipe.

The back end interface would typically be an internal object.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 01/12] char: rename qemu_chr_write() to qemu_chr_fe_write()

[Anthony Liguori]

On 08/04/2011 11:00 AM, Avi Kivity wrote:
> On 08/01/2011 05:22 PM, Anthony Liguori wrote:
>> The char layer is confusing. There is a front-end, typically a device,
>> that
>> can send and receive data. The front-end sends data by calling
>> qemu_chr_write().
>>
>> The back-end, typically created via -chardev, can also send and
>> receive data.
>> Oddly, it sends data by calling qemu_chr_read().
>>
>> Let's be explicit about which function is for which party.
>
> A different way to accomplish this would be to have each pipe expose two
> interfaces (a front end and a back end), and use the same functions for
> both. Just like a unix pipe.

This is exactly what I'm trying to do.  This is why I decided to do this 
before QOM, because this will change the relationships between objects 
dramatically since you no longer need to subclass CharDriverState since 
it's just being used as a pipe.

Regards,

Anthony Liguori

>
> The back end interface would typically be an internal object.
>

Re: [Qemu-devel] [PATCH 01/12] char: rename qemu_chr_write() to qemu_chr_fe_write()

[Avi Kivity]

On 08/04/2011 07:11 PM, Anthony Liguori wrote:
> On 08/04/2011 11:00 AM, Avi Kivity wrote:
>> On 08/01/2011 05:22 PM, Anthony Liguori wrote:
>>> The char layer is confusing. There is a front-end, typically a device,
>>> that
>>> can send and receive data. The front-end sends data by calling
>>> qemu_chr_write().
>>>
>>> The back-end, typically created via -chardev, can also send and
>>> receive data.
>>> Oddly, it sends data by calling qemu_chr_read().
>>>
>>> Let's be explicit about which function is for which party.
>>
>> A different way to accomplish this would be to have each pipe expose two
>> interfaces (a front end and a back end), and use the same functions for
>> both. Just like a unix pipe.
>
> This is exactly what I'm trying to do.  This is why I decided to do 
> this before QOM, because this will change the relationships between 
> objects dramatically since you no longer need to subclass 
> CharDriverState since it's just being used as a pipe.

Yes, I'm just talking about the function names, not about the 
implementation.

   qemu_chr_fe_write(chr,...) -> qemu_chr_write(chr, ...)
   qemu_chr_be_write(chr, ...) -> qemu_chr_write(&chr->backend, ...)

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 01/12] char: rename qemu_chr_write() to qemu_chr_fe_write()

[Avi Kivity]

On 08/04/2011 07:14 PM, Avi Kivity wrote:
>
> Yes, I'm just talking about the function names, not about the 
> implementation.
>
>   qemu_chr_fe_write(chr,...) -> qemu_chr_write(chr, ...)
>   qemu_chr_be_write(chr, ...) -> qemu_chr_write(&chr->backend, ...)
>

And, if you want an internal pipe:

    QemuPipeEndpoint pipe1, pipe2;

    qemu_chr_pipe_init(&pipe0, &pipe1);

so clients don't have to choose between frontend and backend.  Finally 
you can connect COM1: to COM2: with a qemu-provided null modem cable!

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 01/12] char: rename qemu_chr_write() to qemu_chr_fe_write()

[Anthony Liguori]

On 08/04/2011 11:17 AM, Avi Kivity wrote:
> On 08/04/2011 07:14 PM, Avi Kivity wrote:
>>
>> Yes, I'm just talking about the function names, not about the
>> implementation.
>>
>> qemu_chr_fe_write(chr,...) -> qemu_chr_write(chr, ...)
>> qemu_chr_be_write(chr, ...) -> qemu_chr_write(&chr->backend, ...)
>>
>
> And, if you want an internal pipe:
>
> QemuPipeEndpoint pipe1, pipe2;
>
> qemu_chr_pipe_init(&pipe0, &pipe1);
>
> so clients don't have to choose between frontend and backend. Finally
> you can connect COM1: to COM2: with a qemu-provided null modem cable!

Yes, that's exactly the goal :-)

The only problem left to be resolved is how to handle ioctl(). 
qemu_chr_ioctl() forces asymmetry today.

I think if we merge ioctl into qemu_chr_event() by adding a return and 
data payload, it'll make the semantics be synchronous messaging and 
it'll be easy to make ioctl work.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH 01/12] char: rename qemu_chr_write() to qemu_chr_fe_write()

[Anthony Liguori]

On 08/04/2011 11:14 AM, Avi Kivity wrote:
> On 08/04/2011 07:11 PM, Anthony Liguori wrote:
>> On 08/04/2011 11:00 AM, Avi Kivity wrote:
>>> On 08/01/2011 05:22 PM, Anthony Liguori wrote:
>>>> The char layer is confusing. There is a front-end, typically a device,
>>>> that
>>>> can send and receive data. The front-end sends data by calling
>>>> qemu_chr_write().
>>>>
>>>> The back-end, typically created via -chardev, can also send and
>>>> receive data.
>>>> Oddly, it sends data by calling qemu_chr_read().
>>>>
>>>> Let's be explicit about which function is for which party.
>>>
>>> A different way to accomplish this would be to have each pipe expose two
>>> interfaces (a front end and a back end), and use the same functions for
>>> both. Just like a unix pipe.
>>
>> This is exactly what I'm trying to do. This is why I decided to do
>> this before QOM, because this will change the relationships between
>> objects dramatically since you no longer need to subclass
>> CharDriverState since it's just being used as a pipe.
>
> Yes, I'm just talking about the function names, not about the
> implementation.
>
> qemu_chr_fe_write(chr,...) -> qemu_chr_write(chr, ...)
> qemu_chr_be_write(chr, ...) -> qemu_chr_write(&chr->backend, ...)

These are just the incremental patches to get us there.  Some more 
refactoring is needed to get to a single function.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 02/12] char: rename qemu_chr_[can_]read() to qemu_chr_be_[can_]write().

[Anthony Liguori]

To be clear about who calls this function and for what purpose.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 console.c         |    4 ++--
 gdbstub.c         |    2 +-
 hw/baum.c         |   12 ++++++------
 hw/msmouse.c      |    2 +-
 qemu-char.c       |   38 +++++++++++++++++++-------------------
 qemu-char.h       |    4 ++--
 spice-qemu-char.c |    4 ++--
 7 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/console.c b/console.c
index 242086c..489c74c 100644
--- a/console.c
+++ b/console.c
@@ -1123,14 +1123,14 @@ static void kbd_send_chars(void *opaque)
     int len;
     uint8_t buf[16];
 
-    len = qemu_chr_can_read(s->chr);
+    len = qemu_chr_be_can_write(s->chr);
     if (len > s->out_fifo.count)
         len = s->out_fifo.count;
     if (len > 0) {
         if (len > sizeof(buf))
             len = sizeof(buf);
         qemu_fifo_read(&s->out_fifo, buf, len);
-        qemu_chr_read(s->chr, buf, len);
+        qemu_chr_be_write(s->chr, buf, len);
     }
     /* characters are pending: we send them a bit later (XXX:
        horrible, should change char device API) */
diff --git a/gdbstub.c b/gdbstub.c
index d6c362e..058c626 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2194,7 +2194,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             hextomem(mem_buf, p + 5, len);
             len = len / 2;
             mem_buf[len++] = 0;
-            qemu_chr_read(s->mon_chr, mem_buf, len);
+            qemu_chr_be_write(s->mon_chr, mem_buf, len);
             put_packet(s, "OK");
             break;
         }
diff --git a/hw/baum.c b/hw/baum.c
index 33a22a7..244eee5 100644
--- a/hw/baum.c
+++ b/hw/baum.c
@@ -223,7 +223,7 @@ static void baum_accept_input(struct CharDriverState *chr)
 
     if (!baum->out_buf_used)
         return;
-    room = qemu_chr_can_read(chr);
+    room = qemu_chr_be_can_write(chr);
     if (!room)
         return;
     if (room > baum->out_buf_used)
@@ -231,12 +231,12 @@ static void baum_accept_input(struct CharDriverState *chr)
 
     first = BUF_SIZE - baum->out_buf_ptr;
     if (room > first) {
-        qemu_chr_read(chr, baum->out_buf + baum->out_buf_ptr, first);
+        qemu_chr_be_write(chr, baum->out_buf + baum->out_buf_ptr, first);
         baum->out_buf_ptr = 0;
         baum->out_buf_used -= first;
         room -= first;
     }
-    qemu_chr_read(chr, baum->out_buf + baum->out_buf_ptr, room);
+    qemu_chr_be_write(chr, baum->out_buf + baum->out_buf_ptr, room);
     baum->out_buf_ptr += room;
     baum->out_buf_used -= room;
 }
@@ -250,16 +250,16 @@ static void baum_write_packet(BaumDriverState *baum, const uint8_t *buf, int len
     while (len--)
         if ((*cur++ = *buf++) == ESC)
             *cur++ = ESC;
-    room = qemu_chr_can_read(baum->chr);
+    room = qemu_chr_be_can_write(baum->chr);
     len = cur - io_buf;
     if (len <= room) {
         /* Fits */
-        qemu_chr_read(baum->chr, io_buf, len);
+        qemu_chr_be_write(baum->chr, io_buf, len);
     } else {
         int first;
         uint8_t out;
         /* Can't fit all, send what can be, and store the rest. */
-        qemu_chr_read(baum->chr, io_buf, room);
+        qemu_chr_be_write(baum->chr, io_buf, room);
         len -= room;
         cur = io_buf + room;
         if (len > BUF_SIZE - baum->out_buf_used) {
diff --git a/hw/msmouse.c b/hw/msmouse.c
index 67c6cd4..fbed80b 100644
--- a/hw/msmouse.c
+++ b/hw/msmouse.c
@@ -50,7 +50,7 @@ static void msmouse_event(void *opaque,
     /* We always send the packet of, so that we do not have to keep track
        of previous state of the middle button. This can potentially confuse
        some very old drivers for two button mice though. */
-    qemu_chr_read(chr, bytes, 4);
+    qemu_chr_be_write(chr, bytes, 4);
 }
 
 static int msmouse_chr_write (struct CharDriverState *s, const uint8_t *buf, int len)
diff --git a/qemu-char.c b/qemu-char.c
index fe5b28e..795a3cc 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -151,14 +151,14 @@ int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
     return s->chr_ioctl(s, cmd, arg);
 }
 
-int qemu_chr_can_read(CharDriverState *s)
+int qemu_chr_be_can_write(CharDriverState *s)
 {
     if (!s->chr_can_read)
         return 0;
     return s->chr_can_read(s->handler_opaque);
 }
 
-void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
+void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len)
 {
     s->chr_read(s->handler_opaque, buf, len);
 }
@@ -565,7 +565,7 @@ static int fd_chr_read_poll(void *opaque)
     CharDriverState *chr = opaque;
     FDCharDriver *s = chr->opaque;
 
-    s->max_size = qemu_chr_can_read(chr);
+    s->max_size = qemu_chr_be_can_write(chr);
     return s->max_size;
 }
 
@@ -589,7 +589,7 @@ static void fd_chr_read(void *opaque)
         return;
     }
     if (size > 0) {
-        qemu_chr_read(chr, buf, size);
+        qemu_chr_be_write(chr, buf, size);
     }
 }
 
@@ -699,8 +699,8 @@ static int stdio_read_poll(void *opaque)
     CharDriverState *chr = opaque;
 
     /* try to flush the queue if needed */
-    if (term_fifo_size != 0 && qemu_chr_can_read(chr) > 0) {
-        qemu_chr_read(chr, term_fifo, 1);
+    if (term_fifo_size != 0 && qemu_chr_be_can_write(chr) > 0) {
+        qemu_chr_be_write(chr, term_fifo, 1);
         term_fifo_size = 0;
     }
     /* see if we can absorb more chars */
@@ -724,8 +724,8 @@ static void stdio_read(void *opaque)
         return;
     }
     if (size > 0) {
-        if (qemu_chr_can_read(chr) > 0) {
-            qemu_chr_read(chr, buf, 1);
+        if (qemu_chr_be_can_write(chr) > 0) {
+            qemu_chr_be_write(chr, buf, 1);
         } else if (term_fifo_size == 0) {
             term_fifo[term_fifo_size++] = buf[0];
         }
@@ -890,7 +890,7 @@ static int pty_chr_read_poll(void *opaque)
     CharDriverState *chr = opaque;
     PtyCharDriver *s = chr->opaque;
 
-    s->read_bytes = qemu_chr_can_read(chr);
+    s->read_bytes = qemu_chr_be_can_write(chr);
     return s->read_bytes;
 }
 
@@ -914,7 +914,7 @@ static void pty_chr_read(void *opaque)
     }
     if (size > 0) {
         pty_chr_state(chr, 1);
-        qemu_chr_read(chr, buf, size);
+        qemu_chr_be_write(chr, buf, size);
     }
 }
 
@@ -1602,7 +1602,7 @@ static int win_chr_read_poll(CharDriverState *chr)
 {
     WinCharState *s = chr->opaque;
 
-    s->max_size = qemu_chr_can_read(chr);
+    s->max_size = qemu_chr_be_can_write(chr);
     return s->max_size;
 }
 
@@ -1624,7 +1624,7 @@ static void win_chr_readfile(CharDriverState *chr)
     }
 
     if (size > 0) {
-        qemu_chr_read(chr, buf, size);
+        qemu_chr_be_write(chr, buf, size);
     }
 }
 
@@ -1840,15 +1840,15 @@ static int udp_chr_read_poll(void *opaque)
     CharDriverState *chr = opaque;
     NetCharDriver *s = chr->opaque;
 
-    s->max_size = qemu_chr_can_read(chr);
+    s->max_size = qemu_chr_be_can_write(chr);
 
     /* If there were any stray characters in the queue process them
      * first
      */
     while (s->max_size > 0 && s->bufptr < s->bufcnt) {
-        qemu_chr_read(chr, &s->buf[s->bufptr], 1);
+        qemu_chr_be_write(chr, &s->buf[s->bufptr], 1);
         s->bufptr++;
-        s->max_size = qemu_chr_can_read(chr);
+        s->max_size = qemu_chr_be_can_write(chr);
     }
     return s->max_size;
 }
@@ -1867,9 +1867,9 @@ static void udp_chr_read(void *opaque)
 
     s->bufptr = 0;
     while (s->max_size > 0 && s->bufptr < s->bufcnt) {
-        qemu_chr_read(chr, &s->buf[s->bufptr], 1);
+        qemu_chr_be_write(chr, &s->buf[s->bufptr], 1);
         s->bufptr++;
-        s->max_size = qemu_chr_can_read(chr);
+        s->max_size = qemu_chr_be_can_write(chr);
     }
 }
 
@@ -1963,7 +1963,7 @@ static int tcp_chr_read_poll(void *opaque)
     TCPCharDriver *s = chr->opaque;
     if (!s->connected)
         return 0;
-    s->max_size = qemu_chr_can_read(chr);
+    s->max_size = qemu_chr_be_can_write(chr);
     return s->max_size;
 }
 
@@ -2109,7 +2109,7 @@ static void tcp_chr_read(void *opaque)
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
         if (size > 0)
-            qemu_chr_read(chr, buf, size);
+            qemu_chr_be_write(chr, buf, size);
     }
 }
 
diff --git a/qemu-char.h b/qemu-char.h
index 90f0c41..bcd413c 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -97,8 +97,8 @@ void qemu_chr_add_handlers(CharDriverState *s,
                            void *opaque);
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
 void qemu_chr_generic_open(CharDriverState *s);
-int qemu_chr_can_read(CharDriverState *s);
-void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
+int qemu_chr_be_can_write(CharDriverState *s);
+void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
 int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 int qemu_chr_add_client(CharDriverState *s, int fd);
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 95bf6b6..3d06ac4 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -36,10 +36,10 @@ static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
 
     while (len > 0) {
         last_out = MIN(len, VMC_MAX_HOST_WRITE);
-        if (qemu_chr_can_read(scd->chr) < last_out) {
+        if (qemu_chr_be_can_write(scd->chr) < last_out) {
             break;
         }
-        qemu_chr_read(scd->chr, p, last_out);
+        qemu_chr_be_write(scd->chr, p, last_out);
         out += last_out;
         len -= last_out;
         p += last_out;
-- 
1.7.4.1

[Qemu-devel] [PATCH 03/12] char: introduce tx queue to enable Unix style flow control

[Anthony Liguori]

The char layer tries very hard to avoid using an intermediate buffer.  The
implication of this is that when the backend does a write(), the data for that
write must be immediately passed to the front end.

Flow control is needed to handle the likely event that the front end is not
able to handle the data at this point in time.  We implement flow control
today by allowing the front ends to register a polling function.  The polling
function returns non-zero when it is able to receive data.

This works okay because most backends are tied to some sort of file descriptor
and our main loop allows polling to be included with file descriptor
registration.

This falls completely apart when dealing with the front end writing to the
back end though because the front end (devices) don't have an obvious place to
integrate polling.

Short summary: we're broken by design.  A way to fix this is to eliminate
polling entirely and use a Unix style flow control mechanism.  This involves
using an intermediate buffer and allowing registration of notifications when
the buffer either has data in it (readability) or is not full (writability).

This patch introduces a queue and uses it for front end -> back end writes.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qemu-char.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-char.h |   11 +++++++++++
 2 files changed, 68 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 795a3cc..3f9b32c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -139,9 +139,65 @@ void qemu_chr_generic_open(CharDriverState *s)
     }
 }
 
+static size_t char_queue_write(CharQueue *q, const void *data, size_t size)
+{
+    const uint8_t *ptr = data;
+    size_t i;
+
+    for (i = 0; i < size; i++) {
+        if ((q->prod - q->cons) == sizeof(q->ring)) {
+            break;
+        }
+
+        q->ring[q->prod % sizeof(q->ring)] = ptr[i];
+        q->prod++;
+    }
+
+    return i;
+}
+
+static size_t char_queue_read(CharQueue *q, void *data, size_t size)
+{
+    uint8_t *ptr = data;
+    size_t i;
+
+    for (i = 0; i < size; i++) {
+        if (q->cons == q->prod) {
+            break;
+        }
+
+        ptr[i] = q->ring[q->cons % sizeof(q->ring)];
+        q->cons++;
+    }
+
+    return i;
+}
+
+static void qemu_chr_flush_fe_tx(CharDriverState *s)
+{
+    uint8_t buf[MAX_CHAR_QUEUE_RING];
+    int len, written_len;
+
+    /* Drain the queue into a flat buffer */
+    len = char_queue_read(&s->fe_tx, buf, sizeof(buf));
+
+    written_len = s->chr_write(s, buf, len);
+    if (written_len < len) {
+        /* If the backend didn't accept the full write, queue the unwritten
+         * data back in the queue. */
+        char_queue_write(&s->fe_tx, &buf[written_len], len - written_len);
+    }
+}
+
 int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
 {
-    return s->chr_write(s, buf, len);
+    int ret;
+
+    ret = char_queue_write(&s->fe_tx, buf, len);
+
+    qemu_chr_flush_fe_tx(s);
+
+    return ret;
 }
 
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
diff --git a/qemu-char.h b/qemu-char.h
index bcd413c..bb9c1a7 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -51,6 +51,15 @@ typedef struct {
 
 typedef void IOEventHandler(void *opaque, int event);
 
+#define MAX_CHAR_QUEUE_RING 1024
+
+typedef struct CharQueue
+{
+    uint32_t prod;
+    uint32_t cons;
+    uint8_t ring[MAX_CHAR_QUEUE_RING];
+} CharQueue;
+
 struct CharDriverState {
     void (*init)(struct CharDriverState *s);
     int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
@@ -75,6 +84,8 @@ struct CharDriverState {
     int opened;
     int avail_connections;
 
+    CharQueue fe_tx;
+
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 03/12] char: introduce tx queue to enable Unix style flow control

[Avi Kivity]

On 08/01/2011 05:23 PM, Anthony Liguori wrote:
> The char layer tries very hard to avoid using an intermediate buffer.  The
> implication of this is that when the backend does a write(), the data for that
> write must be immediately passed to the front end.
>
> Flow control is needed to handle the likely event that the front end is not
> able to handle the data at this point in time.  We implement flow control
> today by allowing the front ends to register a polling function.  The polling
> function returns non-zero when it is able to receive data.
>
> This works okay because most backends are tied to some sort of file descriptor
> and our main loop allows polling to be included with file descriptor
> registration.
>
> This falls completely apart when dealing with the front end writing to the
> back end though because the front end (devices) don't have an obvious place to
> integrate polling.
>
> Short summary: we're broken by design.  A way to fix this is to eliminate
> polling entirely and use a Unix style flow control mechanism.  This involves
> using an intermediate buffer and allowing registration of notifications when
> the buffer either has data in it (readability) or is not full (writability).
>

If you don't have an obvious place to integrate polling, how do you poll 
for writability?

Although, providing a reasonably sized buffer and blocking the vcpu when 
it's full is a lot better than what we have now.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 03/12] char: introduce tx queue to enable Unix style flow control

[Anthony Liguori]

On 08/04/2011 11:04 AM, Avi Kivity wrote:
> On 08/01/2011 05:23 PM, Anthony Liguori wrote:
>> The char layer tries very hard to avoid using an intermediate buffer. The
>> implication of this is that when the backend does a write(), the data
>> for that
>> write must be immediately passed to the front end.
>>
>> Flow control is needed to handle the likely event that the front end
>> is not
>> able to handle the data at this point in time. We implement flow control
>> today by allowing the front ends to register a polling function. The
>> polling
>> function returns non-zero when it is able to receive data.
>>
>> This works okay because most backends are tied to some sort of file
>> descriptor
>> and our main loop allows polling to be included with file descriptor
>> registration.
>>
>> This falls completely apart when dealing with the front end writing to
>> the
>> back end though because the front end (devices) don't have an obvious
>> place to
>> integrate polling.
>>
>> Short summary: we're broken by design. A way to fix this is to eliminate
>> polling entirely and use a Unix style flow control mechanism. This
>> involves
>> using an intermediate buffer and allowing registration of
>> notifications when
>> the buffer either has data in it (readability) or is not full
>> (writability).
>>
>
> If you don't have an obvious place to integrate polling, how do you poll
> for writability?

You poll by trying to write.  If write fails, you can set a callback for 
notification for when it becomes writable.

> Although, providing a reasonably sized buffer and blocking the vcpu when
> it's full is a lot better than what we have now.

This series won't block the vcpu.  The devices can (and will) register 
callbacks for when the pipe is writable again.

Regards,

Anthony Liguori

>

[Qemu-devel] [PATCH 04/12] char: introduce backend tx queue

[Anthony Liguori]

While the front tx queue has no flow control, the backend tx queue uses a
polling function to determine when the front end can receive data.

To convert this to the new queue model, we simply try to flush the backend tx
queue whenever we poll.  We then return the remaining space in the queue as
the value of the polling function.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qemu-char.c |   49 ++++++++++++++++++++++++++++++++++++++++---------
 qemu-char.h |    3 ++-
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 3f9b32c..2746652 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -173,6 +173,11 @@ static size_t char_queue_read(CharQueue *q, void *data, size_t size)
     return i;
 }
 
+static uint32_t char_queue_get_avail(CharQueue *q)
+{
+    return sizeof(q->ring) - (q->prod - q->cons);
+}
+
 static void qemu_chr_flush_fe_tx(CharDriverState *s)
 {
     uint8_t buf[MAX_CHAR_QUEUE_RING];
@@ -200,23 +205,49 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
     return ret;
 }
 
-int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
+static void qemu_chr_flush_be_tx(CharDriverState *s)
 {
-    if (!s->chr_ioctl)
-        return -ENOTSUP;
-    return s->chr_ioctl(s, cmd, arg);
+    uint8_t buf[MAX_CHAR_QUEUE_RING];
+    int len;
+
+    /* Only drain what the be can handle */
+    len = s->chr_can_read(s->handler_opaque);
+    if (len == 0) {
+        return;
+    }
+
+    len = char_queue_read(&s->be_tx, buf, len);
+
+    /* We only drained what we knew the be could handle so we don't need to
+     * requeue any data. */
+    s->chr_read(s, buf, len);
 }
 
 int qemu_chr_be_can_write(CharDriverState *s)
 {
-    if (!s->chr_can_read)
-        return 0;
-    return s->chr_can_read(s->handler_opaque);
+    /* Try to flush any queued data before returning how much data we can
+     * accept. */
+    qemu_chr_flush_be_tx(s);
+
+    return char_queue_get_avail(&s->be_tx);
 }
 
-void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len)
+int qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len)
 {
-    s->chr_read(s->handler_opaque, buf, len);
+    int ret;
+
+    ret = char_queue_write(&s->be_tx, buf, len);
+
+    qemu_chr_flush_be_tx(s);
+
+    return ret;
+}
+
+int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
+{
+    if (!s->chr_ioctl)
+        return -ENOTSUP;
+    return s->chr_ioctl(s, cmd, arg);
 }
 
 int qemu_chr_get_msgfd(CharDriverState *s)
diff --git a/qemu-char.h b/qemu-char.h
index bb9c1a7..85735b5 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -85,6 +85,7 @@ struct CharDriverState {
     int avail_connections;
 
     CharQueue fe_tx;
+    CharQueue be_tx;
 
     QTAILQ_ENTRY(CharDriverState) next;
 };
@@ -109,7 +110,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
 void qemu_chr_generic_open(CharDriverState *s);
 int qemu_chr_be_can_write(CharDriverState *s);
-void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
+int qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
 int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 int qemu_chr_add_client(CharDriverState *s, int fd);
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 04/12] char: introduce backend tx queue

[Stefan Hajnoczi]

On Mon, Aug 1, 2011 at 3:23 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> While the front tx queue has no flow control, the backend tx queue uses a
> polling function to determine when the front end can receive data.
>
> To convert this to the new queue model, we simply try to flush the backend tx
> queue whenever we poll.  We then return the remaining space in the queue as
> the value of the polling function.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qemu-char.c |   49 ++++++++++++++++++++++++++++++++++++++++---------
>  qemu-char.h |    3 ++-
>  2 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 3f9b32c..2746652 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -173,6 +173,11 @@ static size_t char_queue_read(CharQueue *q, void *data, size_t size)
>     return i;
>  }
>
> +static uint32_t char_queue_get_avail(CharQueue *q)
> +{
> +    return sizeof(q->ring) - (q->prod - q->cons);
> +}

"avail" confuses me.  How many bytes are available for the consumer?
How many bytes are available for the producer?

How about char_queue_get_space() or char_queue_get_nfree()?

Stefan

Re: [Qemu-devel] [PATCH 04/12] char: introduce backend tx queue

[Anthony Liguori]

On 08/01/2011 10:33 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 1, 2011 at 3:23 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> While the front tx queue has no flow control, the backend tx queue uses a
>> polling function to determine when the front end can receive data.
>>
>> To convert this to the new queue model, we simply try to flush the backend tx
>> queue whenever we poll.  We then return the remaining space in the queue as
>> the value of the polling function.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   qemu-char.c |   49 ++++++++++++++++++++++++++++++++++++++++---------
>>   qemu-char.h |    3 ++-
>>   2 files changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 3f9b32c..2746652 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -173,6 +173,11 @@ static size_t char_queue_read(CharQueue *q, void *data, size_t size)
>>      return i;
>>   }
>>
>> +static uint32_t char_queue_get_avail(CharQueue *q)
>> +{
>> +    return sizeof(q->ring) - (q->prod - q->cons);
>> +}
>
> "avail" confuses me.  How many bytes are available for the consumer?
> How many bytes are available for the producer?
>
> How about char_queue_get_space() or char_queue_get_nfree()?

Sure.

Regards,

Anthony Liguori

>
> Stefan
>

[Qemu-devel] [PATCH 05/12] char: add read functions for backend and frontend

[Anthony Liguori]

Instead of waiting for a callback to send data, allow for reading the backend
and frontend tx queues directly.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qemu-char.c |   10 ++++++++++
 qemu-char.h |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 2746652..0e4a30c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -205,6 +205,11 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
     return ret;
 }
 
+int qemu_chr_fe_read(CharDriverState *s, uint8_t *buf, int len)
+{
+    return char_queue_read(&s->be_tx, buf, len);
+}
+
 static void qemu_chr_flush_be_tx(CharDriverState *s)
 {
     uint8_t buf[MAX_CHAR_QUEUE_RING];
@@ -243,6 +248,11 @@ int qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len)
     return ret;
 }
 
+int qemu_chr_be_read(CharDriverState *s, uint8_t *buf, int len)
+{
+    return char_queue_read(&s->fe_tx, buf, len);
+}
+
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
 {
     if (!s->chr_ioctl)
diff --git a/qemu-char.h b/qemu-char.h
index 85735b5..a75bd1c 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -101,6 +101,7 @@ void qemu_chr_close(CharDriverState *chr);
 void qemu_chr_printf(CharDriverState *s, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len);
+int qemu_chr_fe_read(CharDriverState *s, 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,
@@ -111,6 +112,7 @@ int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
 void qemu_chr_generic_open(CharDriverState *s);
 int qemu_chr_be_can_write(CharDriverState *s);
 int qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
+int qemu_chr_be_read(CharDriverState *s, uint8_t *buf, int len);
 int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 int qemu_chr_add_client(CharDriverState *s, int fd);
-- 
1.7.4.1

[Qemu-devel] [PATCH 06/12] char: add an edge event API for the front ends

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qemu-char.c |   91 +++++++++++++++++++++++++++++++++++++++++++++++++----------
 qemu-char.h |    9 ++++++
 2 files changed, 85 insertions(+), 15 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 0e4a30c..9e40e04 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -139,13 +139,23 @@ void qemu_chr_generic_open(CharDriverState *s)
     }
 }
 
+static uint32_t char_queue_get_avail(CharQueue *q)
+{
+    return sizeof(q->ring) - (q->prod - q->cons);
+}
+
+static bool char_queue_get_empty(CharQueue *q)
+{
+    return (q->cons == q->prod);
+}
+
 static size_t char_queue_write(CharQueue *q, const void *data, size_t size)
 {
     const uint8_t *ptr = data;
     size_t i;
 
     for (i = 0; i < size; i++) {
-        if ((q->prod - q->cons) == sizeof(q->ring)) {
+        if (char_queue_get_avail(q) == 0) {
             break;
         }
 
@@ -162,7 +172,7 @@ static size_t char_queue_read(CharQueue *q, void *data, size_t size)
     size_t i;
 
     for (i = 0; i < size; i++) {
-        if (q->cons == q->prod) {
+        if (char_queue_get_empty(q)) {
             break;
         }
 
@@ -173,25 +183,17 @@ static size_t char_queue_read(CharQueue *q, void *data, size_t size)
     return i;
 }
 
-static uint32_t char_queue_get_avail(CharQueue *q)
-{
-    return sizeof(q->ring) - (q->prod - q->cons);
-}
-
 static void qemu_chr_flush_fe_tx(CharDriverState *s)
 {
     uint8_t buf[MAX_CHAR_QUEUE_RING];
-    int len, written_len;
+    int len;
 
     /* Drain the queue into a flat buffer */
     len = char_queue_read(&s->fe_tx, buf, sizeof(buf));
 
-    written_len = s->chr_write(s, buf, len);
-    if (written_len < len) {
-        /* If the backend didn't accept the full write, queue the unwritten
-         * data back in the queue. */
-        char_queue_write(&s->fe_tx, &buf[written_len], len - written_len);
-    }
+    s->chr_write(s, buf, len);
+
+    /* We drop unwritten data until we have backend flow control */
 }
 
 int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
@@ -210,11 +212,43 @@ int qemu_chr_fe_read(CharDriverState *s, uint8_t *buf, int len)
     return char_queue_read(&s->be_tx, buf, len);
 }
 
+void qemu_chr_fe_set_handlers(CharDriverState *s,
+                              IOHandler *chr_read,
+                              IOHandler *chr_write,
+                              IOEventHandler *chr_event,
+                              void *opaque)
+{
+    if (!opaque && !chr_read && !chr_write && !chr_event) {
+        /* chr driver being released. */
+        ++s->avail_connections;
+    }
+    s->fe_read = chr_read;
+    s->fe_write = chr_write;
+    s->chr_event = chr_event;
+    s->handler_opaque = opaque;
+
+    /* FIXME broken for mux */
+    if (s->chr_update_read_handler) {
+        s->chr_update_read_handler(s);
+    }
+
+    /* We're connecting to an already opened device, so let's make sure we
+       also get the open event */
+    if (s->opened) {
+        qemu_chr_generic_open(s);
+    }
+}
+
 static void qemu_chr_flush_be_tx(CharDriverState *s)
 {
     uint8_t buf[MAX_CHAR_QUEUE_RING];
     int len;
 
+    /* Don't invoke the polling handlers if we have edge handlers installed. */
+    if (s->fe_read || s->fe_write) {
+        return;
+    }
+
     /* Only drain what the be can handle */
     len = s->chr_can_read(s->handler_opaque);
     if (len == 0) {
@@ -240,9 +274,21 @@ int qemu_chr_be_can_write(CharDriverState *s)
 int qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len)
 {
     int ret;
+    bool is_empty;
+
+    is_empty = char_queue_get_empty(&s->be_tx);
 
     ret = char_queue_write(&s->be_tx, buf, len);
 
+    /* If the queue was empty, and now it's not, trigger a read edge
+     * event. */
+    if ((s->fe_read || s->fe_write) &&
+        (is_empty && !char_queue_get_empty(&s->be_tx))) {
+        if (s->fe_read) {
+            s->fe_read(s->handler_opaque);
+        }
+    }
+
     qemu_chr_flush_be_tx(s);
 
     return ret;
@@ -250,7 +296,22 @@ int qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len)
 
 int qemu_chr_be_read(CharDriverState *s, uint8_t *buf, int len)
 {
-    return char_queue_read(&s->fe_tx, buf, len);
+    bool is_full;
+    int ret;
+
+    is_full = (char_queue_get_avail(&s->fe_tx) == 0);
+
+    ret = char_queue_read(&s->fe_tx, buf, len);
+
+    /* If the queue was full, and now it's not, trigger an write edge
+     * event. */
+    if (is_full && char_queue_get_avail(&s->fe_tx)) {
+        if (s->fe_write) {
+            s->fe_write(s->handler_opaque);
+        }
+    }
+
+    return ret;
 }
 
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
diff --git a/qemu-char.h b/qemu-char.h
index a75bd1c..84fd628 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -70,6 +70,8 @@ struct CharDriverState {
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
+    IOHandler *fe_read;
+    IOHandler *fe_write;
     void *handler_opaque;
     void (*chr_send_event)(struct CharDriverState *chr, int event);
     void (*chr_close)(struct CharDriverState *chr);
@@ -108,6 +110,13 @@ void qemu_chr_add_handlers(CharDriverState *s,
                            IOReadHandler *fd_read,
                            IOEventHandler *fd_event,
                            void *opaque);
+
+void qemu_chr_fe_set_handlers(CharDriverState *s,
+                              IOHandler *chr_read,
+                              IOHandler *chr_write,
+                              IOEventHandler *chr_event,
+                              void *opaque);
+
 int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
 void qemu_chr_generic_open(CharDriverState *s);
 int qemu_chr_be_can_write(CharDriverState *s);
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 06/12] char: add an edge event API for the front ends

[Stefan Hajnoczi]

On Mon, Aug 1, 2011 at 3:23 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qemu-char.c |   91 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>  qemu-char.h |    9 ++++++
>  2 files changed, 85 insertions(+), 15 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 0e4a30c..9e40e04 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -139,13 +139,23 @@ void qemu_chr_generic_open(CharDriverState *s)
>     }
>  }
>
> +static uint32_t char_queue_get_avail(CharQueue *q)
> +{
> +    return sizeof(q->ring) - (q->prod - q->cons);
> +}
> +
> +static bool char_queue_get_empty(CharQueue *q)
> +{
> +    return (q->cons == q->prod);
> +}

bool function naming nitpick: char_queue_is_empty() is clearer than
char_queue_get_empty().  "is" and "has" are always bool, "get" could
return anything.

Stefan

Re: [Qemu-devel] [PATCH 06/12] char: add an edge event API for the front ends

[Anthony Liguori]

On 08/01/2011 10:39 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 1, 2011 at 3:23 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   qemu-char.c |   91 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>>   qemu-char.h |    9 ++++++
>>   2 files changed, 85 insertions(+), 15 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 0e4a30c..9e40e04 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -139,13 +139,23 @@ void qemu_chr_generic_open(CharDriverState *s)
>>      }
>>   }
>>
>> +static uint32_t char_queue_get_avail(CharQueue *q)
>> +{
>> +    return sizeof(q->ring) - (q->prod - q->cons);
>> +}
>> +
>> +static bool char_queue_get_empty(CharQueue *q)
>> +{
>> +    return (q->cons == q->prod);
>> +}
>
> bool function naming nitpick: char_queue_is_empty() is clearer than
> char_queue_get_empty().  "is" and "has" are always bool, "get" could
> return anything.

Ack.

I thought the same thing to myself, meant to rename it, and then 
promptly forgot to :-)

Regards,

Anthony Liguori

>
> Stefan
>

[Qemu-devel] [PATCH 07/12] char: add backend edge notification interface

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qemu-char.c |   38 ++++++++++++++++++++++++++++++++++----
 qemu-char.h |    2 ++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 9e40e04..4ba18e2 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -188,6 +188,11 @@ static void qemu_chr_flush_fe_tx(CharDriverState *s)
     uint8_t buf[MAX_CHAR_QUEUE_RING];
     int len;
 
+    /* Don't use polling queue if new style handlers are registered */
+    if (s->be_read || s->be_write) {
+        return;
+    }
+
     /* Drain the queue into a flat buffer */
     len = char_queue_read(&s->fe_tx, buf, sizeof(buf));
 
@@ -199,9 +204,20 @@ static void qemu_chr_flush_fe_tx(CharDriverState *s)
 int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
 {
     int ret;
+    bool is_empty;
+
+    is_empty = char_queue_get_empty(&s->fe_tx);
 
     ret = char_queue_write(&s->fe_tx, buf, len);
 
+    /* If the queue was empty, and now it's not, generate a read edge
+     * event for the backend. */
+    if (is_empty && !char_queue_get_empty(&s->fe_tx)) {
+        if (s->be_read) {
+            s->be_read(s->opaque);
+        }
+    }
+
     qemu_chr_flush_fe_tx(s);
 
     return ret;
@@ -209,7 +225,22 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
 
 int qemu_chr_fe_read(CharDriverState *s, uint8_t *buf, int len)
 {
-    return char_queue_read(&s->be_tx, buf, len);
+    bool is_full;
+    int ret;
+
+    is_full = (char_queue_get_avail(&s->be_tx) == 0);
+
+    ret = char_queue_read(&s->be_tx, buf, len);
+
+    /* If the queue was empty, and now its not, generate a write edge
+     * event for the backend. */
+    if (is_full && char_queue_get_avail(&s->be_tx)) {
+        if (s->be_write) {
+            s->be_write(s->opaque);
+        }
+    }
+
+    return ret;
 }
 
 void qemu_chr_fe_set_handlers(CharDriverState *s,
@@ -259,7 +290,7 @@ static void qemu_chr_flush_be_tx(CharDriverState *s)
 
     /* We only drained what we knew the be could handle so we don't need to
      * requeue any data. */
-    s->chr_read(s, buf, len);
+    s->chr_read(s->handler_opaque, buf, len);
 }
 
 int qemu_chr_be_can_write(CharDriverState *s)
@@ -282,8 +313,7 @@ int qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len)
 
     /* If the queue was empty, and now it's not, trigger a read edge
      * event. */
-    if ((s->fe_read || s->fe_write) &&
-        (is_empty && !char_queue_get_empty(&s->be_tx))) {
+    if (is_empty && !char_queue_get_empty(&s->be_tx)) {
         if (s->fe_read) {
             s->fe_read(s->handler_opaque);
         }
diff --git a/qemu-char.h b/qemu-char.h
index 84fd628..c5bbaf6 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -63,6 +63,8 @@ typedef struct CharQueue
 struct CharDriverState {
     void (*init)(struct CharDriverState *s);
     int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
+    IOHandler *be_read;
+    IOHandler *be_write;
     void (*chr_update_read_handler)(struct CharDriverState *s);
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
     int (*get_msgfd)(struct CharDriverState *s);
-- 
1.7.4.1

[Qemu-devel] [PATCH 08/12] char: make monitor use new style interface

[Anthony Liguori]

---
 monitor.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/monitor.c b/monitor.c
index 38d4544..3ca211b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4747,13 +4747,6 @@ cleanup:
     }
 }
 
-static int monitor_can_read(void *opaque)
-{
-    Monitor *mon = opaque;
-
-    return (mon->suspend_cnt == 0) ? 1 : 0;
-}
-
 static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
 {
     int is_cap = compare_cmd(cmd_name, "qmp_capabilities");
@@ -5118,24 +5111,35 @@ out:
 /**
  * monitor_control_read(): Read and handle QMP input
  */
-static void monitor_control_read(void *opaque, const uint8_t *buf, int size)
+static void monitor_control_read(void *opaque)
 {
     Monitor *old_mon = cur_mon;
+    uint8_t buf[1024];
+    int size;
 
     cur_mon = opaque;
 
-    json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
+    size = qemu_chr_fe_read(cur_mon->chr, buf, sizeof(buf));
+    json_message_parser_feed(&cur_mon->mc->parser, (const char *)buf, size);
 
     cur_mon = old_mon;
 }
 
-static void monitor_read(void *opaque, const uint8_t *buf, int size)
+static void monitor_read(void *opaque)
 {
     Monitor *old_mon = cur_mon;
+    uint8_t buf[1024];
+    int size;
     int i;
 
     cur_mon = opaque;
 
+    if (cur_mon->suspend_cnt > 0) {
+        return;
+    }
+
+    size = qemu_chr_fe_read(cur_mon->chr, buf, sizeof(buf));
+
     if (cur_mon->rs) {
         for (i = 0; i < size; i++)
             readline_handle_byte(cur_mon->rs, buf[i]);
@@ -5168,8 +5172,10 @@ void monitor_resume(Monitor *mon)
 {
     if (!mon->rs)
         return;
-    if (--mon->suspend_cnt == 0)
+    if (--mon->suspend_cnt == 0) {
         readline_show_prompt(mon->rs);
+        monitor_read(mon);
+    }
 }
 
 static QObject *get_qmp_greeting(void)
@@ -5273,12 +5279,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_fe_set_handlers(chr, monitor_control_read, NULL,
+                                 monitor_control_event, mon);
         qemu_chr_set_echo(chr, true);
     } else {
-        qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
-                              monitor_event, mon);
+        qemu_chr_fe_set_handlers(chr, monitor_read, NULL, monitor_event, mon);
     }
 
     QLIST_INSERT_HEAD(&mon_list, mon, entry);
-- 
1.7.4.1

[Qemu-devel] [PATCH 09/12] char: rename qemu_chr_guest_open() -> qemu_chr_fe_open()

[Anthony Liguori]

---
 hw/virtio-console.c |    2 +-
 qemu-char.c         |    2 +-
 qemu-char.h         |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 6c386fa..21f752b 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -52,7 +52,7 @@ static void guest_open(VirtIOSerialPort *port)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    qemu_chr_guest_open(vcon->chr);
+    qemu_chr_fe_open(vcon->chr);
 }
 
 /* Callback function that's called when the guest closes the port */
diff --git a/qemu-char.c b/qemu-char.c
index 4ba18e2..7e2cff0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2820,7 +2820,7 @@ void qemu_chr_set_echo(struct CharDriverState *chr, bool echo)
     }
 }
 
-void qemu_chr_guest_open(struct CharDriverState *chr)
+void qemu_chr_fe_open(struct CharDriverState *chr)
 {
     if (chr->chr_guest_open) {
         chr->chr_guest_open(chr);
diff --git a/qemu-char.h b/qemu-char.h
index c5bbaf6..8b37fcf 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -99,8 +99,8 @@ CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
                                     void (*init)(struct CharDriverState *s));
 CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*init)(struct CharDriverState *s));
 void qemu_chr_set_echo(struct CharDriverState *chr, bool echo);
-void qemu_chr_guest_open(struct CharDriverState *chr);
-void qemu_chr_guest_close(struct CharDriverState *chr);
+void qemu_chr_fe_open(struct CharDriverState *chr);
+void qemu_chr_fe_close(struct CharDriverState *chr);
 void qemu_chr_close(CharDriverState *chr);
 void qemu_chr_printf(CharDriverState *s, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
-- 
1.7.4.1

[Qemu-devel] [PATCH 10/12] char: rename qemu_chr_guest_close() -> qemu_chr_fe_close()

[Anthony Liguori]

To be more explicit about what the function is for.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio-console.c |    2 +-
 qemu-char.c         |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 21f752b..d3351c8 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -60,7 +60,7 @@ static void guest_close(VirtIOSerialPort *port)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    qemu_chr_guest_close(vcon->chr);
+    qemu_chr_fe_close(vcon->chr);
 }
 
 /* Readiness of the guest to accept data on a port */
diff --git a/qemu-char.c b/qemu-char.c
index 7e2cff0..fb7c937 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2827,7 +2827,7 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
     }
 }
 
-void qemu_chr_guest_close(struct CharDriverState *chr)
+void qemu_chr_fe_close(struct CharDriverState *chr)
 {
     if (chr->chr_guest_close) {
         chr->chr_guest_close(chr);
-- 
1.7.4.1

[Qemu-devel] [PATCH 11/12] char: make all devices do qemu_chr_fe_open()

[Anthony Liguori]

The semantics of guest_open are unreliable today because they there's no way
of knowing whether a front end will ever call open.

Let's make them reliable which means making everything call open.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 gdbstub.c               |    1 +
 hw/ccid-card-passthru.c |    1 +
 hw/debugcon.c           |    1 +
 hw/escc.c               |    1 +
 hw/etraxfs_ser.c        |    4 +++-
 hw/grlib_apbuart.c      |    1 +
 hw/ivshmem.c            |    2 ++
 hw/lm32_juart.c         |    1 +
 hw/lm32_uart.c          |    1 +
 hw/mcf_uart.c           |    1 +
 hw/milkymist-uart.c     |    1 +
 hw/pl011.c              |    1 +
 hw/pxa2xx.c             |    4 +++-
 hw/serial.c             |    2 ++
 hw/sh_serial.c          |    4 +++-
 hw/spapr_vty.c          |    1 +
 hw/strongarm.c          |    1 +
 hw/syborg_serial.c      |    1 +
 hw/usb-serial.c         |    1 +
 hw/xen_console.c        |    8 ++++++--
 hw/xilinx_uartlite.c    |    4 +++-
 monitor.c               |    2 ++
 usb-redir.c             |    1 +
 23 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 058c626..8e0459e 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2768,6 +2768,7 @@ int gdbserver_start(const char *device)
         if (!chr)
             return -1;
 
+        qemu_chr_fe_open(chr);
         qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
                               gdb_chr_event, NULL);
     }
diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index 082fd82..00468c9 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -281,6 +281,7 @@ static int passthru_initfn(CCIDCardState *base)
     card->vscard_in_hdr = 0;
     if (card->cs) {
         DPRINTF(card, D_INFO, "initing chardev\n");
+        qemu_chr_fe_open(card->cs);
         qemu_chr_add_handlers(card->cs,
             ccid_card_vscard_can_read,
             ccid_card_vscard_read,
diff --git a/hw/debugcon.c b/hw/debugcon.c
index c9ee6d9..1faa918 100644
--- a/hw/debugcon.c
+++ b/hw/debugcon.c
@@ -73,6 +73,7 @@ static void debugcon_init_core(DebugconState *s)
         exit(1);
     }
 
+    qemu_chr_fe_open(s->chr);
     qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, s);
 }
 
diff --git a/hw/escc.c b/hw/escc.c
index c1460b7..9f98bba 100644
--- a/hw/escc.c
+++ b/hw/escc.c
@@ -911,6 +911,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_fe_open(s->chn[i].chr);
             qemu_chr_add_handlers(s->chn[i].chr, serial_can_receive,
                                   serial_receive1, serial_event, &s->chn[i]);
         }
diff --git a/hw/etraxfs_ser.c b/hw/etraxfs_ser.c
index 35f8325..ccffa3e 100644
--- a/hw/etraxfs_ser.c
+++ b/hw/etraxfs_ser.c
@@ -212,10 +212,12 @@ 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)
+    if (s->chr) {
+        qemu_chr_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr,
                       serial_can_receive, serial_receive,
                       serial_event, s);
+    }
     return 0;
 }
 
diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c
index c90b810..184a0c8 100644
--- a/hw/grlib_apbuart.c
+++ b/hw/grlib_apbuart.c
@@ -149,6 +149,7 @@ static int grlib_apbuart_init(SysBusDevice *dev)
     UART *uart      = FROM_SYSBUS(typeof(*uart), dev);
     int   uart_regs = 0;
 
+    qemu_chr_fe_open(uart->chr);
     qemu_chr_add_handlers(uart->chr,
                           grlib_apbuart_can_receive,
                           grlib_apbuart_receive,
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 3055dd2..6c126eb 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -326,6 +326,7 @@ static CharDriverState* create_eventfd_chr_device(void * opaque, int eventfd,
         exit(-1);
     }
 
+    qemu_chr_fe_open(chr);
     /* if MSI is supported we need multiple interrupts */
     if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
         s->eventfd_table[vector].pdev = &s->dev;
@@ -749,6 +750,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
         s->eventfd_chr = qemu_mallocz(s->vectors * sizeof(CharDriverState *));
 
+        qemu_chr_fe_open(s->server_chr);
         qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
                      ivshmem_event, s);
     } else {
diff --git a/hw/lm32_juart.c b/hw/lm32_juart.c
index 5454aa4..c3bf70c 100644
--- a/hw/lm32_juart.c
+++ b/hw/lm32_juart.c
@@ -116,6 +116,7 @@ static int lm32_juart_init(SysBusDevice *dev)
 
     s->chr = qdev_init_chardev(&dev->qdev);
     if (s->chr) {
+        qemu_chr_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr, juart_can_rx, juart_rx, juart_event, s);
     }
 
diff --git a/hw/lm32_uart.c b/hw/lm32_uart.c
index 3678545..3dbe735 100644
--- a/hw/lm32_uart.c
+++ b/hw/lm32_uart.c
@@ -255,6 +255,7 @@ static int lm32_uart_init(SysBusDevice *dev)
 
     s->chr = qdev_init_chardev(&dev->qdev);
     if (s->chr) {
+        qemu_chr_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
     }
 
diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
index 747bc36..fcd18c7 100644
--- a/hw/mcf_uart.c
+++ b/hw/mcf_uart.c
@@ -276,6 +276,7 @@ void *mcf_uart_init(qemu_irq irq, CharDriverState *chr)
     s->chr = chr;
     s->irq = irq;
     if (chr) {
+        qemu_chr_fe_open(chr);
         qemu_chr_add_handlers(chr, mcf_uart_can_receive, mcf_uart_receive,
                               mcf_uart_event, s);
     }
diff --git a/hw/milkymist-uart.c b/hw/milkymist-uart.c
index e8e309d..0579534 100644
--- a/hw/milkymist-uart.c
+++ b/hw/milkymist-uart.c
@@ -147,6 +147,7 @@ static int milkymist_uart_init(SysBusDevice *dev)
 
     s->chr = qdev_init_chardev(&dev->qdev);
     if (s->chr) {
+        qemu_chr_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
     }
 
diff --git a/hw/pl011.c b/hw/pl011.c
index 707a161..1d99c62 100644
--- a/hw/pl011.c
+++ b/hw/pl011.c
@@ -278,6 +278,7 @@ static int pl011_init(SysBusDevice *dev, const unsigned char *id)
     s->cr = 0x300;
     s->flags = 0x90;
     if (s->chr) {
+        qemu_chr_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr, pl011_can_receive, pl011_receive,
                               pl011_event, s);
     }
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index 7516454..b562b42 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -2038,9 +2038,11 @@ 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)
+    if (chr) {
+        qemu_chr_fe_open(chr);
         qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
                         pxa2xx_fir_rx, pxa2xx_fir_event, 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 dc6d1f8..c1c2d76 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -741,6 +741,8 @@ static void serial_init_core(SerialState *s)
 
     qemu_register_reset(serial_reset, s);
 
+    qemu_chr_fe_open(s->chr);
+
     qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
                           serial_event, s);
 }
diff --git a/hw/sh_serial.c b/hw/sh_serial.c
index eabd726..3cf4df2 100644
--- a/hw/sh_serial.c
+++ b/hw/sh_serial.c
@@ -389,9 +389,11 @@ void sh_serial_init (target_phys_addr_t base, int feat,
 
     s->chr = chr;
 
-    if (chr)
+    if (chr) {
+        qemu_chr_fe_open(chr);
         qemu_chr_add_handlers(chr, sh_serial_can_receive1, sh_serial_receive1,
 			      sh_serial_event, s);
+    }
 
     s->eri = eri_source;
     s->rxi = rxi_source;
diff --git a/hw/spapr_vty.c b/hw/spapr_vty.c
index f5046d9..fbb9dcf 100644
--- a/hw/spapr_vty.c
+++ b/hw/spapr_vty.c
@@ -58,6 +58,7 @@ static int spapr_vty_init(VIOsPAPRDevice *sdev)
 {
     VIOsPAPRVTYDevice *dev = (VIOsPAPRVTYDevice *)sdev;
 
+    qemu_chr_fe_open(dev->chardev);
     qemu_chr_add_handlers(dev->chardev, vty_can_receive,
                           vty_receive, NULL, dev);
 
diff --git a/hw/strongarm.c b/hw/strongarm.c
index 50fe0c0..cd10749 100644
--- a/hw/strongarm.c
+++ b/hw/strongarm.c
@@ -1202,6 +1202,7 @@ static int strongarm_uart_init(SysBusDevice *dev)
     s->tx_timer = qemu_new_timer_ns(vm_clock, strongarm_uart_tx, s);
 
     if (s->chr) {
+        qemu_chr_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr,
                         strongarm_uart_can_receive,
                         strongarm_uart_receive,
diff --git a/hw/syborg_serial.c b/hw/syborg_serial.c
index 25c6c9d..748ec94 100644
--- a/hw/syborg_serial.c
+++ b/hw/syborg_serial.c
@@ -304,6 +304,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_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr, syborg_serial_can_receive,
                               syborg_serial_receive, syborg_serial_event, s);
     }
diff --git a/hw/usb-serial.c b/hw/usb-serial.c
index f093d18..63b5aa3 100644
--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -487,6 +487,7 @@ static int usb_serial_initfn(USBDevice *dev)
         return -1;
     }
 
+    qemu_chr_fe_open(s->cs);
     qemu_chr_add_handlers(s->cs, usb_serial_can_read, usb_serial_read,
                           usb_serial_event, s);
     usb_serial_handle_reset(dev);
diff --git a/hw/xen_console.c b/hw/xen_console.c
index e218bb8..35975a3 100644
--- a/hw/xen_console.c
+++ b/hw/xen_console.c
@@ -232,9 +232,11 @@ static int con_connect(struct XenDevice *xendev)
 	return -1;
 
     xen_be_bind_evtchn(&con->xendev);
-    if (con->chr)
+    if (con->chr) {
+        qemu_chr_fe_open(con->chr);
         qemu_chr_add_handlers(con->chr, xencons_can_receive, xencons_receive,
                               NULL, con);
+    }
 
     xen_be_printf(xendev, 1, "ring mfn %d, remote port %d, local port %d, limit %zd\n",
 		  con->ring_ref,
@@ -248,8 +250,10 @@ static void con_disconnect(struct XenDevice *xendev)
 {
     struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
 
-    if (con->chr)
+    if (con->chr) {
         qemu_chr_add_handlers(con->chr, NULL, NULL, NULL, NULL);
+        qemu_chr_fe_close(con->chr);
+    }
     xen_be_unbind_evtchn(&con->xendev);
 
     if (con->sring) {
diff --git a/hw/xilinx_uartlite.c b/hw/xilinx_uartlite.c
index 467a26c..9b40d16 100644
--- a/hw/xilinx_uartlite.c
+++ b/hw/xilinx_uartlite.c
@@ -206,8 +206,10 @@ 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)
+    if (s->chr) {
+        qemu_chr_fe_open(s->chr);
         qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
+    }
     return 0;
 }
 
diff --git a/monitor.c b/monitor.c
index 3ca211b..413ee1e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5276,6 +5276,8 @@ void monitor_init(CharDriverState *chr, int flags)
         monitor_read_command(mon, 0);
     }
 
+    qemu_chr_fe_open(chr);
+
     if (monitor_ctrl_mode(mon)) {
         mon->mc = qemu_mallocz(sizeof(MonitorControl));
         /* Control mode requires special handlers */
diff --git a/usb-redir.c b/usb-redir.c
index 606bc98..6b94161 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -807,6 +807,7 @@ static int usbredir_initfn(USBDevice *udev)
     /* We'll do the attach once we receive the speed from the usb-host */
     udev->auto_attach = 0;
 
+    qemu_chr_fe_open(dev->cs);
     qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
                           usbredir_chardev_read, usbredir_chardev_event, dev);
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 12/12] char: enforce the use of qemu_chr_guest_open()

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qemu-char.c |   10 ++++++++++
 qemu-char.h |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index fb7c937..5e62795 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -206,6 +206,8 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
     int ret;
     bool is_empty;
 
+    assert(s->fe_opened > 0);
+
     is_empty = char_queue_get_empty(&s->fe_tx);
 
     ret = char_queue_write(&s->fe_tx, buf, len);
@@ -228,6 +230,8 @@ int qemu_chr_fe_read(CharDriverState *s, uint8_t *buf, int len)
     bool is_full;
     int ret;
 
+    assert(s->fe_opened > 0);
+
     is_full = (char_queue_get_avail(&s->be_tx) == 0);
 
     ret = char_queue_read(&s->be_tx, buf, len);
@@ -249,6 +253,8 @@ void qemu_chr_fe_set_handlers(CharDriverState *s,
                               IOEventHandler *chr_event,
                               void *opaque)
 {
+    assert(s->fe_opened > 0);
+
     if (!opaque && !chr_read && !chr_write && !chr_event) {
         /* chr driver being released. */
         ++s->avail_connections;
@@ -2822,6 +2828,8 @@ void qemu_chr_set_echo(struct CharDriverState *chr, bool echo)
 
 void qemu_chr_fe_open(struct CharDriverState *chr)
 {
+    chr->fe_opened++;
+
     if (chr->chr_guest_open) {
         chr->chr_guest_open(chr);
     }
@@ -2832,6 +2840,8 @@ void qemu_chr_fe_close(struct CharDriverState *chr)
     if (chr->chr_guest_close) {
         chr->chr_guest_close(chr);
     }
+
+    chr->fe_opened--;
 }
 
 void qemu_chr_close(CharDriverState *chr)
diff --git a/qemu-char.h b/qemu-char.h
index 8b37fcf..b910c5e 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -88,6 +88,8 @@ struct CharDriverState {
     int opened;
     int avail_connections;
 
+    int fe_opened;
+
     CharQueue fe_tx;
     CharQueue be_tx;
 
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 12/12] char: enforce the use of qemu_chr_guest_open()

[Alon Levy]

On Mon, Aug 01, 2011 at 09:23:10AM -0500, Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Title should be "fe_open", not guest_open.

> ---
>  qemu-char.c |   10 ++++++++++
>  qemu-char.h |    2 ++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index fb7c937..5e62795 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -206,6 +206,8 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
>      int ret;
>      bool is_empty;
>  
> +    assert(s->fe_opened > 0);
> +
>      is_empty = char_queue_get_empty(&s->fe_tx);
>  
>      ret = char_queue_write(&s->fe_tx, buf, len);
> @@ -228,6 +230,8 @@ int qemu_chr_fe_read(CharDriverState *s, uint8_t *buf, int len)
>      bool is_full;
>      int ret;
>  
> +    assert(s->fe_opened > 0);
> +
>      is_full = (char_queue_get_avail(&s->be_tx) == 0);
>  
>      ret = char_queue_read(&s->be_tx, buf, len);
> @@ -249,6 +253,8 @@ void qemu_chr_fe_set_handlers(CharDriverState *s,
>                                IOEventHandler *chr_event,
>                                void *opaque)
>  {
> +    assert(s->fe_opened > 0);
> +
>      if (!opaque && !chr_read && !chr_write && !chr_event) {
>          /* chr driver being released. */
>          ++s->avail_connections;
> @@ -2822,6 +2828,8 @@ void qemu_chr_set_echo(struct CharDriverState *chr, bool echo)
>  
>  void qemu_chr_fe_open(struct CharDriverState *chr)
>  {
> +    chr->fe_opened++;
> +
>      if (chr->chr_guest_open) {
>          chr->chr_guest_open(chr);
>      }
> @@ -2832,6 +2840,8 @@ void qemu_chr_fe_close(struct CharDriverState *chr)
>      if (chr->chr_guest_close) {
>          chr->chr_guest_close(chr);
>      }
> +
> +    chr->fe_opened--;
>  }
>  
>  void qemu_chr_close(CharDriverState *chr)
> diff --git a/qemu-char.h b/qemu-char.h
> index 8b37fcf..b910c5e 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -88,6 +88,8 @@ struct CharDriverState {
>      int opened;
>      int avail_connections;
>  
> +    int fe_opened;
> +
>      CharQueue fe_tx;
>      CharQueue be_tx;
>  
> -- 
> 1.7.4.1
> 
> 

Re: [Qemu-devel] [PATCH 12/12] char: enforce the use of qemu_chr_guest_open()

[Anthony Liguori]

On 08/01/2011 10:38 AM, Alon Levy wrote:
> On Mon, Aug 01, 2011 at 09:23:10AM -0500, Anthony Liguori wrote:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> Title should be "fe_open", not guest_open.

Indeed, thanks.

Regards,

Anthony Liguori

>
>> ---
>>   qemu-char.c |   10 ++++++++++
>>   qemu-char.h |    2 ++
>>   2 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index fb7c937..5e62795 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -206,6 +206,8 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
>>       int ret;
>>       bool is_empty;
>>
>> +    assert(s->fe_opened>  0);
>> +
>>       is_empty = char_queue_get_empty(&s->fe_tx);
>>
>>       ret = char_queue_write(&s->fe_tx, buf, len);
>> @@ -228,6 +230,8 @@ int qemu_chr_fe_read(CharDriverState *s, uint8_t *buf, int len)
>>       bool is_full;
>>       int ret;
>>
>> +    assert(s->fe_opened>  0);
>> +
>>       is_full = (char_queue_get_avail(&s->be_tx) == 0);
>>
>>       ret = char_queue_read(&s->be_tx, buf, len);
>> @@ -249,6 +253,8 @@ void qemu_chr_fe_set_handlers(CharDriverState *s,
>>                                 IOEventHandler *chr_event,
>>                                 void *opaque)
>>   {
>> +    assert(s->fe_opened>  0);
>> +
>>       if (!opaque&&  !chr_read&&  !chr_write&&  !chr_event) {
>>           /* chr driver being released. */
>>           ++s->avail_connections;
>> @@ -2822,6 +2828,8 @@ void qemu_chr_set_echo(struct CharDriverState *chr, bool echo)
>>
>>   void qemu_chr_fe_open(struct CharDriverState *chr)
>>   {
>> +    chr->fe_opened++;
>> +
>>       if (chr->chr_guest_open) {
>>           chr->chr_guest_open(chr);
>>       }
>> @@ -2832,6 +2840,8 @@ void qemu_chr_fe_close(struct CharDriverState *chr)
>>       if (chr->chr_guest_close) {
>>           chr->chr_guest_close(chr);
>>       }
>> +
>> +    chr->fe_opened--;
>>   }
>>
>>   void qemu_chr_close(CharDriverState *chr)
>> diff --git a/qemu-char.h b/qemu-char.h
>> index 8b37fcf..b910c5e 100644
>> --- a/qemu-char.h
>> +++ b/qemu-char.h
>> @@ -88,6 +88,8 @@ struct CharDriverState {
>>       int opened;
>>       int avail_connections;
>>
>> +    int fe_opened;
>> +
>>       CharQueue fe_tx;
>>       CharQueue be_tx;
>>
>> --
>> 1.7.4.1
>>
>>
>

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Alon Levy]

On Mon, Aug 01, 2011 at 09:22:58AM -0500, Anthony Liguori wrote:
> The char layer has been growing some nasty warts for some time now as we ask it
> to do things it was never intended on doing.  It's been long over due for an
> overhaul and its become evident to me that we need to address this first before
> adding any more features to the char layer.
> 
> This series is the start at sanitizing the char layer.  It effectively turns
> the char layer into an internal pipe.  It supports flow control using an
> intermediate ring queue for each direction.
> 
> This series is an RFC because I don't think we should merge the series until we
> completely convert the old style flow control users to the new style.
> 
> One particularly nasty area is the mux device.  I'm not entirely sure yet how
> to preceed there.
> 
> 

So, adding a copy - is that really a good idea? I don't have any alternative code,
so I'm already starting bad, I know, and I understand the want to have a "middle
ground" to ease the logic. Maybe keeping an iovec? add a function on each side for
freeing, i.e. release_be_buffer, release_fe_buffer. At least it could make this as
fast as the current code. I'm thinking of copy/paste for vdagent, usbredir, guest
agent doing dmesg or anything larger.

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Anthony Liguori]

On 08/01/2011 11:04 AM, Alon Levy wrote:
> On Mon, Aug 01, 2011 at 09:22:58AM -0500, Anthony Liguori wrote:
>> The char layer has been growing some nasty warts for some time now as we ask it
>> to do things it was never intended on doing.  It's been long over due for an
>> overhaul and its become evident to me that we need to address this first before
>> adding any more features to the char layer.
>>
>> This series is the start at sanitizing the char layer.  It effectively turns
>> the char layer into an internal pipe.  It supports flow control using an
>> intermediate ring queue for each direction.
>>
>> This series is an RFC because I don't think we should merge the series until we
>> completely convert the old style flow control users to the new style.
>>
>> One particularly nasty area is the mux device.  I'm not entirely sure yet how
>> to preceed there.
>>
>>
>
> So, adding a copy - is that really a good idea?

Yes, otherwise I wouldn't have proposed it ;-)

> I don't have any alternative code,
> so I'm already starting bad, I know, and I understand the want to have a "middle
> ground" to ease the logic.

You need an intermediate buffer if you want to preserve a simple 
read/write interface.

> Maybe keeping an iovec? add a function on each side for
> freeing, i.e. release_be_buffer, release_fe_buffer. At least it could make this as
> fast as the current code. I'm thinking of copy/paste for vdagent, usbredir, guest
> agent doing dmesg or anything larger.

I think you're prematurely optimizing.

I think you'll find that there are many other bottlenecks well before 
this copy becomes an issue.

Regardless, correctness needs to trump performance here.  We ought to 
focus on making the layer lossless and sane first, and then we can work 
on improving performance.

Regards,

Anthony Liguori

>
>

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Hans de Goede]

Hi,

On 08/01/2011 04:22 PM, Anthony Liguori wrote:
> The char layer has been growing some nasty warts for some time now as we ask it
> to do things it was never intended on doing.  It's been long over due for an
> overhaul and its become evident to me that we need to address this first before
> adding any more features to the char layer.
>
> This series is the start at sanitizing the char layer.  It effectively turns
> the char layer into an internal pipe.  It supports flow control using an
> intermediate ring queue for each direction.
>
> This series is an RFC because I don't think we should merge the series until we
> completely convert the old style flow control users to the new style.
>
> One particularly nasty area is the mux device.  I'm not entirely sure yet how
> to preceed there.
>

Anthony, thanks for looking into this / cooking up a patchset for this.
Unfortunately I don't have the time to look into right now. Likely I won't
have any time for this until after kvm forum. But if no-one has given this
a serious look by then I'll try to convert the various spice related
chardev frontends / backends to this and run some tests.

Regards,

Hans

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Blue Swirl]

On Mon, Aug 1, 2011 at 2:22 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> The char layer has been growing some nasty warts for some time now as we ask it
> to do things it was never intended on doing.  It's been long over due for an
> overhaul and its become evident to me that we need to address this first before
> adding any more features to the char layer.
>
> This series is the start at sanitizing the char layer.  It effectively turns
> the char layer into an internal pipe.  It supports flow control using an
> intermediate ring queue for each direction.
>
> This series is an RFC because I don't think we should merge the series until we
> completely convert the old style flow control users to the new style.

The terms 'back-end' and 'front-end' could be improved. How about just
'device' or 'hw' and 'chrdev'?

The architecture could be described in for example qemu-tech.texi.

> One particularly nasty area is the mux device.  I'm not entirely sure yet how
> to preceed there.
>
>
>

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Anthony Liguori]

On 08/01/2011 04:54 PM, Blue Swirl wrote:
> On Mon, Aug 1, 2011 at 2:22 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> The char layer has been growing some nasty warts for some time now as we ask it
>> to do things it was never intended on doing.  It's been long over due for an
>> overhaul and its become evident to me that we need to address this first before
>> adding any more features to the char layer.
>>
>> This series is the start at sanitizing the char layer.  It effectively turns
>> the char layer into an internal pipe.  It supports flow control using an
>> intermediate ring queue for each direction.
>>
>> This series is an RFC because I don't think we should merge the series until we
>> completely convert the old style flow control users to the new style.
>
> The terms 'back-end' and 'front-end' could be improved. How about just
> 'device' or 'hw' and 'chrdev'?

It's all temporary as there currently is very little asymmetry. 
Historically, the biggest source of asymmetry was the fact that the 
back-end -> front-end path had flow control and the opposite direction 
didn't.

This series fixes that.  The only remaining asymmetry is the ioctl() 
call.  I think if we change the semantics of qemu_chr_event() though to 
have a return value and a data parameter, I think we can fold ioctl into 
event and ultimately fix that asymmetry.

Once that's done, there's no longer a distinction between front-end and 
back-end.  That makes CharDriverState act as basically a socketpair().

>
> The architecture could be described in for example qemu-tech.texi.

I'll include something for docs/ for the next submission.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Amit Shah]

On (Mon) 01 Aug 2011 [09:22:58], Anthony Liguori wrote:
> The char layer has been growing some nasty warts for some time now as we ask it
> to do things it was never intended on doing.  It's been long over due for an
> overhaul and its become evident to me that we need to address this first before
> adding any more features to the char layer.
> 
> This series is the start at sanitizing the char layer.  It effectively turns
> the char layer into an internal pipe.  It supports flow control using an
> intermediate ring queue for each direction.

Let's break down what we have now:

* chardev -> guest (backend writes):
   we have connect/disconnect notifications, we have an ugly
   can_read() implementation that is executed each time iohandlers are
   run.  However, it gives us reasonable flow control.

* guest -> chardev (frontend writes):
   we don't get chardev connect/disconnect events, neither do we get
   to know if the chardev is overwhelmed with data and to stop sending
   any more till it has some room in its queue.  This is because we
   need poll semantics instead of select to get POLLIN and POLLOUT
   events, using which we can ascertain what state the chardev is in.
   There's no call corresponding to the existing qemu_chr_can_read(),
   which essentially confirms if a chardev is able to handle data for
   output.  This series only adds a qemu_char_fe_can_write(), doesn't
   add callbacks for connect/disconnect events since that depends on
   polling.

The problem I think with adding a buffer is it just solves the flow
control problem without solving the connect/disconnect events issue by
just switching to poll, and solving all the problems at once.  Is
there something that we solve by having a buffer in addition to
poll()?

Also, introducing a buffer doesn't really solve all the problems; just
shifts them into the char layer instead of letting the OS handle the
queues naturally.

		Amit

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Anthony Liguori]

On 08/04/2011 01:45 AM, Amit Shah wrote:
> On (Mon) 01 Aug 2011 [09:22:58], Anthony Liguori wrote:
>> The char layer has been growing some nasty warts for some time now as we ask it
>> to do things it was never intended on doing.  It's been long over due for an
>> overhaul and its become evident to me that we need to address this first before
>> adding any more features to the char layer.
>>
>> This series is the start at sanitizing the char layer.  It effectively turns
>> the char layer into an internal pipe.  It supports flow control using an
>> intermediate ring queue for each direction.
>
> Let's break down what we have now:
>
> * chardev ->  guest (backend writes):
>     we have connect/disconnect notifications, we have an ugly
>     can_read() implementation that is executed each time iohandlers are
>     run.  However, it gives us reasonable flow control.

It has one major flaw.  It assumes that the backends can implement 
polling to determine when the front end can read.

This makes it impossible to implement a backend that isn't associated 
with a file descriptor (like a QMP server as required by the guest agent).

> * guest ->  chardev (frontend writes):
>     we don't get chardev connect/disconnect events, neither do we get
>     to know if the chardev is overwhelmed with data and to stop sending
>     any more till it has some room in its queue.

We do have connect/disconnect event--that's open/close.


>     This is because we
>     need poll semantics instead of select to get POLLIN and POLLOUT
>     events, using which we can ascertain what state the chardev is in.
>     There's no call corresponding to the existing qemu_chr_can_read(),
>     which essentially confirms if a chardev is able to handle data for
>     output.  This series only adds a qemu_char_fe_can_write(), doesn't
>     add callbacks for connect/disconnect events since that depends on
>     polling.

There are already events for connect/disconnect so I'm not sure what 
you're talking about.

>
> The problem I think with adding a buffer is it just solves the flow
> control problem without solving the connect/disconnect events issue by
> just switching to poll,

I don't understand at all what you're saying here :-)

Hopefully you'd agree, that from a design perspective, the closer a 
chrdrv looks to a normal unix socket, the more correct it is.

After this series, we have:

1) read/write methods that behave like unix read/write (except zero 
indicates EAGAIN, not EOF).

2) OPENED/CLOSE events that map to accept/EOF

3) edge events for readability that semantically map to epoll()

I think that's pretty complete.  I don't see anything that's missing.

> and solving all the problems at once.  Is
> there something that we solve by having a buffer in addition to
> poll()?
>
> Also, introducing a buffer doesn't really solve all the problems; just
> shifts them into the char layer instead of letting the OS handle the
> queues naturally.

What does "naturally" mean?

Regards,

Anthony Liguori

>
> 		Amit

Re: [Qemu-devel] [PATCH 00/12][RFC] char: add flow control and fix guest_[open|close]

[Amit Shah]

(Adding Gerd to cc)

On (Thu) 04 Aug 2011 [08:11:23], Anthony Liguori wrote:
> On 08/04/2011 01:45 AM, Amit Shah wrote:
> >On (Mon) 01 Aug 2011 [09:22:58], Anthony Liguori wrote:
> >>The char layer has been growing some nasty warts for some time now as we ask it
> >>to do things it was never intended on doing.  It's been long over due for an
> >>overhaul and its become evident to me that we need to address this first before
> >>adding any more features to the char layer.
> >>
> >>This series is the start at sanitizing the char layer.  It effectively turns
> >>the char layer into an internal pipe.  It supports flow control using an
> >>intermediate ring queue for each direction.
> >
> >Let's break down what we have now:
> >
> >* chardev ->  guest (backend writes):
> >    we have connect/disconnect notifications, we have an ugly
> >    can_read() implementation that is executed each time iohandlers are
> >    run.  However, it gives us reasonable flow control.
> 
> It has one major flaw.  It assumes that the backends can implement
> polling to determine when the front end can read.
> 
> This makes it impossible to implement a backend that isn't
> associated with a file descriptor (like a QMP server as required by
> the guest agent).

OK; is a ring the best way to get these 'into the fold'?  How about a
separate, qemu-specific poll() for such code?  Does glib have any
support for this?  Should we look at extending glib with such library
code instead?

> >* guest ->  chardev (frontend writes):
> >    we don't get chardev connect/disconnect events, neither do we get
> >    to know if the chardev is overwhelmed with data and to stop sending
> >    any more till it has some room in its queue.
> 
> We do have connect/disconnect event--that's open/close.

chardev connect/disconnect events right now aren't useful.  Eg., a tcp
disconnect or a unix disconnect is only noticed when the socket gets
re-connected.  And this is because select() can't give us POLLHUP.

> >    This is because we
> >    need poll semantics instead of select to get POLLIN and POLLOUT
> >    events, using which we can ascertain what state the chardev is in.
> >    There's no call corresponding to the existing qemu_chr_can_read(),
> >    which essentially confirms if a chardev is able to handle data for
> >    output.  This series only adds a qemu_char_fe_can_write(), doesn't
> >    add callbacks for connect/disconnect events since that depends on
> >    polling.
> 
> There are already events for connect/disconnect so I'm not sure what
> you're talking about.

(see above)

> >The problem I think with adding a buffer is it just solves the flow
> >control problem without solving the connect/disconnect events issue by
> >just switching to poll,
> 
> I don't understand at all what you're saying here :-)
> 
> Hopefully you'd agree, that from a design perspective, the closer a
> chrdrv looks to a normal unix socket, the more correct it is.

What I wanted to say there, without knowing about the special in-qemu
QMP server usage, is: we already have an fd for host-side chardevs, so
introducing a ring isn't beneficial.

For the frontend (as well as to take care of the QMP server case you
cite), we can have something like a QEMUFD, that supports open, read,
write, close and poll() semantics.  Getting edge notifications from
frontends shouldn't really be a problem, and all of this can be tied
into the main loop.

Is that workable?  Removes the need for a ring for sure.

> After this series, we have:
> 
> 1) read/write methods that behave like unix read/write (except zero
> indicates EAGAIN, not EOF).
> 
> 2) OPENED/CLOSE events that map to accept/EOF
> 
> 3) edge events for readability that semantically map to epoll()
> 
> I think that's pretty complete.  I don't see anything that's missing.

(keeping for context)

		Amit