[Qemu-devel] [PATCH v2 0/4] Monitor: make output buffer dynamic

[Qemu-devel] [PATCH v2 0/4] Monitor: make output buffer dynamic

[Luiz Capitulino]

Hi,

This series does two things: it makes the Monitor's output buffer dynamic
(so that we fix an assertion bug in the Monitor); and it also switches
the human-monitor-command command to use that buffer instead of the
Memory chardev driver, which is dropped.

Most important details in patches 2/4 and 3/4.

o v2

 - Switch human-monitor-command to the dynamic buffer
 - Drop the Memory chardev driver

Luiz Capitulino (4):
  qstring: add qstring_get_length()
  Monitor: Make output buffer dynamic
  hmp: human-monitor-command: stop using the Memory chardev driver
  chardev: drop the Memory chardev driver

 include/qapi/qmp/qstring.h |  1 +
 monitor.c                  | 60 +++++++++++++++++++++++++------------------
 qemu-char.c                | 64 ----------------------------------------------
 qobject/qstring.c          |  8 ++++++
 4 files changed, 44 insertions(+), 89 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/4] qstring: add qstring_get_length()

[Luiz Capitulino]

Long overdue.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 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 0e690f4..1bc3666 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 5f7376c..607b7a1 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -32,6 +32,14 @@ QString *qstring_new(void)
 }
 
 /**
+ * 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
  *
  * Return string reference
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/4] Monitor: Make output buffer dynamic

[Luiz Capitulino]

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 <lcapitulino@redhat.com>
---
 monitor.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4ec1db9..8712c53 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;
+
+    buf = qstring_get_str(mon->outbuf);
+    len = qstring_get_length(mon->outbuf);
 
-    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) {
+    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;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/4] Monitor: Make output buffer dynamic

[Eric Blake]

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

On 04/02/2013 02:18 PM, Luiz Capitulino wrote:
> 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 <lcapitulino@redhat.com>
> ---
>  monitor.c | 45 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

[Qemu-devel] [PATCH 3/4] hmp: human-monitor-command: stop using the Memory chardev driver

[Luiz Capitulino]

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
obversations 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 <lcapitulino@redhat.com>
---
 monitor.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8712c53..b4bda77 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;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 3/4] hmp: human-monitor-command: stop using the Memory chardev driver

[Eric Blake]

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

On 04/02/2013 02:18 PM, Luiz Capitulino wrote:
> 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
> obversations about it:

s/obversations/observations/

> 
>  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.

Ultimately, libvirt will still buffer until it has a complete JSON
reply, even if portions of that reply are being sent unbuffered now
where they were previously buffered until newlines.  At any rate, I
guess you could do some testing with libvirt, such as:

virsh qemu-monitor-command $dom --hmp info

and checking that libvirt isn't confused by the new output timing behavior.

> 
> 	I don't think this will matter in practice though, as I believe
> 	all HMP commands print the new-line character anyway.

Just looking at it from libvirt's point of view, the few HMP commands
that libvirt.so relies on when talking to qemu 1.4 are:

set_cpu
drive_add
drive_del
savevm
loadvm
delvm
inject-nmi
sendkey

[libvirt-qemu.so exposes human-monitor-command as a development aid, via
'virsh qemu-monitor-command', but this is explicitly unsupported and
relegated to a separate library instead of libvirt.so for a reason]

I chose to look at 'delvm'; that command always has a newline ending any
error message, but on the success case, it looked like there was no
output at all.  But once again, since the command is only being used via
the QMP 'human-monitor-command' wrapper, and libvirt is only checking
for known errors and declaring success if none of the error strings are
present, I think it will still work.  I haven't actually tested it, though.

> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

Even without an explicit test on my part, this looks sane enough that I
don't mind if you use:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/4] hmp: human-monitor-command: stop using the Memory chardev driver

[Luiz Capitulino]

On Tue, 02 Apr 2013 14:44:44 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 04/02/2013 02:18 PM, Luiz Capitulino wrote:
> > 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
> > obversations about it:
> 
> s/obversations/observations/

Fixed.

> >  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.
> 
> Ultimately, libvirt will still buffer until it has a complete JSON
> reply, even if portions of that reply are being sent unbuffered now
> where they were previously buffered until newlines.  At any rate, I
> guess you could do some testing with libvirt, such as:
> 
> virsh qemu-monitor-command $dom --hmp info
> 
> and checking that libvirt isn't confused by the new output timing behavior.

Ok, I can test libvirt, but I think my explanation wasn't good enough.

The buffered output I'm referring to isn't the JSON replies, but the
HMP command's output carried in the JSON reply.

For example, if you try "info version":

{ "execute": "human-monitor-command", "arguments": { "command-line": "info version" } }
{"return": "1.4.50\r\n"}

We can see that the 'return' dict contains the output as the hmp_info_version()
command printed with monitor_printf() and that it ends with \n\r.

Now, if we drop the ending new-line from the current code you'll get:

{ "execute": "human-monitor-command", "arguments": { "command-line": "info version" } }
{"return": ""}

That happens because the current code is buffered and the new-line character
flushes the output. If the output is not flushed it's lost, because the buffer
is destroyed when qmp_human_monitor_command() returns.

This commit changes it to unbuffered, meaning that we'll get the output even
if the command being run by human-monitor-command doesn't flush the output
with a new-line.

In theory, we could get broken output like say:

{"return": "1."}

In practice, I think that all HMP commands flush the output anyway, otherwise
we'd have caught this on human-monitor-command already because we wouldn't get
any output with the current code... Thinking about it now the current behavior
seems to be worse :)

And let me re-enforce that I'm doing this change to avoid having two dynamic
buffers (Monitor's and human-monitor-command's).
 
> > 	I don't think this will matter in practice though, as I believe
> > 	all HMP commands print the new-line character anyway.
> 
> Just looking at it from libvirt's point of view, the few HMP commands
> that libvirt.so relies on when talking to qemu 1.4 are:
> 
> set_cpu
> drive_add
> drive_del
> savevm
> loadvm
> delvm
> inject-nmi
> sendkey
> 
> [libvirt-qemu.so exposes human-monitor-command as a development aid, via
> 'virsh qemu-monitor-command', but this is explicitly unsupported and
> relegated to a separate library instead of libvirt.so for a reason]
> 
> I chose to look at 'delvm'; that command always has a newline ending any
> error message, but on the success case, it looked like there was no
> output at all.  

Yes, non-info commands usually don't print anything.

> But once again, since the command is only being used via
> the QMP 'human-monitor-command' wrapper, and libvirt is only checking
> for known errors and declaring success if none of the error strings are
> present, I think it will still work.  I haven't actually tested it, though.

I'll test libvirt before sending a pull request.

> 
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> Even without an explicit test on my part, this looks sane enough that I
> don't mind if you use:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] [PATCH 4/4] chardev: drop the Memory chardev driver

[Luiz Capitulino]

It's not used anymore since the last commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-char.c | 64 -------------------------------------------------------------
 1 file changed, 64 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 7acbf53..5820bda 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2793,70 +2793,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 */
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 4/4] chardev: drop the Memory chardev driver

[Eric Blake]

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

On 04/02/2013 02:18 PM, Luiz Capitulino wrote:
> It's not used anymore since the last commit.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-char.c | 64 -------------------------------------------------------------
>  1 file changed, 64 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] Monitor: make output buffer dynamic

[Gerd Hoffmann]

On 04/02/13 22:18, Luiz Capitulino wrote:
> This series does two things: it makes the Monitor's output buffer dynamic
> (so that we fix an assertion bug in the Monitor); and it also switches
> the human-monitor-command command to use that buffer instead of the
> Memory chardev driver, which is dropped.
> 
> Most important details in patches 2/4 and 3/4.
> 
> o v2
> 
>  - Switch human-monitor-command to the dynamic buffer
>  - Drop the Memory chardev driver

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd