[Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe

[Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe

[Paolo Bonzini]

Even though virtio-blk-dataplane mostly synchronizes with the block layer
by means of the AioContext, we still need to introduce mutexes for other
QEMU subsystems that the dataplane thread might encounter on its way.
Adding rerror/werror support, for example, means that the dataplane
thread will have to generate QMP events.

monitor_puts is the entry point for generating QMP responses and events.
Making it thread-safe lets virtio-blk-dataplane threads generate QMP
events; because the same entry point is also used for responses, a
response and an event will never be intertwined.

Protection is inserted at both the qemu-char and monitor levels.
A generic mutex is necessary in qemu_fe_chr_write so that
qemu_chr_fe_write_all does not break its output; we reuse that
mutex in some of the character devices.

There is no need to protect against removal of the monitor's backend,
since the monitor itself cannot be removed.

Paolo Bonzini (6):
  qemu-char: introduce qemu_chr_alloc
  qemu-char: do not call chr_write directly
  qemu-char: move pty_chr_update_read_handler around
  qemu-char: make writes thread-safe
  monitor: protect outbuf with mutex
  monitor: protect event emission

 backends/baum.c       |   2 +-
 backends/msmouse.c    |   2 +-
 include/sysemu/char.h |  20 ++++++--
 monitor.c             |  55 ++++++++++++++++++----
 qemu-char.c           | 125 +++++++++++++++++++++++++++++++++-----------------
 spice-qemu-char.c     |   2 +-
 ui/console.c          |   2 +-
 7 files changed, 149 insertions(+), 59 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/6] qemu-char: introduce qemu_chr_alloc

[Paolo Bonzini]

The next patch will modify this function to initialize state that is
common to all backends.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 backends/baum.c       |  2 +-
 backends/msmouse.c    |  2 +-
 include/sysemu/char.h |  9 +++++++++
 qemu-char.c           | 32 +++++++++++++++++++-------------
 spice-qemu-char.c     |  2 +-
 ui/console.c          |  2 +-
 6 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/backends/baum.c b/backends/baum.c
index 759003f..796512d 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -574,7 +574,7 @@ CharDriverState *chr_baum_init(void)
     int tty;
 
     baum = g_malloc0(sizeof(BaumDriverState));
-    baum->chr = chr = g_malloc0(sizeof(CharDriverState));
+    baum->chr = chr = qemu_chr_alloc();
 
     chr->opaque = baum;
     chr->chr_write = baum_write;
diff --git a/backends/msmouse.c b/backends/msmouse.c
index c0dbfcd..650a531 100644
--- a/backends/msmouse.c
+++ b/backends/msmouse.c
@@ -67,7 +67,7 @@ CharDriverState *qemu_chr_open_msmouse(void)
 {
     CharDriverState *chr;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     chr->chr_write = msmouse_chr_write;
     chr->chr_close = msmouse_chr_close;
     chr->explicit_be_open = true;
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index b81a6ff..9bbfd06 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -85,6 +85,15 @@ struct CharDriverState {
 };
 
 /**
+ * @qemu_chr_alloc:
+ *
+ * Allocate and initialize a new CharDriverState.
+ *
+ * Returns: a newly allocated CharDriverState.
+ */
+CharDriverState *qemu_chr_alloc(void);
+
+/**
  * @qemu_chr_new_from_opts:
  *
  * Create a new character backend from a QemuOpts list.
diff --git a/qemu-char.c b/qemu-char.c
index 54ed244..3df5db7 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -91,6 +91,12 @@
 static QTAILQ_HEAD(CharDriverStateHead, CharDriverState) chardevs =
     QTAILQ_HEAD_INITIALIZER(chardevs);
 
+CharDriverState *qemu_chr_alloc(void)
+{
+    CharDriverState *chr = g_malloc0(sizeof(CharDriverState));
+    return chr;
+}
+
 void qemu_chr_be_event(CharDriverState *s, int event)
 {
     /* Keep track if the char device is open */
@@ -236,7 +242,7 @@ static CharDriverState *qemu_chr_open_null(void)
 {
     CharDriverState *chr;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     chr->chr_write = null_chr_write;
     chr->explicit_be_open = true;
     return chr;
@@ -524,7 +530,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
     CharDriverState *chr;
     MuxDriver *d;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     d = g_malloc0(sizeof(MuxDriver));
 
     chr->opaque = d;
@@ -899,7 +905,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
     CharDriverState *chr;
     FDCharDriver *s;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(FDCharDriver));
     s->fd_in = io_channel_from_fd(fd_in);
     s->fd_out = io_channel_from_fd(fd_out);
@@ -1176,7 +1182,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
 
     close(slave_fd);
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
 
     chr->filename = g_strdup_printf("pty:%s", pty_name);
     ret->pty = g_strdup(pty_name);
@@ -1538,7 +1544,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
     drv->fd = fd;
     drv->mode = IEEE1284_MODE_COMPAT;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     chr->chr_write = null_chr_write;
     chr->chr_ioctl = pp_ioctl;
     chr->chr_close = pp_close;
@@ -1593,7 +1599,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
 {
     CharDriverState *chr;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     chr->opaque = (void *)(intptr_t)fd;
     chr->chr_write = null_chr_write;
     chr->chr_ioctl = pp_ioctl;
@@ -1817,7 +1823,7 @@ static CharDriverState *qemu_chr_open_win_path(const char *filename)
     CharDriverState *chr;
     WinCharState *s;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(WinCharState));
     chr->opaque = s;
     chr->chr_write = win_chr_write;
@@ -1916,7 +1922,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
     CharDriverState *chr;
     WinCharState *s;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(WinCharState));
     chr->opaque = s;
     chr->chr_write = win_chr_write;
@@ -1935,7 +1941,7 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
     CharDriverState *chr;
     WinCharState *s;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(WinCharState));
     s->hcom = fd_out;
     chr->opaque = s;
@@ -2091,7 +2097,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
     DWORD              dwMode;
     int                is_console = 0;
 
-    chr   = g_malloc0(sizeof(CharDriverState));
+    chr   = qemu_chr_alloc();
     stdio = g_malloc0(sizeof(WinStdioCharState));
 
     stdio->hStdIn = GetStdHandle(STD_INPUT_HANDLE);
@@ -2253,7 +2259,7 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
     CharDriverState *chr = NULL;
     NetCharDriver *s = NULL;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(NetCharDriver));
 
     s->fd = fd;
@@ -2637,7 +2643,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
         return NULL;
     }
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(TCPCharDriver));
 
     s->connected = 0;
@@ -2822,7 +2828,7 @@ static CharDriverState *qemu_chr_open_ringbuf(ChardevRingbuf *opts,
     CharDriverState *chr;
     RingBufCharDriver *d;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     d = g_malloc(sizeof(*d));
 
     d->size = opts->has_size ? opts->size : 65536;
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 6624559..4518a4d 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -268,7 +268,7 @@ static CharDriverState *chr_open(const char *subtype,
     CharDriverState *chr;
     SpiceCharDriver *s;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(SpiceCharDriver));
     s->chr = chr;
     s->active = false;
diff --git a/ui/console.c b/ui/console.c
index e057755..c893b27 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1795,7 +1795,7 @@ static CharDriverState *text_console_init(ChardevVC *vc)
     unsigned width = 0;
     unsigned height = 0;
 
-    chr = g_malloc0(sizeof(CharDriverState));
+    chr = qemu_chr_alloc();
 
     if (vc->has_width) {
         width = vc->width;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/6] qemu-char: do not call chr_write directly

[Paolo Bonzini]

Make the mux always go through qemu_chr_fe_write, so that we'll get
the mutex for the underlying chardev.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 3df5db7..2bda2fb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -279,7 +279,7 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
     MuxDriver *d = chr->opaque;
     int ret;
     if (!d->timestamps) {
-        ret = d->drv->chr_write(d->drv, buf, len);
+        ret = qemu_chr_fe_write(d->drv, buf, len);
     } else {
         int i;
 
@@ -301,10 +301,10 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
                          (secs / 60) % 60,
                          secs % 60,
                          (int)(ti % 1000));
-                d->drv->chr_write(d->drv, (uint8_t *)buf1, strlen(buf1));
+                qemu_chr_fe_write(d->drv, (uint8_t *)buf1, strlen(buf1));
                 d->linestart = 0;
             }
-            ret += d->drv->chr_write(d->drv, buf+i, 1);
+            ret += qemu_chr_fe_write(d->drv, buf+i, 1);
             if (buf[i] == '\n') {
                 d->linestart = 1;
             }
@@ -339,13 +339,13 @@ static void mux_print_help(CharDriverState *chr)
                  "\n\rEscape-Char set to Ascii: 0x%02x\n\r\n\r",
                  term_escape_char);
     }
-    chr->chr_write(chr, (uint8_t *)cbuf, strlen(cbuf));
+    qemu_chr_fe_write(chr, (uint8_t *)cbuf, strlen(cbuf));
     for (i = 0; mux_help[i] != NULL; i++) {
         for (j=0; mux_help[i][j] != '\0'; j++) {
             if (mux_help[i][j] == '%')
-                chr->chr_write(chr, (uint8_t *)ebuf, strlen(ebuf));
+                qemu_chr_fe_write(chr, (uint8_t *)ebuf, strlen(ebuf));
             else
-                chr->chr_write(chr, (uint8_t *)&mux_help[i][j], 1);
+                qemu_chr_fe_write(chr, (uint8_t *)&mux_help[i][j], 1);
         }
     }
 }
@@ -370,7 +370,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
         case 'x':
             {
                  const char *term =  "QEMU: Terminated\n\r";
-                 chr->chr_write(chr,(uint8_t *)term,strlen(term));
+                 qemu_chr_fe_write(chr, (uint8_t *)term, strlen(term));
                  exit(0);
                  break;
             }
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/6] qemu-char: move pty_chr_update_read_handler around

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 2bda2fb..b478a3d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1055,6 +1055,22 @@ static void pty_chr_rearm_timer(CharDriverState *chr, int ms)
     }
 }
 
+static void pty_chr_update_read_handler(CharDriverState *chr)
+{
+    PtyCharDriver *s = chr->opaque;
+    GPollFD pfd;
+
+    pfd.fd = g_io_channel_unix_get_fd(s->fd);
+    pfd.events = G_IO_OUT;
+    pfd.revents = 0;
+    g_poll(&pfd, 1, 0);
+    if (pfd.revents & G_IO_HUP) {
+        pty_chr_state(chr, 0);
+    } else {
+        pty_chr_state(chr, 1);
+    }
+}
+
 static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     PtyCharDriver *s = chr->opaque;
@@ -1107,22 +1123,6 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     return TRUE;
 }
 
-static void pty_chr_update_read_handler(CharDriverState *chr)
-{
-    PtyCharDriver *s = chr->opaque;
-    GPollFD pfd;
-
-    pfd.fd = g_io_channel_unix_get_fd(s->fd);
-    pfd.events = G_IO_OUT;
-    pfd.revents = 0;
-    g_poll(&pfd, 1, 0);
-    if (pfd.revents & G_IO_HUP) {
-        pty_chr_state(chr, 0);
-    } else {
-        pty_chr_state(chr, 1);
-    }
-}
-
 static void pty_chr_state(CharDriverState *chr, int connected)
 {
     PtyCharDriver *s = chr->opaque;
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe

[Paolo Bonzini]

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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/sysemu/char.h | 11 +++++++----
 qemu-char.c           | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 9bbfd06..b589099 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);
     GSource *(*chr_add_watch)(struct CharDriverState *s, GIOCondition cond);
@@ -160,6 +161,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
  */
@@ -172,8 +174,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
@@ -188,7 +191,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
@@ -200,7 +203,7 @@ int qemu_chr_fe_write_all(CharDriverState *s, const 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 b478a3d..dcd0765 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);
@@ -147,6 +153,7 @@ int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len)
 
         offset += res;
     }
+    qemu_mutex_unlock(&s->chr_write_lock);
 
     return offset;
 }
@@ -269,11 +276,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;
@@ -819,6 +829,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;
@@ -1018,12 +1029,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 do_pty_chr_update_read_handler(CharDriverState *chr);
 static void pty_chr_state(CharDriverState *chr, int connected);
 
 static gboolean pty_chr_timer(gpointer opaque)
@@ -1031,14 +1044,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);
+        do_pty_chr_update_read_handler(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;
@@ -1055,7 +1071,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 do_pty_chr_update_read_handler(CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
     GPollFD pfd;
@@ -1071,13 +1088,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);
+    do_pty_chr_update_read_handler(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);
+        do_pty_chr_update_read_handler(chr);
         return 0;
     }
     return io_channel_send(s->fd, buf, len);
@@ -1123,6 +1148,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;
@@ -1613,9 +1639,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 {
@@ -1725,6 +1754,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;
@@ -2167,6 +2197,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;
@@ -2307,6 +2338,7 @@ typedef struct {
 
 static gboolean tcp_chr_accept(GIOChannel *chan, GIOCondition cond, void *opaque);
 
+/* Called with chr_write_lock held.  */
 static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     TCPCharDriver *s = chr->opaque;
@@ -2782,6 +2814,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;
@@ -2806,9 +2839,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.8.3.1

[Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex

[Paolo Bonzini]

This lets the block layer emit QMP events from outside the I/O thread.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index 342e83b..ebc66fb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -191,8 +191,11 @@ struct Monitor {
     int flags;
     int suspend_cnt;
     bool skip_flush;
+
+    QemuMutex out_lock;
     QString *outbuf;
-    guint watch;
+    guint out_watch;
+
     ReadLineState *rs;
     MonitorControl *mc;
     CPUState *mon_cpu;
@@ -265,17 +268,22 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
     }
 }
 
+static void do_monitor_flush(Monitor *mon);
+
 static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
                                   void *opaque)
 {
     Monitor *mon = opaque;
 
-    mon->watch = 0;
+    qemu_mutex_lock(&mon->out_lock);
+    mon->out_watch = 0;
     monitor_flush(mon);
+    qemu_mutex_unlock(&mon->out_lock);
     return FALSE;
 }
 
-void monitor_flush(Monitor *mon)
+/* Called with mon->out_lock held.  */
+static void do_monitor_flush(Monitor *mon)
 {
     int rc;
     size_t len;
@@ -302,18 +310,26 @@ void monitor_flush(Monitor *mon)
             QDECREF(mon->outbuf);
             mon->outbuf = tmp;
         }
-        if (mon->watch == 0) {
-            mon->watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
-                                               monitor_unblocked, mon);
+        if (mon->out_watch == 0) {
+            mon->out_watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
+                                                   monitor_unblocked, mon);
         }
     }
 }
 
+void monitor_flush(Monitor *mon)
+{
+    qemu_mutex_lock(&mon->out_lock);
+    do_monitor_flush(mon);
+    qemu_mutex_unlock(&mon->out_lock);
+}
+
 /* flush at every end of line */
 static void monitor_puts(Monitor *mon, const char *str)
 {
     char c;
 
+    qemu_mutex_lock(&mon->out_lock);
     for(;;) {
         c = *str++;
         if (c == '\0')
@@ -323,9 +339,10 @@ static void monitor_puts(Monitor *mon, const char *str)
         }
         qstring_append_chr(mon->outbuf, c);
         if (c == '\n') {
-            monitor_flush(mon);
+            do_monitor_flush(mon);
         }
     }
+    qemu_mutex_unlock(&mon->out_lock);
 }
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
@@ -690,6 +707,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline);
 static void monitor_data_init(Monitor *mon)
 {
     memset(mon, 0, sizeof(Monitor));
+    qemu_mutex_init(&mon->out_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
@@ -698,6 +716,7 @@ static void monitor_data_init(Monitor *mon)
 static void monitor_data_destroy(Monitor *mon)
 {
     QDECREF(mon->outbuf);
+    qemu_mutex_destroy(&mon->out_lock);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -725,11 +744,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     handle_user_command(&hmp, command_line);
     cur_mon = old_mon;
 
+    qemu_mutex_lock(&hmp.out_lock);
     if (qstring_get_length(hmp.outbuf) > 0) {
         output = g_strdup(qstring_get_str(hmp.outbuf));
     } else {
         output = g_strdup("");
     }
+    qemu_mutex_unlock(&hmp.out_lock);
 
 out:
     monitor_data_destroy(&hmp);
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/6] monitor: protect event emission

[Paolo Bonzini]

Event emission must be protected by a mutex because of access to
the shared rate-limiting state, and to guard against concurrent
monitor "hot-plug" by means of human-monitor-command.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index ebc66fb..f7a4ccf 100644
--- a/monitor.c
+++ b/monitor.c
@@ -210,6 +210,9 @@ struct Monitor {
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
+/* Protects mon_list, monitor_event_state.  */
+static QemuMutex monitor_lock;
+
 static QLIST_HEAD(mon_list, Monitor) mon_list;
 static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
 static int mon_refcount;
@@ -531,10 +534,11 @@ static const char *monitor_event_names[] = {
 };
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 
-MonitorEventState monitor_event_state[QEVENT_MAX];
+static MonitorEventState monitor_event_state[QEVENT_MAX];
 
 /*
- * Emits the event to every monitor instance
+ * Emits the event to every monitor instance.
+ * Called with monitor_lock held.
  */
 static void
 monitor_protocol_event_emit(MonitorEvent event,
@@ -571,6 +575,7 @@ monitor_protocol_event_queue(MonitorEvent event,
                                        now);
 
     /* Rate limit of 0 indicates no throttling */
+    qemu_mutex_lock(&monitor_lock);
     if (!evstate->rate) {
         monitor_protocol_event_emit(event, data);
         evstate->last = now;
@@ -595,6 +600,7 @@ monitor_protocol_event_queue(MonitorEvent event,
             evstate->last = now;
         }
     }
+    qemu_mutex_unlock(&monitor_lock);
 }
 
 
@@ -612,12 +618,14 @@ static void monitor_protocol_event_handler(void *opaque)
                                          evstate->data,
                                          evstate->last,
                                          now);
+    qemu_mutex_lock(&monitor_lock);
     if (evstate->data) {
         monitor_protocol_event_emit(evstate->event, evstate->data);
         qobject_decref(evstate->data);
         evstate->data = NULL;
     }
     evstate->last = now;
+    qemu_mutex_unlock(&monitor_lock);
 }
 
 
@@ -5040,6 +5048,11 @@ static void monitor_readline_flush(void *opaque)
     monitor_flush(opaque);
 }
 
+static void __attribute__((constructor)) monitor_lock_init(void)
+{
+    qemu_mutex_init(&monitor_lock);
+}
+
 void monitor_init(CharDriverState *chr, int flags)
 {
     static int is_first_init = 1;
@@ -5077,7 +5090,10 @@ void monitor_init(CharDriverState *chr, int flags)
                               monitor_event, mon);
     }
 
+    qemu_mutex_lock(&monitor_lock);
     QLIST_INSERT_HEAD(&mon_list, mon, entry);
+    qemu_mutex_unlock(&monitor_lock);
+
     if (!default_mon || (flags & MONITOR_IS_DEFAULT))
         default_mon = mon;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 6/6] monitor: protect event emission

[Luiz Capitulino]

On Tue,  3 Jun 2014 18:39:11 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Event emission must be protected by a mutex because of access to
> the shared rate-limiting state, and to guard against concurrent
> monitor "hot-plug" by means of human-monitor-command.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  monitor.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ebc66fb..f7a4ccf 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -210,6 +210,9 @@ struct Monitor {
>  /* QMP checker flags */
>  #define QMP_ACCEPT_UNKNOWNS 1
>  
> +/* Protects mon_list, monitor_event_state.  */
> +static QemuMutex monitor_lock;
> +
>  static QLIST_HEAD(mon_list, Monitor) mon_list;
>  static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
>  static int mon_refcount;
> @@ -531,10 +534,11 @@ static const char *monitor_event_names[] = {
>  };
>  QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
>  
> -MonitorEventState monitor_event_state[QEVENT_MAX];
> +static MonitorEventState monitor_event_state[QEVENT_MAX];
>  
>  /*
> - * Emits the event to every monitor instance
> + * Emits the event to every monitor instance.
> + * Called with monitor_lock held.
>   */
>  static void
>  monitor_protocol_event_emit(MonitorEvent event,
> @@ -571,6 +575,7 @@ monitor_protocol_event_queue(MonitorEvent event,
>                                         now);
>  
>      /* Rate limit of 0 indicates no throttling */
> +    qemu_mutex_lock(&monitor_lock);
>      if (!evstate->rate) {
>          monitor_protocol_event_emit(event, data);
>          evstate->last = now;
> @@ -595,6 +600,7 @@ monitor_protocol_event_queue(MonitorEvent event,
>              evstate->last = now;
>          }
>      }
> +    qemu_mutex_unlock(&monitor_lock);
>  }
>  
>  
> @@ -612,12 +618,14 @@ static void monitor_protocol_event_handler(void *opaque)
>                                           evstate->data,
>                                           evstate->last,
>                                           now);
> +    qemu_mutex_lock(&monitor_lock);
>      if (evstate->data) {
>          monitor_protocol_event_emit(evstate->event, evstate->data);
>          qobject_decref(evstate->data);
>          evstate->data = NULL;
>      }
>      evstate->last = now;
> +    qemu_mutex_unlock(&monitor_lock);
>  }
>  
>  
> @@ -5040,6 +5048,11 @@ static void monitor_readline_flush(void *opaque)
>      monitor_flush(opaque);
>  }
>  
> +static void __attribute__((constructor)) monitor_lock_init(void)
> +{
> +    qemu_mutex_init(&monitor_lock);
> +}
> +
>  void monitor_init(CharDriverState *chr, int flags)
>  {
>      static int is_first_init = 1;
> @@ -5077,7 +5090,10 @@ void monitor_init(CharDriverState *chr, int flags)
>                                monitor_event, mon);
>      }
>  
> +    qemu_mutex_lock(&monitor_lock);
>      QLIST_INSERT_HEAD(&mon_list, mon, entry);
> +    qemu_mutex_unlock(&monitor_lock);
> +
>      if (!default_mon || (flags & MONITOR_IS_DEFAULT))
>          default_mon = mon;
>  }

Re: [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex

[Luiz Capitulino]

On Tue,  3 Jun 2014 18:39:10 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This lets the block layer emit QMP events from outside the I/O thread.

To be honest I'm starting to forget monitor code, but this looks correct
to me. I have only one comment below that doesn't prevent me from adding:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  monitor.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 342e83b..ebc66fb 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -191,8 +191,11 @@ struct Monitor {
>      int flags;
>      int suspend_cnt;
>      bool skip_flush;
> +
> +    QemuMutex out_lock;
>      QString *outbuf;
> -    guint watch;
> +    guint out_watch;
> +
>      ReadLineState *rs;
>      MonitorControl *mc;
>      CPUState *mon_cpu;
> @@ -265,17 +268,22 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>      }
>  }
>  
> +static void do_monitor_flush(Monitor *mon);
> +
>  static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
>                                    void *opaque)
>  {
>      Monitor *mon = opaque;
>  
> -    mon->watch = 0;
> +    qemu_mutex_lock(&mon->out_lock);
> +    mon->out_watch = 0;
>      monitor_flush(mon);
> +    qemu_mutex_unlock(&mon->out_lock);
>      return FALSE;
>  }
>  
> -void monitor_flush(Monitor *mon)
> +/* Called with mon->out_lock held.  */
> +static void do_monitor_flush(Monitor *mon)
>  {
>      int rc;
>      size_t len;
> @@ -302,18 +310,26 @@ void monitor_flush(Monitor *mon)
>              QDECREF(mon->outbuf);
>              mon->outbuf = tmp;
>          }
> -        if (mon->watch == 0) {
> -            mon->watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
> -                                               monitor_unblocked, mon);
> +        if (mon->out_watch == 0) {
> +            mon->out_watch = qemu_chr_fe_add_watch(mon->chr, G_IO_OUT,
> +                                                   monitor_unblocked, mon);
>          }
>      }
>  }
>  
> +void monitor_flush(Monitor *mon)
> +{
> +    qemu_mutex_lock(&mon->out_lock);
> +    do_monitor_flush(mon);
> +    qemu_mutex_unlock(&mon->out_lock);
> +}
> +
>  /* flush at every end of line */
>  static void monitor_puts(Monitor *mon, const char *str)
>  {
>      char c;
>  
> +    qemu_mutex_lock(&mon->out_lock);
>      for(;;) {
>          c = *str++;
>          if (c == '\0')
> @@ -323,9 +339,10 @@ static void monitor_puts(Monitor *mon, const char *str)
>          }
>          qstring_append_chr(mon->outbuf, c);
>          if (c == '\n') {
> -            monitor_flush(mon);
> +            do_monitor_flush(mon);
>          }
>      }
> +    qemu_mutex_unlock(&mon->out_lock);
>  }
>  
>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
> @@ -690,6 +707,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline);
>  static void monitor_data_init(Monitor *mon)
>  {
>      memset(mon, 0, sizeof(Monitor));
> +    qemu_mutex_init(&mon->out_lock);
>      mon->outbuf = qstring_new();
>      /* Use *mon_cmds by default. */
>      mon->cmd_table = mon_cmds;
> @@ -698,6 +716,7 @@ static void monitor_data_init(Monitor *mon)
>  static void monitor_data_destroy(Monitor *mon)
>  {
>      QDECREF(mon->outbuf);
> +    qemu_mutex_destroy(&mon->out_lock);
>  }
>  
>  char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> @@ -725,11 +744,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      handle_user_command(&hmp, command_line);
>      cur_mon = old_mon;
>  
> +    qemu_mutex_lock(&hmp.out_lock);
>      if (qstring_get_length(hmp.outbuf) > 0) {
>          output = g_strdup(qstring_get_str(hmp.outbuf));
>      } else {
>          output = g_strdup("");
>      }
> +    qemu_mutex_unlock(&hmp.out_lock);

Are you sure we need to lock/unlock in this function? hmp is allocated
in the stack.

>  
>  out:
>      monitor_data_destroy(&hmp);

Re: [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex

[Paolo Bonzini]

Il 10/06/2014 16:10, Luiz Capitulino ha scritto:
>> > +    qemu_mutex_lock(&hmp.out_lock);
>> >      if (qstring_get_length(hmp.outbuf) > 0) {
>> >          output = g_strdup(qstring_get_str(hmp.outbuf));
>> >      } else {
>> >          output = g_strdup("");
>> >      }
>> > +    qemu_mutex_unlock(&hmp.out_lock);
> Are you sure we need to lock/unlock in this function? hmp is allocated
> in the stack.
>

No, we don't but it was more obvious to me this way (and looking at it 
again, I'm pretty sure that some static analyzer would complain without 
these).

Paolo

Re: [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex

[Luiz Capitulino]

On Tue, 10 Jun 2014 16:24:29 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 10/06/2014 16:10, Luiz Capitulino ha scritto:
> >> > +    qemu_mutex_lock(&hmp.out_lock);
> >> >      if (qstring_get_length(hmp.outbuf) > 0) {
> >> >          output = g_strdup(qstring_get_str(hmp.outbuf));
> >> >      } else {
> >> >          output = g_strdup("");
> >> >      }
> >> > +    qemu_mutex_unlock(&hmp.out_lock);
> > Are you sure we need to lock/unlock in this function? hmp is allocated
> > in the stack.
> >
> 
> No, we don't but it was more obvious to me this way (and looking at it 
> again, I'm pretty sure that some static analyzer would complain without 
> these).

Fine, I'm OK with that.

Re: [Qemu-devel] [PATCH 1/6] qemu-char: introduce qemu_chr_alloc

[Fam Zheng]

On Tue, 06/03 18:39, Paolo Bonzini wrote:
> The next patch will modify this function to initialize state that is
> common to all backends.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
>  backends/baum.c       |  2 +-
>  backends/msmouse.c    |  2 +-
>  include/sysemu/char.h |  9 +++++++++
>  qemu-char.c           | 32 +++++++++++++++++++-------------
>  spice-qemu-char.c     |  2 +-
>  ui/console.c          |  2 +-
>  6 files changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/backends/baum.c b/backends/baum.c
> index 759003f..796512d 100644
> --- a/backends/baum.c
> +++ b/backends/baum.c
> @@ -574,7 +574,7 @@ CharDriverState *chr_baum_init(void)
>      int tty;
>  
>      baum = g_malloc0(sizeof(BaumDriverState));
> -    baum->chr = chr = g_malloc0(sizeof(CharDriverState));
> +    baum->chr = chr = qemu_chr_alloc();
>  
>      chr->opaque = baum;
>      chr->chr_write = baum_write;
> diff --git a/backends/msmouse.c b/backends/msmouse.c
> index c0dbfcd..650a531 100644
> --- a/backends/msmouse.c
> +++ b/backends/msmouse.c
> @@ -67,7 +67,7 @@ CharDriverState *qemu_chr_open_msmouse(void)
>  {
>      CharDriverState *chr;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      chr->chr_write = msmouse_chr_write;
>      chr->chr_close = msmouse_chr_close;
>      chr->explicit_be_open = true;
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index b81a6ff..9bbfd06 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -85,6 +85,15 @@ struct CharDriverState {
>  };
>  
>  /**
> + * @qemu_chr_alloc:
> + *
> + * Allocate and initialize a new CharDriverState.
> + *
> + * Returns: a newly allocated CharDriverState.
> + */
> +CharDriverState *qemu_chr_alloc(void);
> +
> +/**
>   * @qemu_chr_new_from_opts:
>   *
>   * Create a new character backend from a QemuOpts list.
> diff --git a/qemu-char.c b/qemu-char.c
> index 54ed244..3df5db7 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -91,6 +91,12 @@
>  static QTAILQ_HEAD(CharDriverStateHead, CharDriverState) chardevs =
>      QTAILQ_HEAD_INITIALIZER(chardevs);
>  
> +CharDriverState *qemu_chr_alloc(void)
> +{
> +    CharDriverState *chr = g_malloc0(sizeof(CharDriverState));
> +    return chr;
> +}
> +
>  void qemu_chr_be_event(CharDriverState *s, int event)
>  {
>      /* Keep track if the char device is open */
> @@ -236,7 +242,7 @@ static CharDriverState *qemu_chr_open_null(void)
>  {
>      CharDriverState *chr;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      chr->chr_write = null_chr_write;
>      chr->explicit_be_open = true;
>      return chr;
> @@ -524,7 +530,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
>      CharDriverState *chr;
>      MuxDriver *d;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      d = g_malloc0(sizeof(MuxDriver));
>  
>      chr->opaque = d;
> @@ -899,7 +905,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
>      CharDriverState *chr;
>      FDCharDriver *s;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(FDCharDriver));
>      s->fd_in = io_channel_from_fd(fd_in);
>      s->fd_out = io_channel_from_fd(fd_out);
> @@ -1176,7 +1182,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>  
>      close(slave_fd);
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>  
>      chr->filename = g_strdup_printf("pty:%s", pty_name);
>      ret->pty = g_strdup(pty_name);
> @@ -1538,7 +1544,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
>      drv->fd = fd;
>      drv->mode = IEEE1284_MODE_COMPAT;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      chr->chr_write = null_chr_write;
>      chr->chr_ioctl = pp_ioctl;
>      chr->chr_close = pp_close;
> @@ -1593,7 +1599,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
>  {
>      CharDriverState *chr;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      chr->opaque = (void *)(intptr_t)fd;
>      chr->chr_write = null_chr_write;
>      chr->chr_ioctl = pp_ioctl;
> @@ -1817,7 +1823,7 @@ static CharDriverState *qemu_chr_open_win_path(const char *filename)
>      CharDriverState *chr;
>      WinCharState *s;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(WinCharState));
>      chr->opaque = s;
>      chr->chr_write = win_chr_write;
> @@ -1916,7 +1922,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
>      CharDriverState *chr;
>      WinCharState *s;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(WinCharState));
>      chr->opaque = s;
>      chr->chr_write = win_chr_write;
> @@ -1935,7 +1941,7 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
>      CharDriverState *chr;
>      WinCharState *s;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(WinCharState));
>      s->hcom = fd_out;
>      chr->opaque = s;
> @@ -2091,7 +2097,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>      DWORD              dwMode;
>      int                is_console = 0;
>  
> -    chr   = g_malloc0(sizeof(CharDriverState));
> +    chr   = qemu_chr_alloc();
>      stdio = g_malloc0(sizeof(WinStdioCharState));
>  
>      stdio->hStdIn = GetStdHandle(STD_INPUT_HANDLE);
> @@ -2253,7 +2259,7 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
>      CharDriverState *chr = NULL;
>      NetCharDriver *s = NULL;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(NetCharDriver));
>  
>      s->fd = fd;
> @@ -2637,7 +2643,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
>          return NULL;
>      }
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(TCPCharDriver));
>  
>      s->connected = 0;
> @@ -2822,7 +2828,7 @@ static CharDriverState *qemu_chr_open_ringbuf(ChardevRingbuf *opts,
>      CharDriverState *chr;
>      RingBufCharDriver *d;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      d = g_malloc(sizeof(*d));
>  
>      d->size = opts->has_size ? opts->size : 65536;
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 6624559..4518a4d 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -268,7 +268,7 @@ static CharDriverState *chr_open(const char *subtype,
>      CharDriverState *chr;
>      SpiceCharDriver *s;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(SpiceCharDriver));
>      s->chr = chr;
>      s->active = false;
> diff --git a/ui/console.c b/ui/console.c
> index e057755..c893b27 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1795,7 +1795,7 @@ static CharDriverState *text_console_init(ChardevVC *vc)
>      unsigned width = 0;
>      unsigned height = 0;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> +    chr = qemu_chr_alloc();
>  
>      if (vc->has_width) {
>          width = vc->width;
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 2/6] qemu-char: do not call chr_write directly

[Fam Zheng]

On Tue, 06/03 18:39, Paolo Bonzini wrote:
> Make the mux always go through qemu_chr_fe_write, so that we'll get
> the mutex for the underlying chardev.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
>  qemu-char.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 3df5db7..2bda2fb 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -279,7 +279,7 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>      MuxDriver *d = chr->opaque;
>      int ret;
>      if (!d->timestamps) {
> -        ret = d->drv->chr_write(d->drv, buf, len);
> +        ret = qemu_chr_fe_write(d->drv, buf, len);
>      } else {
>          int i;
>  
> @@ -301,10 +301,10 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>                           (secs / 60) % 60,
>                           secs % 60,
>                           (int)(ti % 1000));
> -                d->drv->chr_write(d->drv, (uint8_t *)buf1, strlen(buf1));
> +                qemu_chr_fe_write(d->drv, (uint8_t *)buf1, strlen(buf1));
>                  d->linestart = 0;
>              }
> -            ret += d->drv->chr_write(d->drv, buf+i, 1);
> +            ret += qemu_chr_fe_write(d->drv, buf+i, 1);
>              if (buf[i] == '\n') {
>                  d->linestart = 1;
>              }
> @@ -339,13 +339,13 @@ static void mux_print_help(CharDriverState *chr)
>                   "\n\rEscape-Char set to Ascii: 0x%02x\n\r\n\r",
>                   term_escape_char);
>      }
> -    chr->chr_write(chr, (uint8_t *)cbuf, strlen(cbuf));
> +    qemu_chr_fe_write(chr, (uint8_t *)cbuf, strlen(cbuf));
>      for (i = 0; mux_help[i] != NULL; i++) {
>          for (j=0; mux_help[i][j] != '\0'; j++) {
>              if (mux_help[i][j] == '%')
> -                chr->chr_write(chr, (uint8_t *)ebuf, strlen(ebuf));
> +                qemu_chr_fe_write(chr, (uint8_t *)ebuf, strlen(ebuf));
>              else
> -                chr->chr_write(chr, (uint8_t *)&mux_help[i][j], 1);
> +                qemu_chr_fe_write(chr, (uint8_t *)&mux_help[i][j], 1);
>          }
>      }
>  }
> @@ -370,7 +370,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
>          case 'x':
>              {
>                   const char *term =  "QEMU: Terminated\n\r";
> -                 chr->chr_write(chr,(uint8_t *)term,strlen(term));
> +                 qemu_chr_fe_write(chr, (uint8_t *)term, strlen(term));
>                   exit(0);
>                   break;
>              }
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 3/6] qemu-char: move pty_chr_update_read_handler around

[Fam Zheng]

On Tue, 06/03 18:39, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
>  qemu-char.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 2bda2fb..b478a3d 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1055,6 +1055,22 @@ static void pty_chr_rearm_timer(CharDriverState *chr, int ms)
>      }
>  }
>  
> +static void pty_chr_update_read_handler(CharDriverState *chr)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +    GPollFD pfd;
> +
> +    pfd.fd = g_io_channel_unix_get_fd(s->fd);
> +    pfd.events = G_IO_OUT;
> +    pfd.revents = 0;
> +    g_poll(&pfd, 1, 0);
> +    if (pfd.revents & G_IO_HUP) {
> +        pty_chr_state(chr, 0);
> +    } else {
> +        pty_chr_state(chr, 1);
> +    }
> +}
> +
>  static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>  {
>      PtyCharDriver *s = chr->opaque;
> @@ -1107,22 +1123,6 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      return TRUE;
>  }
>  
> -static void pty_chr_update_read_handler(CharDriverState *chr)
> -{
> -    PtyCharDriver *s = chr->opaque;
> -    GPollFD pfd;
> -
> -    pfd.fd = g_io_channel_unix_get_fd(s->fd);
> -    pfd.events = G_IO_OUT;
> -    pfd.revents = 0;
> -    g_poll(&pfd, 1, 0);
> -    if (pfd.revents & G_IO_HUP) {
> -        pty_chr_state(chr, 0);
> -    } else {
> -        pty_chr_state(chr, 1);
> -    }
> -}
> -
>  static void pty_chr_state(CharDriverState *chr, int connected)
>  {
>      PtyCharDriver *s = chr->opaque;
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe

[Fam Zheng]

On Tue, 06/03 18:39, Paolo Bonzini wrote:
> diff --git a/qemu-char.c b/qemu-char.c
> index b478a3d..dcd0765 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);
> @@ -147,6 +153,7 @@ int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len)

More context in the loop:

              if (res == -1 && errno == EAGAIN) {
                  g_usleep(100);
              }
          } while (res == -1 && errno == EAGAIN);

          if (res == 0) {
              break;
          }

          if (res < 0) {
(*)           return res;
          }

Doesn't (*) need an unlock?


Fam
>  
>          offset += res;
>      }
> +    qemu_mutex_unlock(&s->chr_write_lock);
>  
>      return offset;
>  }

Re: [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe

[Paolo Bonzini]

Il 11/06/2014 08:59, Fam Zheng ha scritto:
> On Tue, 06/03 18:39, Paolo Bonzini wrote:
>> diff --git a/qemu-char.c b/qemu-char.c
>> index b478a3d..dcd0765 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);
>> @@ -147,6 +153,7 @@ int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len)
>
> More context in the loop:
>
>               if (res == -1 && errno == EAGAIN) {
>                   g_usleep(100);
>               }
>           } while (res == -1 && errno == EAGAIN);
>
>           if (res == 0) {
>               break;
>           }
>
>           if (res < 0) {
> (*)           return res;
>           }
>
> Doesn't (*) need an unlock?

Indeed, thanks.

Paolo

Re: [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

On Tue, Jun 03, 2014 at 06:39:05PM +0200, Paolo Bonzini wrote:
> Even though virtio-blk-dataplane mostly synchronizes with the block layer
> by means of the AioContext, we still need to introduce mutexes for other
> QEMU subsystems that the dataplane thread might encounter on its way.
> Adding rerror/werror support, for example, means that the dataplane
> thread will have to generate QMP events.
> 
> monitor_puts is the entry point for generating QMP responses and events.
> Making it thread-safe lets virtio-blk-dataplane threads generate QMP
> events; because the same entry point is also used for responses, a
> response and an event will never be intertwined.
> 
> Protection is inserted at both the qemu-char and monitor levels.
> A generic mutex is necessary in qemu_fe_chr_write so that
> qemu_chr_fe_write_all does not break its output; we reuse that
> mutex in some of the character devices.
> 
> There is no need to protect against removal of the monitor's backend,
> since the monitor itself cannot be removed.
> 
> Paolo Bonzini (6):
>   qemu-char: introduce qemu_chr_alloc
>   qemu-char: do not call chr_write directly
>   qemu-char: move pty_chr_update_read_handler around
>   qemu-char: make writes thread-safe
>   monitor: protect outbuf with mutex
>   monitor: protect event emission
> 
>  backends/baum.c       |   2 +-
>  backends/msmouse.c    |   2 +-
>  include/sysemu/char.h |  20 ++++++--
>  monitor.c             |  55 ++++++++++++++++++----
>  qemu-char.c           | 125 +++++++++++++++++++++++++++++++++-----------------
>  spice-qemu-char.c     |   2 +-
>  ui/console.c          |   2 +-
>  7 files changed, 149 insertions(+), 59 deletions(-)

Modulo Fam's missing unlock comment:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe

[Luiz Capitulino]

On Fri, 27 Jun 2014 11:43:20 +0200
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Tue, Jun 03, 2014 at 06:39:05PM +0200, Paolo Bonzini wrote:
> > Even though virtio-blk-dataplane mostly synchronizes with the block layer
> > by means of the AioContext, we still need to introduce mutexes for other
> > QEMU subsystems that the dataplane thread might encounter on its way.
> > Adding rerror/werror support, for example, means that the dataplane
> > thread will have to generate QMP events.
> > 
> > monitor_puts is the entry point for generating QMP responses and events.
> > Making it thread-safe lets virtio-blk-dataplane threads generate QMP
> > events; because the same entry point is also used for responses, a
> > response and an event will never be intertwined.
> > 
> > Protection is inserted at both the qemu-char and monitor levels.
> > A generic mutex is necessary in qemu_fe_chr_write so that
> > qemu_chr_fe_write_all does not break its output; we reuse that
> > mutex in some of the character devices.
> > 
> > There is no need to protect against removal of the monitor's backend,
> > since the monitor itself cannot be removed.
> > 
> > Paolo Bonzini (6):
> >   qemu-char: introduce qemu_chr_alloc
> >   qemu-char: do not call chr_write directly
> >   qemu-char: move pty_chr_update_read_handler around
> >   qemu-char: make writes thread-safe
> >   monitor: protect outbuf with mutex
> >   monitor: protect event emission
> > 
> >  backends/baum.c       |   2 +-
> >  backends/msmouse.c    |   2 +-
> >  include/sysemu/char.h |  20 ++++++--
> >  monitor.c             |  55 ++++++++++++++++++----
> >  qemu-char.c           | 125 +++++++++++++++++++++++++++++++++-----------------
> >  spice-qemu-char.c     |   2 +-
> >  ui/console.c          |   2 +-
> >  7 files changed, 149 insertions(+), 59 deletions(-)
> 
> Modulo Fam's missing unlock comment:
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

I appreciate your review, but this one is already merged on master.