* [Qemu-devel] [PATCH 1/6] qemu-char: introduce qemu_chr_alloc
2014-06-03 16:39 [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Paolo Bonzini
@ 2014-06-03 16:39 ` Paolo Bonzini
2014-06-11 6:28 ` Fam Zheng
2014-06-03 16:39 ` [Qemu-devel] [PATCH 2/6] qemu-char: do not call chr_write directly Paolo Bonzini
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-03 16:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, kraxel, stefanha, lcapitulino
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 1/6] qemu-char: introduce qemu_chr_alloc
2014-06-03 16:39 ` [Qemu-devel] [PATCH 1/6] qemu-char: introduce qemu_chr_alloc Paolo Bonzini
@ 2014-06-11 6:28 ` Fam Zheng
0 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2014-06-11 6:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, kraxel, qemu-devel, stefanha, lcapitulino
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 2/6] qemu-char: do not call chr_write directly
2014-06-03 16:39 [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Paolo Bonzini
2014-06-03 16:39 ` [Qemu-devel] [PATCH 1/6] qemu-char: introduce qemu_chr_alloc Paolo Bonzini
@ 2014-06-03 16:39 ` Paolo Bonzini
2014-06-11 6:30 ` Fam Zheng
2014-06-03 16:39 ` [Qemu-devel] [PATCH 3/6] qemu-char: move pty_chr_update_read_handler around Paolo Bonzini
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-03 16:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, kraxel, stefanha, lcapitulino
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] qemu-char: do not call chr_write directly
2014-06-03 16:39 ` [Qemu-devel] [PATCH 2/6] qemu-char: do not call chr_write directly Paolo Bonzini
@ 2014-06-11 6:30 ` Fam Zheng
0 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2014-06-11 6:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, kraxel, qemu-devel, stefanha, lcapitulino
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 3/6] qemu-char: move pty_chr_update_read_handler around
2014-06-03 16:39 [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Paolo Bonzini
2014-06-03 16:39 ` [Qemu-devel] [PATCH 1/6] qemu-char: introduce qemu_chr_alloc Paolo Bonzini
2014-06-03 16:39 ` [Qemu-devel] [PATCH 2/6] qemu-char: do not call chr_write directly Paolo Bonzini
@ 2014-06-03 16:39 ` Paolo Bonzini
2014-06-11 6:32 ` Fam Zheng
2014-06-03 16:39 ` [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe Paolo Bonzini
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-03 16:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, kraxel, stefanha, lcapitulino
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 3/6] qemu-char: move pty_chr_update_read_handler around
2014-06-03 16:39 ` [Qemu-devel] [PATCH 3/6] qemu-char: move pty_chr_update_read_handler around Paolo Bonzini
@ 2014-06-11 6:32 ` Fam Zheng
0 siblings, 0 replies; 18+ messages in thread
From: Fam Zheng @ 2014-06-11 6:32 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, kraxel, qemu-devel, stefanha, lcapitulino
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe
2014-06-03 16:39 [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Paolo Bonzini
` (2 preceding siblings ...)
2014-06-03 16:39 ` [Qemu-devel] [PATCH 3/6] qemu-char: move pty_chr_update_read_handler around Paolo Bonzini
@ 2014-06-03 16:39 ` Paolo Bonzini
2014-06-11 6:59 ` Fam Zheng
2014-06-03 16:39 ` [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex Paolo Bonzini
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-03 16:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, kraxel, stefanha, lcapitulino
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe
2014-06-03 16:39 ` [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe Paolo Bonzini
@ 2014-06-11 6:59 ` Fam Zheng
2014-06-11 8:16 ` Paolo Bonzini
0 siblings, 1 reply; 18+ messages in thread
From: Fam Zheng @ 2014-06-11 6:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, kraxel, qemu-devel, stefanha, lcapitulino
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;
> }
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe
2014-06-11 6:59 ` Fam Zheng
@ 2014-06-11 8:16 ` Paolo Bonzini
0 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-11 8:16 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, kraxel, qemu-devel, stefanha, lcapitulino
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex
2014-06-03 16:39 [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Paolo Bonzini
` (3 preceding siblings ...)
2014-06-03 16:39 ` [Qemu-devel] [PATCH 4/6] qemu-char: make writes thread-safe Paolo Bonzini
@ 2014-06-03 16:39 ` Paolo Bonzini
2014-06-10 14:10 ` Luiz Capitulino
2014-06-03 16:39 ` [Qemu-devel] [PATCH 6/6] monitor: protect event emission Paolo Bonzini
2014-06-27 9:43 ` [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Stefan Hajnoczi
6 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-03 16:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, kraxel, stefanha, lcapitulino
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex
2014-06-03 16:39 ` [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex Paolo Bonzini
@ 2014-06-10 14:10 ` Luiz Capitulino
2014-06-10 14:24 ` Paolo Bonzini
0 siblings, 1 reply; 18+ messages in thread
From: Luiz Capitulino @ 2014-06-10 14:10 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, famz, qemu-devel, stefanha, kraxel
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);
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex
2014-06-10 14:10 ` Luiz Capitulino
@ 2014-06-10 14:24 ` Paolo Bonzini
2014-06-10 14:28 ` Luiz Capitulino
0 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-10 14:24 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, famz, qemu-devel, stefanha, kraxel
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex
2014-06-10 14:24 ` Paolo Bonzini
@ 2014-06-10 14:28 ` Luiz Capitulino
0 siblings, 0 replies; 18+ messages in thread
From: Luiz Capitulino @ 2014-06-10 14:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, famz, qemu-devel, stefanha, kraxel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 6/6] monitor: protect event emission
2014-06-03 16:39 [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Paolo Bonzini
` (4 preceding siblings ...)
2014-06-03 16:39 ` [Qemu-devel] [PATCH 5/6] monitor: protect outbuf with mutex Paolo Bonzini
@ 2014-06-03 16:39 ` Paolo Bonzini
2014-06-10 13:33 ` Luiz Capitulino
2014-06-27 9:43 ` [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Stefan Hajnoczi
6 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2014-06-03 16:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, kraxel, stefanha, lcapitulino
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] monitor: protect event emission
2014-06-03 16:39 ` [Qemu-devel] [PATCH 6/6] monitor: protect event emission Paolo Bonzini
@ 2014-06-10 13:33 ` Luiz Capitulino
0 siblings, 0 replies; 18+ messages in thread
From: Luiz Capitulino @ 2014-06-10 13:33 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, famz, qemu-devel, stefanha, kraxel
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;
> }
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe
2014-06-03 16:39 [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Paolo Bonzini
` (5 preceding siblings ...)
2014-06-03 16:39 ` [Qemu-devel] [PATCH 6/6] monitor: protect event emission Paolo Bonzini
@ 2014-06-27 9:43 ` Stefan Hajnoczi
2014-06-27 12:33 ` Luiz Capitulino
6 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2014-06-27 9:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, famz, kraxel, qemu-devel, lcapitulino
[-- 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 --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe
2014-06-27 9:43 ` [Qemu-devel] [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe Stefan Hajnoczi
@ 2014-06-27 12:33 ` Luiz Capitulino
0 siblings, 0 replies; 18+ messages in thread
From: Luiz Capitulino @ 2014-06-27 12:33 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, Paolo Bonzini, famz, qemu-devel, kraxel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread