From 54d49ac99227aff646ac940abfab3417f5cb1693 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Mon, 25 Mar 2013 13:46:37 -0400 Subject: [PATCH 1/4] qstring: add qstring_get_length() Long overdue. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Acked-by: Gerd Hoffmann --- include/qapi/qmp/qstring.h | 1 + qobject/qstring.c | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h index 0e690f4849..1bc3666107 100644 --- a/include/qapi/qmp/qstring.h +++ b/include/qapi/qmp/qstring.h @@ -26,6 +26,7 @@ typedef struct QString { QString *qstring_new(void); QString *qstring_from_str(const char *str); QString *qstring_from_substr(const char *str, int start, int end); +size_t qstring_get_length(const QString *qstring); const char *qstring_get_str(const QString *qstring); void qstring_append_int(QString *qstring, int64_t value); void qstring_append(QString *qstring, const char *str); diff --git a/qobject/qstring.c b/qobject/qstring.c index 5f7376c336..607b7a142c 100644 --- a/qobject/qstring.c +++ b/qobject/qstring.c @@ -31,6 +31,14 @@ QString *qstring_new(void) return qstring_from_str(""); } +/** + * qstring_get_length(): Get the length of a QString + */ +size_t qstring_get_length(const QString *qstring) +{ + return qstring->length; +} + /** * qstring_from_substr(): Create a new QString from a C string substring * From e1f2641b5926d20f63d36f0de45206be774da8da Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Mon, 25 Mar 2013 13:52:26 -0400 Subject: [PATCH 2/4] Monitor: Make output buffer dynamic Commit f628926bb423fa8a7e0b114511400ea9df38b76a changed monitor_flush() to retry on qemu_chr_fe_write() errors. However, the Monitor's output buffer can keep growing while the retry is not issued and this can cause the buffer to overflow. To reproduce this issue, just start qemu and type on the Monitor: (qemu) ? This will cause an assertion to trig. To fix this problem this commit makes the Monitor buffer dynamic, which means that it can grow as much as needed. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Acked-by: Gerd Hoffmann --- monitor.c | 45 ++++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/monitor.c b/monitor.c index 4ec1db980c..8712c53626 100644 --- a/monitor.c +++ b/monitor.c @@ -188,8 +188,7 @@ struct Monitor { int reset_seen; int flags; int suspend_cnt; - uint8_t outbuf[1024]; - int outbuf_index; + QString *outbuf; ReadLineState *rs; MonitorControl *mc; CPUArchState *mon_cpu; @@ -271,45 +270,52 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, void monitor_flush(Monitor *mon) { int rc; + size_t len; + const char *buf; - if (mon && mon->outbuf_index != 0 && !mon->mux_out) { - rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); - if (rc == mon->outbuf_index) { + buf = qstring_get_str(mon->outbuf); + len = qstring_get_length(mon->outbuf); + + if (mon && len && !mon->mux_out) { + rc = qemu_chr_fe_write(mon->chr, (const uint8_t *) buf, len); + if (rc == len) { /* all flushed */ - mon->outbuf_index = 0; + QDECREF(mon->outbuf); + mon->outbuf = qstring_new(); return; } if (rc > 0) { /* partinal write */ - memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - rc); - mon->outbuf_index -= rc; + QString *tmp = qstring_from_str(buf + rc); + QDECREF(mon->outbuf); + mon->outbuf = tmp; } qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon); } } -/* flush at every end of line or if the buffer is full */ +/* flush at every end of line */ static void monitor_puts(Monitor *mon, const char *str) { char c; for(;;) { - assert(mon->outbuf_index < sizeof(mon->outbuf) - 1); c = *str++; if (c == '\0') break; - if (c == '\n') - mon->outbuf[mon->outbuf_index++] = '\r'; - mon->outbuf[mon->outbuf_index++] = c; - if (mon->outbuf_index >= (sizeof(mon->outbuf) - 1) - || c == '\n') + if (c == '\n') { + qstring_append_chr(mon->outbuf, '\r'); + } + qstring_append_chr(mon->outbuf, c); + if (c == '\n') { monitor_flush(mon); + } } } void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { - char buf[4096]; + char *buf; if (!mon) return; @@ -318,8 +324,9 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) return; } - vsnprintf(buf, sizeof(buf), fmt, ap); + buf = g_strdup_vprintf(fmt, ap); monitor_puts(mon, buf); + g_free(buf); } void monitor_printf(Monitor *mon, const char *fmt, ...) @@ -671,6 +678,8 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, CharDriverState mchar; memset(&hmp, 0, sizeof(hmp)); + hmp.outbuf = qstring_new(); + qemu_chr_init_mem(&mchar); hmp.chr = &mchar; @@ -699,6 +708,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, } out: + QDECREF(hmp.outbuf); qemu_chr_close_mem(hmp.chr); return output; } @@ -4749,6 +4759,7 @@ void monitor_init(CharDriverState *chr, int flags) } mon = g_malloc0(sizeof(*mon)); + mon->outbuf = qstring_new(); mon->chr = chr; mon->flags = flags; From 48c043d0d1835c64b571c484a9f229fe6d220287 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 2 Apr 2013 15:07:33 -0400 Subject: [PATCH 3/4] hmp: human-monitor-command: stop using the Memory chardev driver The Memory chardev driver was added because, as the Monitor's output buffer was static, we needed a way to accumulate the output of an HMP commmand when ran by human-monitor-command. However, the Monitor's output buffer is now dynamic, so it's possible for the human-monitor-command to use it instead of the Memory chardev driver. This commit does that change, but there are two important observations about it: 1. We need a way to signal to the Monitor that it shouldn't call chardev functions when flushing its output. This is done by adding a new flag to the Monitor object called skip_flush (which is set to true by qmp_human_monitor_command()) 2. The current code has buffered semantics: QMP clients will only see a command's output if it flushes its output with a new-line character. This commit changes this to unbuffered, which means that QMP clients will see a command's output whenever the command prints anything. I don't think this will matter in practice though, as I believe all HMP commands print the new-line character anyway. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Acked-by: Gerd Hoffmann --- monitor.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/monitor.c b/monitor.c index 8712c53626..b4bda7777e 100644 --- a/monitor.c +++ b/monitor.c @@ -188,6 +188,7 @@ struct Monitor { int reset_seen; int flags; int suspend_cnt; + bool skip_flush; QString *outbuf; ReadLineState *rs; MonitorControl *mc; @@ -273,6 +274,10 @@ void monitor_flush(Monitor *mon) size_t len; const char *buf; + if (mon->skip_flush) { + return; + } + buf = qstring_get_str(mon->outbuf); len = qstring_get_length(mon->outbuf); @@ -675,13 +680,10 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, { char *output = NULL; Monitor *old_mon, hmp; - CharDriverState mchar; memset(&hmp, 0, sizeof(hmp)); hmp.outbuf = qstring_new(); - - qemu_chr_init_mem(&mchar); - hmp.chr = &mchar; + hmp.skip_flush = true; old_mon = cur_mon; cur_mon = &hmp; @@ -699,17 +701,14 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, handle_user_command(&hmp, command_line); cur_mon = old_mon; - if (qemu_chr_mem_osize(hmp.chr) > 0) { - QString *str = qemu_chr_mem_to_qs(hmp.chr); - output = g_strdup(qstring_get_str(str)); - QDECREF(str); + if (qstring_get_length(hmp.outbuf) > 0) { + output = g_strdup(qstring_get_str(hmp.outbuf)); } else { output = g_strdup(""); } out: QDECREF(hmp.outbuf); - qemu_chr_close_mem(hmp.chr); return output; } From 4bf0bb8014ac2ac61b1004f5d92b2a4594d48017 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 2 Apr 2013 15:29:29 -0400 Subject: [PATCH 4/4] chardev: drop the Memory chardev driver It's not used anymore since the last commit. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Acked-by: Gerd Hoffmann --- qemu-char.c | 64 ----------------------------------------------------- 1 file changed, 64 deletions(-) diff --git a/qemu-char.c b/qemu-char.c index e5eb8dd2ef..0f7f32decb 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2796,70 +2796,6 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) return NULL; } -/***********************************************************/ -/* Memory chardev */ -typedef struct { - size_t outbuf_size; - size_t outbuf_capacity; - uint8_t *outbuf; -} MemoryDriver; - -static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int len) -{ - MemoryDriver *d = chr->opaque; - - /* TODO: the QString implementation has the same code, we should - * introduce a generic way to do this in cutils.c */ - if (d->outbuf_capacity < d->outbuf_size + len) { - /* grow outbuf */ - d->outbuf_capacity += len; - d->outbuf_capacity *= 2; - d->outbuf = g_realloc(d->outbuf, d->outbuf_capacity); - } - - memcpy(d->outbuf + d->outbuf_size, buf, len); - d->outbuf_size += len; - - return len; -} - -void qemu_chr_init_mem(CharDriverState *chr) -{ - MemoryDriver *d; - - d = g_malloc(sizeof(*d)); - d->outbuf_size = 0; - d->outbuf_capacity = 4096; - d->outbuf = g_malloc0(d->outbuf_capacity); - - memset(chr, 0, sizeof(*chr)); - chr->opaque = d; - chr->chr_write = mem_chr_write; -} - -QString *qemu_chr_mem_to_qs(CharDriverState *chr) -{ - MemoryDriver *d = chr->opaque; - return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1); -} - -/* NOTE: this driver can not be closed with qemu_chr_delete()! */ -void qemu_chr_close_mem(CharDriverState *chr) -{ - MemoryDriver *d = chr->opaque; - - g_free(d->outbuf); - g_free(chr->opaque); - chr->opaque = NULL; - chr->chr_write = NULL; -} - -size_t qemu_chr_mem_osize(const CharDriverState *chr) -{ - const MemoryDriver *d = chr->opaque; - return d->outbuf_size; -} - /*********************************************************/ /* Ring buffer chardev */