[Qemu-devel] [PULL 41/43] qemu-char: make writes thread-safe

[Luiz Capitulino]

From: Paolo Bonzini <address@hidden>

This will let threads other than the I/O thread raise QMP events.

GIOChannel is thread-safe, and send and receive state is usually
well-separated.  The only driver that requires some care is the
pty driver, where some of the state is shared by the read and write
sides.  That state is protected with the chr_write_lock too.

Reviewed-by: Fam Zheng <address@hidden>
Signed-off-by: Paolo Bonzini <address@hidden>
Signed-off-by: Luiz Capitulino <address@hidden>
---
 include/sysemu/char.h | 11 ++++++----
 qemu-char.c           | 58 ++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index bd2b62d..c8b15f9 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -54,6 +54,7 @@ typedef struct {
 typedef void IOEventHandler(void *opaque, int event);
 
 struct CharDriverState {
+    QemuMutex chr_write_lock;
     void (*init)(struct CharDriverState *s);
     int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
     int (*chr_sync_read)(struct CharDriverState *s,
@@ -164,6 +165,7 @@ void qemu_chr_fe_event(CharDriverState *s, int event);
  * @qemu_chr_fe_printf:
  *
  * Write to a character backend using a printf style interface.
+ * This function is thread-safe.
  *
  * @fmt see #printf
  */
@@ -176,8 +178,9 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition 
cond,
 /**
  * @qemu_chr_fe_write:
  *
- * Write data to a character backend from the front end.  This function will
- * send data from the front end to the back end.
+ * Write data to a character backend from the front end.  This function
+ * will send data from the front end to the back end.  This function
+ * is thread-safe.
  *
  * @buf the data
  * @len the number of bytes to send
@@ -192,7 +195,7 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t 
*buf, int len);
  * Write data to a character backend from the front end.  This function will
  * send data from the front end to the back end.  Unlike @qemu_chr_fe_write,
  * this function will block if the back end cannot consume all of the data
- * attempted to be written.
+ * attempted to be written.  This function is thread-safe.
  *
  * @buf the data
  * @len the number of bytes to send
@@ -216,7 +219,7 @@ int qemu_chr_fe_read_all(CharDriverState *s, uint8_t *buf, 
int len);
 /**
  * @qemu_chr_fe_ioctl:
  *
- * Issue a device specific ioctl to a backend.
+ * Issue a device specific ioctl to a backend.  This function is thread-safe.
  *
  * @cmd see CHR_IOCTL_*
  * @arg the data associated with @cmd
diff --git a/qemu-char.c b/qemu-char.c
index 9961b02..a9025e6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -121,7 +121,12 @@ void qemu_chr_be_generic_open(CharDriverState *s)
 
 int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
 {
-    return s->chr_write(s, buf, len);
+    int ret;
+
+    qemu_mutex_lock(&s->chr_write_lock);
+    ret = s->chr_write(s, buf, len);
+    qemu_mutex_unlock(&s->chr_write_lock);
+    return ret;
 }
 
 int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len)
@@ -129,6 +134,7 @@ int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t 
*buf, int len)
     int offset = 0;
     int res;
 
+    qemu_mutex_lock(&s->chr_write_lock);
     while (offset < len) {
         do {
             res = s->chr_write(s, buf + offset, len - offset);
@@ -137,17 +143,17 @@ int qemu_chr_fe_write_all(CharDriverState *s, const 
uint8_t *buf, int len)
             }
         } while (res == -1 && errno == EAGAIN);
 
-        if (res == 0) {
+        if (res <= 0) {
             break;
         }
 
-        if (res < 0) {
-            return res;
-        }
-
         offset += res;
     }
+    qemu_mutex_unlock(&s->chr_write_lock);
 
+    if (res < 0) {
+        return res;
+    }
     return offset;
 }
 
@@ -315,11 +321,14 @@ typedef struct {
     int prod[MAX_MUX];
     int cons[MAX_MUX];
     int timestamps;
+
+    /* Protected by the CharDriverState chr_write_lock.  */
     int linestart;
     int64_t timestamps_start;
 } MuxDriver;
 
 
+/* Called with chr_write_lock held.  */
 static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     MuxDriver *d = chr->opaque;
@@ -865,6 +874,7 @@ typedef struct FDCharDriver {
     QTAILQ_ENTRY(FDCharDriver) node;
 } FDCharDriver;
 
+/* Called with chr_write_lock held.  */
 static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     FDCharDriver *s = chr->opaque;
@@ -1064,12 +1074,14 @@ static CharDriverState 
*qemu_chr_open_stdio(ChardevStdio *opts)
 
 typedef struct {
     GIOChannel *fd;
-    int connected;
     int read_bytes;
+
+    /* Protected by the CharDriverState chr_write_lock.  */
+    int connected;
     guint timer_tag;
 } PtyCharDriver;
 
-static void pty_chr_update_read_handler(CharDriverState *chr);
+static void pty_chr_update_read_handler_locked(CharDriverState *chr);
 static void pty_chr_state(CharDriverState *chr, int connected);
 
 static gboolean pty_chr_timer(gpointer opaque)
@@ -1077,14 +1089,17 @@ static gboolean pty_chr_timer(gpointer opaque)
     struct CharDriverState *chr = opaque;
     PtyCharDriver *s = chr->opaque;
 
+    qemu_mutex_lock(&chr->chr_write_lock);
     s->timer_tag = 0;
     if (!s->connected) {
         /* Next poll ... */
-        pty_chr_update_read_handler(chr);
+        pty_chr_update_read_handler_locked(chr);
     }
+    qemu_mutex_unlock(&chr->chr_write_lock);
     return FALSE;
 }
 
+/* Called with chr_write_lock held.  */
 static void pty_chr_rearm_timer(CharDriverState *chr, int ms)
 {
     PtyCharDriver *s = chr->opaque;
@@ -1101,7 +1116,8 @@ static void pty_chr_rearm_timer(CharDriverState *chr, int 
ms)
     }
 }
 
-static void pty_chr_update_read_handler(CharDriverState *chr)
+/* Called with chr_write_lock held.  */
+static void pty_chr_update_read_handler_locked(CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
     GPollFD pfd;
@@ -1117,13 +1133,21 @@ static void pty_chr_update_read_handler(CharDriverState 
*chr)
     }
 }
 
+static void pty_chr_update_read_handler(CharDriverState *chr)
+{
+    qemu_mutex_lock(&chr->chr_write_lock);
+    pty_chr_update_read_handler_locked(chr);
+    qemu_mutex_unlock(&chr->chr_write_lock);
+}
+
+/* Called with chr_write_lock held.  */
 static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     PtyCharDriver *s = chr->opaque;
 
     if (!s->connected) {
         /* guest sends data, check for (re-)connect */
-        pty_chr_update_read_handler(chr);
+        pty_chr_update_read_handler_locked(chr);
         return 0;
     }
     return io_channel_send(s->fd, buf, len);
@@ -1169,6 +1193,7 @@ static gboolean pty_chr_read(GIOChannel *chan, 
GIOCondition cond, void *opaque)
     return TRUE;
 }
 
+/* Called with chr_write_lock held.  */
 static void pty_chr_state(CharDriverState *chr, int connected)
 {
     PtyCharDriver *s = chr->opaque;
@@ -1659,9 +1684,12 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
 typedef struct {
     int max_size;
     HANDLE hcom, hrecv, hsend;
-    OVERLAPPED orecv, osend;
+    OVERLAPPED orecv;
     BOOL fpipe;
     DWORD len;
+
+    /* Protected by the CharDriverState chr_write_lock.  */
+    OVERLAPPED osend;
 } WinCharState;
 
 typedef struct {
@@ -1771,6 +1799,7 @@ static int win_chr_init(CharDriverState *chr, const char 
*filename)
     return -1;
 }
 
+/* Called with chr_write_lock held.  */
 static int win_chr_write(CharDriverState *chr, const uint8_t *buf, int len1)
 {
     WinCharState *s = chr->opaque;
@@ -2213,6 +2242,7 @@ typedef struct {
     int max_size;
 } NetCharDriver;
 
+/* Called with chr_write_lock held.  */
 static int udp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     NetCharDriver *s = chr->opaque;
@@ -2403,6 +2433,7 @@ static int unix_send_msgfds(CharDriverState *chr, const 
uint8_t *buf, int len)
 }
 #endif
 
+/* Called with chr_write_lock held.  */
 static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     TCPCharDriver *s = chr->opaque;
@@ -3000,6 +3031,7 @@ static size_t ringbuf_count(const CharDriverState *chr)
     return d->prod - d->cons;
 }
 
+/* Called with chr_write_lock held.  */
 static int ringbuf_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     RingBufCharDriver *d = chr->opaque;
@@ -3024,9 +3056,11 @@ static int ringbuf_chr_read(CharDriverState *chr, 
uint8_t *buf, int len)
     RingBufCharDriver *d = chr->opaque;
     int i;
 
+    qemu_mutex_lock(&chr->chr_write_lock);
     for (i = 0; i < len && d->cons != d->prod; i++) {
         buf[i] = d->cbuf[d->cons++ & (d->size - 1)];
     }
+    qemu_mutex_unlock(&chr->chr_write_lock);
 
     return i;
 }
-- 
1.9.3