[Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

This command allows QMP clients to execute HMP commands.

Please, check the documentation added to the qmp-commands.hx file
for additional details about the interface and its limitations.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   42 ++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 61607c5..4fe7f5d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -490,6 +490,48 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
     return 0;
 }
 
+static int mon_set_cpu(int cpu_index);
+static void handle_user_command(Monitor *mon, const char *cmdline);
+
+static int do_hmp_passthrough(Monitor *mon, const QDict *params,
+                              QObject **ret_data)
+{
+    int ret = 0;
+    QString *qs;
+    Monitor *old_mon, hmp;
+    CharDriverState bufchr;
+
+    memset(&hmp, 0, sizeof(hmp));
+    hmp.chr = &bufchr;
+    qemu_chr_init_buffered(hmp.chr);
+
+    old_mon = cur_mon;
+    cur_mon = &hmp;
+
+    if (qdict_haskey(params, "cpu-index")) {
+        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
+        if (ret < 0) {
+            goto out;
+        }
+    }
+
+    handle_user_command(&hmp, qdict_get_str(params, "command-line"));
+
+    qs = qemu_chr_buffered_to_qs(hmp.chr);
+    if (qs) {
+        *ret_data = QOBJECT(qs);
+    }
+
+out:
+    cur_mon = old_mon;
+    qemu_chr_close_buffered(hmp.chr);
+
+    if (ret < 0) {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index","a CPU number");
+    }
+    return ret;
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..b344096 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -761,6 +761,51 @@ Example:
 
 Note: This command must be issued before issuing any other command.
 
+EQMP
+
+    {
+        .name       = "hmp_passthrough",
+        .args_type  = "command-line:s,cpu-index:i?",
+        .params     = "",
+        .help       = "",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hmp_passthrough,
+    },
+
+SQMP
+hmp_passthrough
+---------------
+
+Execute a Human Monitor command.
+
+Arguments: 
+
+- command-line: the command name and its arguments, just like the
+                Human Monitor's shell (json-string)
+- cpu-index: select the CPU number to be used by commands which access CPU
+             data, like 'info registers'. The Monitor selects CPU 0 if this
+             argument is not provided (json-int, optional)
+
+Example:
+
+-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
+<- { "return": "kvm support: enabled\r\n" }
+
+Notes:
+
+(1) The Human Monitor is NOT an stable interface, this means that command
+    names, arguments and responses can change or be removed at ANY time.
+    Applications that rely on long term stability guarantees should NOT
+    use this command
+
+(2) Limitations:
+
+    o This command is stateless, this means that commands that depend
+      on state information (such as getfd) might not work
+
+    o Commands that prompt the user for data (eg. 'cont' when the block
+      device is encrypted) don't currently work
+
 3. Query Commands
 =================
 
-- 
1.7.3.2.145.g7ebee

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This command allows QMP clients to execute HMP commands.
>
> Please, check the documentation added to the qmp-commands.hx file
> for additional details about the interface and its limitations.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c       |   42 ++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 61607c5..4fe7f5d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -490,6 +490,48 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
>      return 0;
>  }
>  
> +static int mon_set_cpu(int cpu_index);
> +static void handle_user_command(Monitor *mon, const char *cmdline);
> +
> +static int do_hmp_passthrough(Monitor *mon, const QDict *params,
> +                              QObject **ret_data)
> +{
> +    int ret = 0;
> +    QString *qs;
> +    Monitor *old_mon, hmp;
> +    CharDriverState bufchr;
> +
> +    memset(&hmp, 0, sizeof(hmp));
> +    hmp.chr = &bufchr;
> +    qemu_chr_init_buffered(hmp.chr);
> +
> +    old_mon = cur_mon;
> +    cur_mon = &hmp;
> +
> +    if (qdict_haskey(params, "cpu-index")) {
> +        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
> +        if (ret < 0) {
> +            goto out;
> +        }
> +    }
> +
> +    handle_user_command(&hmp, qdict_get_str(params, "command-line"));
> +
> +    qs = qemu_chr_buffered_to_qs(hmp.chr);
> +    if (qs) {
> +        *ret_data = QOBJECT(qs);
> +    }
> +
> +out:
> +    cur_mon = old_mon;
> +    qemu_chr_close_buffered(hmp.chr);
> +
> +    if (ret < 0) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index","a CPU number");
> +    }

Emit the error right where ret becomes negative.  Cleaner, and allows
for other errors, should they become necessary.

> +    return ret;
> +}
> +
>  static int compare_cmd(const char *name, const char *list)
>  {
>      const char *p, *pstart;
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 793cf1c..b344096 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -761,6 +761,51 @@ Example:
>  
>  Note: This command must be issued before issuing any other command.
>  
> +EQMP
> +
> +    {
> +        .name       = "hmp_passthrough",
> +        .args_type  = "command-line:s,cpu-index:i?",
> +        .params     = "",
> +        .help       = "",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hmp_passthrough,
> +    },
> +
> +SQMP
> +hmp_passthrough
> +---------------
> +
> +Execute a Human Monitor command.
> +
> +Arguments: 
> +
> +- command-line: the command name and its arguments, just like the
> +                Human Monitor's shell (json-string)
> +- cpu-index: select the CPU number to be used by commands which access CPU
> +             data, like 'info registers'. The Monitor selects CPU 0 if this
> +             argument is not provided (json-int, optional)
> +
> +Example:
> +
> +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
> +<- { "return": "kvm support: enabled\r\n" }
> +
> +Notes:
> +
> +(1) The Human Monitor is NOT an stable interface, this means that command
> +    names, arguments and responses can change or be removed at ANY time.
> +    Applications that rely on long term stability guarantees should NOT
> +    use this command
> +
> +(2) Limitations:
> +
> +    o This command is stateless, this means that commands that depend
> +      on state information (such as getfd) might not work
> +
> +    o Commands that prompt the user for data (eg. 'cont' when the block
> +      device is encrypted) don't currently work
> +
>  3. Query Commands
>  =================

In the real human monitor, cpu-index is state (Monitor member mon_cpu).
For pass through, you shift that state into the client (argument
cpu-index).  Is there any other state that could need shifting?  You
mention getfd.

A possible alternative would be to keep the state around instead of
creating a new one for each pass through command.  Dunno, haven't
thought that through.  Maybe you did?

Regardless, this is clearly better than nothing.  We can work on
limitations later, should they turn out to be bothersome.

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

On Wed, 10 Nov 2010 14:20:12 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This command allows QMP clients to execute HMP commands.
> >
> > Please, check the documentation added to the qmp-commands.hx file
> > for additional details about the interface and its limitations.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c       |   42 ++++++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 87 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 61607c5..4fe7f5d 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -490,6 +490,48 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
> >      return 0;
> >  }
> >  
> > +static int mon_set_cpu(int cpu_index);
> > +static void handle_user_command(Monitor *mon, const char *cmdline);
> > +
> > +static int do_hmp_passthrough(Monitor *mon, const QDict *params,
> > +                              QObject **ret_data)
> > +{
> > +    int ret = 0;
> > +    QString *qs;
> > +    Monitor *old_mon, hmp;
> > +    CharDriverState bufchr;
> > +
> > +    memset(&hmp, 0, sizeof(hmp));
> > +    hmp.chr = &bufchr;
> > +    qemu_chr_init_buffered(hmp.chr);
> > +
> > +    old_mon = cur_mon;
> > +    cur_mon = &hmp;
> > +
> > +    if (qdict_haskey(params, "cpu-index")) {
> > +        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
> > +        if (ret < 0) {
> > +            goto out;
> > +        }
> > +    }
> > +
> > +    handle_user_command(&hmp, qdict_get_str(params, "command-line"));
> > +
> > +    qs = qemu_chr_buffered_to_qs(hmp.chr);
> > +    if (qs) {
> > +        *ret_data = QOBJECT(qs);
> > +    }
> > +
> > +out:
> > +    cur_mon = old_mon;
> > +    qemu_chr_close_buffered(hmp.chr);
> > +
> > +    if (ret < 0) {
> > +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index","a CPU number");
> > +    }
> 
> Emit the error right where ret becomes negative.  Cleaner, and allows
> for other errors, should they become necessary.
> 
> > +    return ret;
> > +}
> > +
> >  static int compare_cmd(const char *name, const char *list)
> >  {
> >      const char *p, *pstart;
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 793cf1c..b344096 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -761,6 +761,51 @@ Example:
> >  
> >  Note: This command must be issued before issuing any other command.
> >  
> > +EQMP
> > +
> > +    {
> > +        .name       = "hmp_passthrough",
> > +        .args_type  = "command-line:s,cpu-index:i?",
> > +        .params     = "",
> > +        .help       = "",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = do_hmp_passthrough,
> > +    },
> > +
> > +SQMP
> > +hmp_passthrough
> > +---------------
> > +
> > +Execute a Human Monitor command.
> > +
> > +Arguments: 
> > +
> > +- command-line: the command name and its arguments, just like the
> > +                Human Monitor's shell (json-string)
> > +- cpu-index: select the CPU number to be used by commands which access CPU
> > +             data, like 'info registers'. The Monitor selects CPU 0 if this
> > +             argument is not provided (json-int, optional)
> > +
> > +Example:
> > +
> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
> > +<- { "return": "kvm support: enabled\r\n" }
> > +
> > +Notes:
> > +
> > +(1) The Human Monitor is NOT an stable interface, this means that command
> > +    names, arguments and responses can change or be removed at ANY time.
> > +    Applications that rely on long term stability guarantees should NOT
> > +    use this command
> > +
> > +(2) Limitations:
> > +
> > +    o This command is stateless, this means that commands that depend
> > +      on state information (such as getfd) might not work
> > +
> > +    o Commands that prompt the user for data (eg. 'cont' when the block
> > +      device is encrypted) don't currently work
> > +
> >  3. Query Commands
> >  =================
> 
> In the real human monitor, cpu-index is state (Monitor member mon_cpu).
> For pass through, you shift that state into the client (argument
> cpu-index).  Is there any other state that could need shifting?  You
> mention getfd.

Surprisingly or not, this is a very important question for QMP itself.

Anthony has said that we should make it stateless, and I do think this
is good because it seems to simplify things considerably.

However, I haven't thought about how to make things like getfd stateless.

> A possible alternative would be to keep the state around instead of
> creating a new one for each pass through command.  Dunno, haven't
> thought that through.  Maybe you did?

It's above ;) becoming stateless seems to be the way for QMP.

> Regardless, this is clearly better than nothing.  We can work on
> limitations later, should they turn out to be bothersome.

Agreed.

[Qemu-devel] [PATCH v2 0/3]: QMP: Human Monitor passthrough

[Luiz Capitulino]

Simple example:

-> { "execute": "hmp_passthrough", "arguments": { "command-line": "print /i 10+25" } }
<- { "return": "35\r\n" }

Please, check individual patches for details. Also note that this series
depends on the script improvements one.

changelog
---------

v1 -> v2

- A number of small cleanups and clarifications

Thanks.

[Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Luiz Capitulino]

This driver handles in-memory chardev operations. That's, all writes
to this driver are stored in an internal buffer and it doesn't talk
to the external world in any way.

Right now it's very simple: it supports only writes. But it can be
easily extended to support more operations.

This is going to be used by the monitor's "HMP passthrough via QMP"
feature, which needs to run monitor handlers without a backing
device.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-char.h |    6 +++++
 2 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 88997f9..896df14 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2275,6 +2275,72 @@ 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) {
+        /* grown outbuf */
+        d->outbuf_capacity += len;
+        d->outbuf_capacity *= 2;
+        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
+    }
+
+    memcpy(d->outbuf + d->outbuf_size, buf, len);
+    d->outbuf_size += len;
+
+    return len;
+}
+
+#define DEFAULT_BUF_SIZE 4096
+
+void qemu_chr_init_mem(CharDriverState *chr)
+{
+    MemoryDriver *d;
+
+    d = qemu_malloc(sizeof(*d));
+    d->outbuf_size = 0;
+    d->outbuf_capacity = DEFAULT_BUF_SIZE;
+    d->outbuf = qemu_mallocz(d->outbuf_capacity);
+
+    memset(chr, 0, sizeof(*chr));
+    chr->opaque = d;
+    chr->chr_write = mem_chr_write;
+}
+
+/* assumes the stored data is a string */
+QString *qemu_chr_mem_to_qs(CharDriverState *chr)
+{
+    MemoryDriver *d = chr->opaque;
+
+    if (d->outbuf_size == 0) {
+        return NULL;
+    }
+
+    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
+}
+
+/* NOTE: this driver can not be closed with qemu_chr_close()! */
+void qemu_chr_close_mem(CharDriverState *chr)
+{
+    MemoryDriver *d = chr->opaque;
+
+    qemu_free(d->outbuf);
+    qemu_free(chr->opaque);
+    chr->opaque = NULL;
+    chr->chr_write = NULL;
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
diff --git a/qemu-char.h b/qemu-char.h
index 18ad12b..c4e55b4 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -6,6 +6,7 @@
 #include "qemu-option.h"
 #include "qemu-config.h"
 #include "qobject.h"
+#include "qstring.h"
 
 /* character device */
 
@@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
 
 extern int term_escape_char;
 
+/* memory chardev */
+void qemu_chr_init_mem(CharDriverState *chr);
+void qemu_chr_close_mem(CharDriverState *chr);
+QString *qemu_chr_mem_to_qs(CharDriverState *chr);
+
 /* async I/O support */
 
 int qemu_set_fd_handler2(int fd,
-- 
1.7.3.2.164.g6f10c

[Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

This command allows QMP clients to execute HMP commands.

Please, check the documentation added to the qmp-commands.hx file
for additional details about the interface and its limitations.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   39 +++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8cee35d..89513be 100644
--- a/monitor.c
+++ b/monitor.c
@@ -491,6 +491,45 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
     return 0;
 }
 
+static int mon_set_cpu(int cpu_index);
+static void handle_user_command(Monitor *mon, const char *cmdline);
+
+static int do_hmp_passthrough(Monitor *mon, const QDict *params,
+                              QObject **ret_data)
+{
+    int ret = 0;
+    QString *qs;
+    Monitor *old_mon, hmp;
+    CharDriverState memchr;
+
+    memset(&hmp, 0, sizeof(hmp));
+    hmp.chr = &memchr;
+    qemu_chr_init_mem(hmp.chr);
+
+    old_mon = cur_mon;
+    cur_mon = &hmp;
+
+    if (qdict_haskey(params, "cpu-index")) {
+        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
+        if (ret < 0) {
+            qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index", "a CPU number");
+            goto out;
+        }
+    }
+
+    handle_user_command(&hmp, qdict_get_str(params, "command-line"));
+
+    qs = qemu_chr_mem_to_qs(hmp.chr);
+    if (qs) {
+        *ret_data = QOBJECT(qs);
+    }
+
+out:
+    cur_mon = old_mon;
+    qemu_chr_close_mem(hmp.chr);
+    return ret;
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..b344096 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -761,6 +761,51 @@ Example:
 
 Note: This command must be issued before issuing any other command.
 
+EQMP
+
+    {
+        .name       = "hmp_passthrough",
+        .args_type  = "command-line:s,cpu-index:i?",
+        .params     = "",
+        .help       = "",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hmp_passthrough,
+    },
+
+SQMP
+hmp_passthrough
+---------------
+
+Execute a Human Monitor command.
+
+Arguments: 
+
+- command-line: the command name and its arguments, just like the
+                Human Monitor's shell (json-string)
+- cpu-index: select the CPU number to be used by commands which access CPU
+             data, like 'info registers'. The Monitor selects CPU 0 if this
+             argument is not provided (json-int, optional)
+
+Example:
+
+-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
+<- { "return": "kvm support: enabled\r\n" }
+
+Notes:
+
+(1) The Human Monitor is NOT an stable interface, this means that command
+    names, arguments and responses can change or be removed at ANY time.
+    Applications that rely on long term stability guarantees should NOT
+    use this command
+
+(2) Limitations:
+
+    o This command is stateless, this means that commands that depend
+      on state information (such as getfd) might not work
+
+    o Commands that prompt the user for data (eg. 'cont' when the block
+      device is encrypted) don't currently work
+
 3. Query Commands
 =================
 
-- 
1.7.3.2.164.g6f10c

[Qemu-devel] [PATCH 3/3] QMP/qmp-shell: Introduce HMP mode

[Luiz Capitulino]

In which qmp-shell will exclusively use the HMP passthrough feature,
this is useful for testing.

Example:

    # ./qmp-shell -H qmp-sock
    Welcome to the HMP shell!
    Connected to QEMU 0.13.50

    (QEMU) info network
    VLAN 0 devices:
      user.0: net=10.0.2.0, restricted=n
        e1000.0: model=e1000,macaddr=52:54:00:12:34:56
        Devices not on any VLAN:
    (QEMU)

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-shell |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/QMP/qmp-shell b/QMP/qmp-shell
index 1fb7e76..ce0d259 100755
--- a/QMP/qmp-shell
+++ b/QMP/qmp-shell
@@ -145,6 +145,72 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         else:
             return self._execute_cmd(cmdline)
 
+class HMPShell(QMPShell):
+    def __init__(self, address):
+        QMPShell.__init__(self, address)
+        self.__cpu_index = 0
+
+    def __cmd_completion(self):
+        for cmd in self.__cmd_passthrough('help')['return'].split('\r\n'):
+            if cmd and cmd[0] != '[' and cmd[0] != '\t':
+                name = cmd.split()[0] # drop help text
+                if name == 'info':
+                    continue
+                if name.find('|') != -1:
+                    # Command in the form 'foobar|f' or 'f|foobar', take the
+                    # full name
+                    opt = name.split('|')
+                    if len(opt[0]) == 1:
+                        name = opt[1]
+                    else:
+                        name = opt[0]
+                self._completer.append(name)
+                self._completer.append('help ' + name) # help completion
+
+    def __info_completion(self):
+        for cmd in self.__cmd_passthrough('info')['return'].split('\r\n'):
+            if cmd:
+                self._completer.append('info ' + cmd.split()[1])
+
+    def __other_completion(self):
+        # special cases
+        self._completer.append('help info')
+
+    def _fill_completion(self):
+        self.__cmd_completion()
+        self.__info_completion()
+        self.__other_completion()
+
+    def __cmd_passthrough(self, cmdline):
+        return self.cmd_obj({ 'execute': 'hmp_passthrough', 'arguments':
+                              { 'command-line': cmdline,
+                                'cpu-index': self.__cpu_index } })
+
+    def _execute_cmd(self, cmdline):
+        if cmdline.split()[0] == "cpu":
+            # trap the cpu command, it requires special setting
+            try:
+                self.__cpu_index = int(cmdline.split()[1])
+            except ValueError:
+                print 'cpu command takes an integer argument'
+                return True
+        resp = self.__cmd_passthrough(cmdline)
+        if resp is None:
+            print 'Disconnected'
+            return False
+        assert 'return' in resp or 'error' in resp
+        if 'return' in resp:
+            # Success
+            if len(resp['return']) > 0:
+                print resp['return'],
+        else:
+            # Error
+            print '%s: %s' % (resp['error']['class'], resp['error']['desc'])
+        return True
+
+    def show_banner(self):
+        QMPShell.show_banner(self, msg='Welcome to the HMP shell!')
+
 def die(msg):
     sys.stderr.write('ERROR: %s\n' % msg)
     sys.exit(1)
@@ -156,9 +222,16 @@ def fail_cmdline(option=None):
     sys.exit(1)
 
 def main():
+    addr = ''
     try:
         if len(sys.argv) == 2:
             qemu = QMPShell(sys.argv[1])
+            addr = sys.argv[1]
+        elif len(sys.argv) == 3:
+            if sys.argv[1] != '-H':
+                fail_cmdline(sys.argv[1])
+            qemu = HMPShell(sys.argv[2])
+            addr = sys.argv[2]
         else:
                 fail_cmdline()
     except QMPShellBadPort:
@@ -171,7 +244,7 @@ def main():
     except qmp.QMPCapabilitiesError:
         die('Could not negotiate capabilities')
     except qemu.error:
-        die('Could not connect to %s' % sys.argv[1])
+        die('Could not connect to %s' % addr)
 
     qemu.show_banner()
     while qemu.read_exec_command('(QEMU) '):
-- 
1.7.3.2.164.g6f10c

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This driver handles in-memory chardev operations. That's, all writes
> to this driver are stored in an internal buffer and it doesn't talk
> to the external world in any way.
>
> Right now it's very simple: it supports only writes. But it can be
> easily extended to support more operations.
>
> This is going to be used by the monitor's "HMP passthrough via QMP"
> feature, which needs to run monitor handlers without a backing
> device.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-char.h |    6 +++++
>  2 files changed, 72 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 88997f9..896df14 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2275,6 +2275,72 @@ 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) {
> +        /* grown outbuf */

Used to say "grow" (sans n) here.  Intentional change?

> +        d->outbuf_capacity += len;
> +        d->outbuf_capacity *= 2;
> +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
> +    }
> +
> +    memcpy(d->outbuf + d->outbuf_size, buf, len);
> +    d->outbuf_size += len;
> +
> +    return len;
> +}
> +
> +#define DEFAULT_BUF_SIZE 4096

It's the *initial* buffer size, isn't it?

Doubt it's worth a #define (there's just one user), but that's a matter
of taste.

> +
> +void qemu_chr_init_mem(CharDriverState *chr)
> +{
> +    MemoryDriver *d;
> +
> +    d = qemu_malloc(sizeof(*d));
> +    d->outbuf_size = 0;
> +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
> +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
> +
> +    memset(chr, 0, sizeof(*chr));
> +    chr->opaque = d;
> +    chr->chr_write = mem_chr_write;
> +}
> +
> +/* assumes the stored data is a string */

What else could it be?  Worrying about embedded '\0's?

> +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
> +{
> +    MemoryDriver *d = chr->opaque;
> +
> +    if (d->outbuf_size == 0) {
> +        return NULL;
> +    }

Did you forget to change this?  We agreed to return an empty QString
when chr contains an empty string.

> +
> +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
> +}
> +
> +/* NOTE: this driver can not be closed with qemu_chr_close()! */
> +void qemu_chr_close_mem(CharDriverState *chr)
> +{
> +    MemoryDriver *d = chr->opaque;
> +
> +    qemu_free(d->outbuf);
> +    qemu_free(chr->opaque);
> +    chr->opaque = NULL;
> +    chr->chr_write = NULL;
> +}
> +

Unlike normal character drivers, this one can't be closed with
qemu_chr_close().  It probably explodes if you try.  Please add a
suitable assertion to qemu_chr_close() to document the fact, and to
ensure misuse fails in a controlled, obvious manner.

>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];
> diff --git a/qemu-char.h b/qemu-char.h
> index 18ad12b..c4e55b4 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -6,6 +6,7 @@
>  #include "qemu-option.h"
>  #include "qemu-config.h"
>  #include "qobject.h"
> +#include "qstring.h"
>  
>  /* character device */
>  
> @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
>  
>  extern int term_escape_char;
>  
> +/* memory chardev */
> +void qemu_chr_init_mem(CharDriverState *chr);
> +void qemu_chr_close_mem(CharDriverState *chr);
> +QString *qemu_chr_mem_to_qs(CharDriverState *chr);
> +
>  /* async I/O support */
>  
>  int qemu_set_fd_handler2(int fd,

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 10 Nov 2010 14:20:12 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
[...]
>> > diff --git a/qmp-commands.hx b/qmp-commands.hx
>> > index 793cf1c..b344096 100644
>> > --- a/qmp-commands.hx
>> > +++ b/qmp-commands.hx
>> > @@ -761,6 +761,51 @@ Example:
>> >  
>> >  Note: This command must be issued before issuing any other command.
>> >  
>> > +EQMP
>> > +
>> > +    {
>> > +        .name       = "hmp_passthrough",
>> > +        .args_type  = "command-line:s,cpu-index:i?",
>> > +        .params     = "",
>> > +        .help       = "",
>> > +        .user_print = monitor_user_noop,
>> > +        .mhandler.cmd_new = do_hmp_passthrough,
>> > +    },
>> > +
>> > +SQMP
>> > +hmp_passthrough
>> > +---------------
>> > +
>> > +Execute a Human Monitor command.
>> > +
>> > +Arguments: 
>> > +
>> > +- command-line: the command name and its arguments, just like the
>> > +                Human Monitor's shell (json-string)
>> > +- cpu-index: select the CPU number to be used by commands which access CPU
>> > +             data, like 'info registers'. The Monitor selects CPU 0 if this
>> > +             argument is not provided (json-int, optional)
>> > +
>> > +Example:
>> > +
>> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
>> > +<- { "return": "kvm support: enabled\r\n" }
>> > +
>> > +Notes:
>> > +
>> > +(1) The Human Monitor is NOT an stable interface, this means that command
>> > +    names, arguments and responses can change or be removed at ANY time.
>> > +    Applications that rely on long term stability guarantees should NOT
>> > +    use this command
>> > +
>> > +(2) Limitations:
>> > +
>> > +    o This command is stateless, this means that commands that depend
>> > +      on state information (such as getfd) might not work
>> > +
>> > +    o Commands that prompt the user for data (eg. 'cont' when the block
>> > +      device is encrypted) don't currently work
>> > +
>> >  3. Query Commands
>> >  =================
>> 
>> In the real human monitor, cpu-index is state (Monitor member mon_cpu).
>> For pass through, you shift that state into the client (argument
>> cpu-index).  Is there any other state that could need shifting?  You
>> mention getfd.
>
> Surprisingly or not, this is a very important question for QMP itself.
>
> Anthony has said that we should make it stateless, and I do think this
> is good because it seems to simplify things considerably.
>
> However, I haven't thought about how to make things like getfd stateless.

Hmm, that sounds like we should investigate the getfd problem sooner
rather than later.

[...]

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This command allows QMP clients to execute HMP commands.
>
> Please, check the documentation added to the qmp-commands.hx file
> for additional details about the interface and its limitations.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c       |   39 +++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+), 0 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 8cee35d..89513be 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -491,6 +491,45 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
>      return 0;
>  }
>  
> +static int mon_set_cpu(int cpu_index);
> +static void handle_user_command(Monitor *mon, const char *cmdline);
> +
> +static int do_hmp_passthrough(Monitor *mon, const QDict *params,
> +                              QObject **ret_data)
> +{
> +    int ret = 0;
> +    QString *qs;
> +    Monitor *old_mon, hmp;
> +    CharDriverState memchr;

Uh, let's not shadow memchr() from string.h.

> +
> +    memset(&hmp, 0, sizeof(hmp));
> +    hmp.chr = &memchr;
> +    qemu_chr_init_mem(hmp.chr);
> +
> +    old_mon = cur_mon;
> +    cur_mon = &hmp;
> +
> +    if (qdict_haskey(params, "cpu-index")) {
> +        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
> +        if (ret < 0) {
> +            qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index", "a CPU number");
> +            goto out;
> +        }
> +    }
> +
> +    handle_user_command(&hmp, qdict_get_str(params, "command-line"));

It would sure be nice if handle_user_command() returned status...  But
fixing that is out of this patch series' scope.

> +
> +    qs = qemu_chr_mem_to_qs(hmp.chr);
> +    if (qs) {
> +        *ret_data = QOBJECT(qs);
> +    }

Conditional goes away when qemu_chr_mem_to_qs() is changed not to return
NULL for empty character device.

> +
> +out:
> +    cur_mon = old_mon;
> +    qemu_chr_close_mem(hmp.chr);
> +    return ret;
> +}
> +
>  static int compare_cmd(const char *name, const char *list)
>  {
>      const char *p, *pstart;
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 793cf1c..b344096 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -761,6 +761,51 @@ Example:
>  
>  Note: This command must be issued before issuing any other command.
>  
> +EQMP
> +
> +    {
> +        .name       = "hmp_passthrough",
> +        .args_type  = "command-line:s,cpu-index:i?",
> +        .params     = "",
> +        .help       = "",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hmp_passthrough,
> +    },
> +
> +SQMP
> +hmp_passthrough
> +---------------
> +
> +Execute a Human Monitor command.
> +
> +Arguments: 
> +
> +- command-line: the command name and its arguments, just like the
> +                Human Monitor's shell (json-string)
> +- cpu-index: select the CPU number to be used by commands which access CPU
> +             data, like 'info registers'. The Monitor selects CPU 0 if this
> +             argument is not provided (json-int, optional)
> +
> +Example:
> +
> +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
> +<- { "return": "kvm support: enabled\r\n" }
> +
> +Notes:
> +
> +(1) The Human Monitor is NOT an stable interface, this means that command
> +    names, arguments and responses can change or be removed at ANY time.
> +    Applications that rely on long term stability guarantees should NOT
> +    use this command
> +
> +(2) Limitations:
> +
> +    o This command is stateless, this means that commands that depend
> +      on state information (such as getfd) might not work
> +
> +    o Commands that prompt the user for data (eg. 'cont' when the block
> +      device is encrypted) don't currently work
> +
>  3. Query Commands
>  =================

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Luiz Capitulino]

On Thu, 11 Nov 2010 16:30:26 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This driver handles in-memory chardev operations. That's, all writes
> > to this driver are stored in an internal buffer and it doesn't talk
> > to the external world in any way.
> >
> > Right now it's very simple: it supports only writes. But it can be
> > easily extended to support more operations.
> >
> > This is going to be used by the monitor's "HMP passthrough via QMP"
> > feature, which needs to run monitor handlers without a backing
> > device.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qemu-char.h |    6 +++++
> >  2 files changed, 72 insertions(+), 0 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 88997f9..896df14 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -2275,6 +2275,72 @@ 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) {
> > +        /* grown outbuf */
> 
> Used to say "grow" (sans n) here.  Intentional change?

Hum, no. I think I've squashed an older commit while rebasing (but this seems
to be the only problem).

> 
> > +        d->outbuf_capacity += len;
> > +        d->outbuf_capacity *= 2;
> > +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
> > +    }
> > +
> > +    memcpy(d->outbuf + d->outbuf_size, buf, len);
> > +    d->outbuf_size += len;
> > +
> > +    return len;
> > +}
> > +
> > +#define DEFAULT_BUF_SIZE 4096
> 
> It's the *initial* buffer size, isn't it?

Yes.

> Doubt it's worth a #define (there's just one user), but that's a matter
> of taste.
> 
> > +
> > +void qemu_chr_init_mem(CharDriverState *chr)
> > +{
> > +    MemoryDriver *d;
> > +
> > +    d = qemu_malloc(sizeof(*d));
> > +    d->outbuf_size = 0;
> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
> > +
> > +    memset(chr, 0, sizeof(*chr));
> > +    chr->opaque = d;
> > +    chr->chr_write = mem_chr_write;
> > +}
> > +
> > +/* assumes the stored data is a string */
> 
> What else could it be?  Worrying about embedded '\0's?

Yes, as the driver itself doesn't interpret the contents of its
buffer.

> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
> > +{
> > +    MemoryDriver *d = chr->opaque;
> > +
> > +    if (d->outbuf_size == 0) {
> > +        return NULL;
> > +    }
> 
> Did you forget to change this?  We agreed to return an empty QString
> when chr contains an empty string.

I've changed my mind and forgot to mention it: I thought that we would
need to return NULL on error conditions, but turns out that this function
never fails.

So, I do think it's better to let it that way for two reasons:

 1. An empty has at least the '\0' character, but in this case the buffer
    is really empty

 2. Returning an empty string for this case will add unneeded complexity
    to the caller, ie. checking if the QString's length is 0 and decref'ing it

> 
> > +
> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
> > +}
> > +
> > +/* NOTE: this driver can not be closed with qemu_chr_close()! */
> > +void qemu_chr_close_mem(CharDriverState *chr)
> > +{
> > +    MemoryDriver *d = chr->opaque;
> > +
> > +    qemu_free(d->outbuf);
> > +    qemu_free(chr->opaque);
> > +    chr->opaque = NULL;
> > +    chr->chr_write = NULL;
> > +}
> > +
> 
> Unlike normal character drivers, this one can't be closed with
> qemu_chr_close().  It probably explodes if you try.  Please add a
> suitable assertion to qemu_chr_close() to document the fact, and to
> ensure misuse fails in a controlled, obvious manner.

Ah forgot, but that can be done as a separate patch, so if I don't respin
this series I'll send an additional patch for that.

> 
> >  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> >  {
> >      char host[65], port[33], width[8], height[8];
> > diff --git a/qemu-char.h b/qemu-char.h
> > index 18ad12b..c4e55b4 100644
> > --- a/qemu-char.h
> > +++ b/qemu-char.h
> > @@ -6,6 +6,7 @@
> >  #include "qemu-option.h"
> >  #include "qemu-config.h"
> >  #include "qobject.h"
> > +#include "qstring.h"
> >  
> >  /* character device */
> >  
> > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
> >  
> >  extern int term_escape_char;
> >  
> > +/* memory chardev */
> > +void qemu_chr_init_mem(CharDriverState *chr);
> > +void qemu_chr_close_mem(CharDriverState *chr);
> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr);
> > +
> >  /* async I/O support */
> >  
> >  int qemu_set_fd_handler2(int fd,
> 

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Daniel P. Berrange]

On Thu, Nov 11, 2010 at 04:47:41PM +0100, Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 10 Nov 2010 14:20:12 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> [...]
> >> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> > index 793cf1c..b344096 100644
> >> > --- a/qmp-commands.hx
> >> > +++ b/qmp-commands.hx
> >> > @@ -761,6 +761,51 @@ Example:
> >> >  
> >> >  Note: This command must be issued before issuing any other command.
> >> >  
> >> > +EQMP
> >> > +
> >> > +    {
> >> > +        .name       = "hmp_passthrough",
> >> > +        .args_type  = "command-line:s,cpu-index:i?",
> >> > +        .params     = "",
> >> > +        .help       = "",
> >> > +        .user_print = monitor_user_noop,
> >> > +        .mhandler.cmd_new = do_hmp_passthrough,
> >> > +    },
> >> > +
> >> > +SQMP
> >> > +hmp_passthrough
> >> > +---------------
> >> > +
> >> > +Execute a Human Monitor command.
> >> > +
> >> > +Arguments: 
> >> > +
> >> > +- command-line: the command name and its arguments, just like the
> >> > +                Human Monitor's shell (json-string)
> >> > +- cpu-index: select the CPU number to be used by commands which access CPU
> >> > +             data, like 'info registers'. The Monitor selects CPU 0 if this
> >> > +             argument is not provided (json-int, optional)
> >> > +
> >> > +Example:
> >> > +
> >> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
> >> > +<- { "return": "kvm support: enabled\r\n" }
> >> > +
> >> > +Notes:
> >> > +
> >> > +(1) The Human Monitor is NOT an stable interface, this means that command
> >> > +    names, arguments and responses can change or be removed at ANY time.
> >> > +    Applications that rely on long term stability guarantees should NOT
> >> > +    use this command
> >> > +
> >> > +(2) Limitations:
> >> > +
> >> > +    o This command is stateless, this means that commands that depend
> >> > +      on state information (such as getfd) might not work
> >> > +
> >> > +    o Commands that prompt the user for data (eg. 'cont' when the block
> >> > +      device is encrypted) don't currently work
> >> > +
> >> >  3. Query Commands
> >> >  =================
> >> 
> >> In the real human monitor, cpu-index is state (Monitor member mon_cpu).
> >> For pass through, you shift that state into the client (argument
> >> cpu-index).  Is there any other state that could need shifting?  You
> >> mention getfd.
> >
> > Surprisingly or not, this is a very important question for QMP itself.
> >
> > Anthony has said that we should make it stateless, and I do think this
> > is good because it seems to simplify things considerably.
> >
> > However, I haven't thought about how to make things like getfd stateless.
> 
> Hmm, that sounds like we should investigate the getfd problem sooner
> rather than later.

The SCM_RIGHTS code allows you to send/receive multiple file handles in a 
single sendmsg/recvmsg  call. So why don't we just allow sending of the
file handles with the monitor command that actually needs them, instead of
ahead of time using send_fd. This simplifies life for the client because
they also don't have to worry about cleanup using close_fd if the command
using the FD fails.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Anthony Liguori]

On 11/11/2010 09:55 AM, Daniel P. Berrange wrote:
> On Thu, Nov 11, 2010 at 04:47:41PM +0100, Markus Armbruster wrote:
>    
>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>
>>      
>>> On Wed, 10 Nov 2010 14:20:12 +0100
>>> Markus Armbruster<armbru@redhat.com>  wrote:
>>>
>>>        
>>>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>>>          
>> [...]
>>      
>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>> index 793cf1c..b344096 100644
>>>>> --- a/qmp-commands.hx
>>>>> +++ b/qmp-commands.hx
>>>>> @@ -761,6 +761,51 @@ Example:
>>>>>
>>>>>   Note: This command must be issued before issuing any other command.
>>>>>
>>>>> +EQMP
>>>>> +
>>>>> +    {
>>>>> +        .name       = "hmp_passthrough",
>>>>> +        .args_type  = "command-line:s,cpu-index:i?",
>>>>> +        .params     = "",
>>>>> +        .help       = "",
>>>>> +        .user_print = monitor_user_noop,
>>>>> +        .mhandler.cmd_new = do_hmp_passthrough,
>>>>> +    },
>>>>> +
>>>>> +SQMP
>>>>> +hmp_passthrough
>>>>> +---------------
>>>>> +
>>>>> +Execute a Human Monitor command.
>>>>> +
>>>>> +Arguments:
>>>>> +
>>>>> +- command-line: the command name and its arguments, just like the
>>>>> +                Human Monitor's shell (json-string)
>>>>> +- cpu-index: select the CPU number to be used by commands which access CPU
>>>>> +             data, like 'info registers'. The Monitor selects CPU 0 if this
>>>>> +             argument is not provided (json-int, optional)
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +->  { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
>>>>> +<- { "return": "kvm support: enabled\r\n" }
>>>>> +
>>>>> +Notes:
>>>>> +
>>>>> +(1) The Human Monitor is NOT an stable interface, this means that command
>>>>> +    names, arguments and responses can change or be removed at ANY time.
>>>>> +    Applications that rely on long term stability guarantees should NOT
>>>>> +    use this command
>>>>> +
>>>>> +(2) Limitations:
>>>>> +
>>>>> +    o This command is stateless, this means that commands that depend
>>>>> +      on state information (such as getfd) might not work
>>>>> +
>>>>> +    o Commands that prompt the user for data (eg. 'cont' when the block
>>>>> +      device is encrypted) don't currently work
>>>>> +
>>>>>   3. Query Commands
>>>>>   =================
>>>>>            
>>>> In the real human monitor, cpu-index is state (Monitor member mon_cpu).
>>>> For pass through, you shift that state into the client (argument
>>>> cpu-index).  Is there any other state that could need shifting?  You
>>>> mention getfd.
>>>>          
>>> Surprisingly or not, this is a very important question for QMP itself.
>>>
>>> Anthony has said that we should make it stateless, and I do think this
>>> is good because it seems to simplify things considerably.
>>>
>>> However, I haven't thought about how to make things like getfd stateless.
>>>        
>> Hmm, that sounds like we should investigate the getfd problem sooner
>> rather than later.
>>      
> The SCM_RIGHTS code allows you to send/receive multiple file handles in a
> single sendmsg/recvmsg  call. So why don't we just allow sending of the
> file handles with the monitor command that actually needs them, instead of
> ahead of time using send_fd. This simplifies life for the client because
> they also don't have to worry about cleanup using close_fd if the command
> using the FD fails.
>    

How do we identify file descriptors and then map them to a command?

Regards,

Anthony Liguori

> Regards,
> Daniel
>    

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 11 Nov 2010 16:30:26 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > This driver handles in-memory chardev operations. That's, all writes
>> > to this driver are stored in an internal buffer and it doesn't talk
>> > to the external world in any way.
>> >
>> > Right now it's very simple: it supports only writes. But it can be
>> > easily extended to support more operations.
>> >
>> > This is going to be used by the monitor's "HMP passthrough via QMP"
>> > feature, which needs to run monitor handlers without a backing
>> > device.
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  qemu-char.h |    6 +++++
>> >  2 files changed, 72 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/qemu-char.c b/qemu-char.c
>> > index 88997f9..896df14 100644
>> > --- a/qemu-char.c
>> > +++ b/qemu-char.c
>> > @@ -2275,6 +2275,72 @@ 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) {
>> > +        /* grown outbuf */
>> 
>> Used to say "grow" (sans n) here.  Intentional change?
>
> Hum, no. I think I've squashed an older commit while rebasing (but this seems
> to be the only problem).
>
>> 
>> > +        d->outbuf_capacity += len;
>> > +        d->outbuf_capacity *= 2;
>> > +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
>> > +    }
>> > +
>> > +    memcpy(d->outbuf + d->outbuf_size, buf, len);
>> > +    d->outbuf_size += len;
>> > +
>> > +    return len;
>> > +}
>> > +
>> > +#define DEFAULT_BUF_SIZE 4096
>> 
>> It's the *initial* buffer size, isn't it?
>
> Yes.

Could we make the name reflect that then?

>> Doubt it's worth a #define (there's just one user), but that's a matter
>> of taste.
>> 
>> > +
>> > +void qemu_chr_init_mem(CharDriverState *chr)
>> > +{
>> > +    MemoryDriver *d;
>> > +
>> > +    d = qemu_malloc(sizeof(*d));
>> > +    d->outbuf_size = 0;
>> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
>> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
>> > +
>> > +    memset(chr, 0, sizeof(*chr));
>> > +    chr->opaque = d;
>> > +    chr->chr_write = mem_chr_write;
>> > +}
>> > +
>> > +/* assumes the stored data is a string */
>> 
>> What else could it be?  Worrying about embedded '\0's?
>
> Yes, as the driver itself doesn't interpret the contents of its
> buffer.

What happens if there are embedded '\0's?

>> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
>> > +{
>> > +    MemoryDriver *d = chr->opaque;
>> > +
>> > +    if (d->outbuf_size == 0) {
>> > +        return NULL;
>> > +    }
>> 
>> Did you forget to change this?  We agreed to return an empty QString
>> when chr contains an empty string.
>
> I've changed my mind and forgot to mention it: I thought that we would
> need to return NULL on error conditions, but turns out that this function
> never fails.
>
> So, I do think it's better to let it that way for two reasons:
>
>  1. An empty has at least the '\0' character, but in this case the buffer
>     is really empty

qstring_from_substr() copies the contents of the buffer (any length
works, including 0), then appends a '\0'.  I'm afraid I don't get the
problem here...

>  2. Returning an empty string for this case will add unneeded complexity
>     to the caller, ie. checking if the QString's length is 0 and decref'ing it

I strongly recommend not to screw up the interface of a generally useful
function like qemu_chr_mem_to_qs() just to make its initial user
marginally simpler.

If you decide not to follow my recommendation, please document the
unusual mapping of empty string to null pointer in a function comment.

>> > +
>> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
>> > +}
>> > +
>> > +/* NOTE: this driver can not be closed with qemu_chr_close()! */
>> > +void qemu_chr_close_mem(CharDriverState *chr)
>> > +{
>> > +    MemoryDriver *d = chr->opaque;
>> > +
>> > +    qemu_free(d->outbuf);
>> > +    qemu_free(chr->opaque);
>> > +    chr->opaque = NULL;
>> > +    chr->chr_write = NULL;
>> > +}
>> > +
>> 
>> Unlike normal character drivers, this one can't be closed with
>> qemu_chr_close().  It probably explodes if you try.  Please add a
>> suitable assertion to qemu_chr_close() to document the fact, and to
>> ensure misuse fails in a controlled, obvious manner.
>
> Ah forgot, but that can be done as a separate patch, so if I don't respin
> this series I'll send an additional patch for that.

Okay.

>> 
>> >  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>> >  {
>> >      char host[65], port[33], width[8], height[8];
>> > diff --git a/qemu-char.h b/qemu-char.h
>> > index 18ad12b..c4e55b4 100644
>> > --- a/qemu-char.h
>> > +++ b/qemu-char.h
>> > @@ -6,6 +6,7 @@
>> >  #include "qemu-option.h"
>> >  #include "qemu-config.h"
>> >  #include "qobject.h"
>> > +#include "qstring.h"
>> >  
>> >  /* character device */
>> >  
>> > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
>> >  
>> >  extern int term_escape_char;
>> >  
>> > +/* memory chardev */
>> > +void qemu_chr_init_mem(CharDriverState *chr);
>> > +void qemu_chr_close_mem(CharDriverState *chr);
>> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr);
>> > +
>> >  /* async I/O support */
>> >  
>> >  int qemu_set_fd_handler2(int fd,
>> 

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Daniel P. Berrange]

On Thu, Nov 11, 2010 at 10:30:47AM -0600, Anthony Liguori wrote:
> On 11/11/2010 09:55 AM, Daniel P. Berrange wrote:
> >On Thu, Nov 11, 2010 at 04:47:41PM +0100, Markus Armbruster wrote:
> >>>>>  3. Query Commands
> >>>>>  =================
> >>>>>           
> >>>>In the real human monitor, cpu-index is state (Monitor member mon_cpu).
> >>>>For pass through, you shift that state into the client (argument
> >>>>cpu-index).  Is there any other state that could need shifting?  You
> >>>>mention getfd.
> >>>>         
> >>>Surprisingly or not, this is a very important question for QMP itself.
> >>>
> >>>Anthony has said that we should make it stateless, and I do think this
> >>>is good because it seems to simplify things considerably.
> >>>
> >>>However, I haven't thought about how to make things like getfd stateless.
> >>>       
> >>Hmm, that sounds like we should investigate the getfd problem sooner
> >>rather than later.
> >>     
> >The SCM_RIGHTS code allows you to send/receive multiple file handles in a
> >single sendmsg/recvmsg  call. So why don't we just allow sending of the
> >file handles with the monitor command that actually needs them, instead of
> >ahead of time using send_fd. This simplifies life for the client because
> >they also don't have to worry about cleanup using close_fd if the command
> >using the FD fails.
> 
> How do we identify file descriptors and then map them to a command?

IIUC, the FDs sent/received via struct cmsghdr are in a strictly
ordered array, so why not just define a placeholder syntax for
the commands that maps to the array indexes. eg

  netdev_add tap,fd=$0,vhost_fd=$1,id=hostnet0 

The '$' sign is not valid for a normal FD number, so use of a $0,
$1, $2, etc can reliably be substituted with the real FD number from
the cmsghdr array elements 0, 1, 2, etc

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

On Thu, 11 Nov 2010 16:39:52 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Thu, Nov 11, 2010 at 10:30:47AM -0600, Anthony Liguori wrote:
> > On 11/11/2010 09:55 AM, Daniel P. Berrange wrote:
> > >On Thu, Nov 11, 2010 at 04:47:41PM +0100, Markus Armbruster wrote:
> > >>>>>  3. Query Commands
> > >>>>>  =================
> > >>>>>           
> > >>>>In the real human monitor, cpu-index is state (Monitor member mon_cpu).
> > >>>>For pass through, you shift that state into the client (argument
> > >>>>cpu-index).  Is there any other state that could need shifting?  You
> > >>>>mention getfd.
> > >>>>         
> > >>>Surprisingly or not, this is a very important question for QMP itself.
> > >>>
> > >>>Anthony has said that we should make it stateless, and I do think this
> > >>>is good because it seems to simplify things considerably.
> > >>>
> > >>>However, I haven't thought about how to make things like getfd stateless.
> > >>>       
> > >>Hmm, that sounds like we should investigate the getfd problem sooner
> > >>rather than later.
> > >>     
> > >The SCM_RIGHTS code allows you to send/receive multiple file handles in a
> > >single sendmsg/recvmsg  call. So why don't we just allow sending of the
> > >file handles with the monitor command that actually needs them, instead of
> > >ahead of time using send_fd. This simplifies life for the client because
> > >they also don't have to worry about cleanup using close_fd if the command
> > >using the FD fails.
> > 
> > How do we identify file descriptors and then map them to a command?
> 
> IIUC, the FDs sent/received via struct cmsghdr are in a strictly
> ordered array, so why not just define a placeholder syntax for
> the commands that maps to the array indexes. eg
> 
>   netdev_add tap,fd=$0,vhost_fd=$1,id=hostnet0 
> 
> The '$' sign is not valid for a normal FD number, so use of a $0,
> $1, $2, etc can reliably be substituted with the real FD number from
> the cmsghdr array elements 0, 1, 2, etc

I have the impression that the real problem is that they are being
stored in the wrong place (ie. the monitor), *maybe* they should be stored
somewhere else, ie. a special chardev or something.

Today those fds are linked to a given monitor session..

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

On Thu, 11 Nov 2010 16:47:41 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 10 Nov 2010 14:20:12 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> [...]
> >> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> > index 793cf1c..b344096 100644
> >> > --- a/qmp-commands.hx
> >> > +++ b/qmp-commands.hx
> >> > @@ -761,6 +761,51 @@ Example:
> >> >  
> >> >  Note: This command must be issued before issuing any other command.
> >> >  
> >> > +EQMP
> >> > +
> >> > +    {
> >> > +        .name       = "hmp_passthrough",
> >> > +        .args_type  = "command-line:s,cpu-index:i?",
> >> > +        .params     = "",
> >> > +        .help       = "",
> >> > +        .user_print = monitor_user_noop,
> >> > +        .mhandler.cmd_new = do_hmp_passthrough,
> >> > +    },
> >> > +
> >> > +SQMP
> >> > +hmp_passthrough
> >> > +---------------
> >> > +
> >> > +Execute a Human Monitor command.
> >> > +
> >> > +Arguments: 
> >> > +
> >> > +- command-line: the command name and its arguments, just like the
> >> > +                Human Monitor's shell (json-string)
> >> > +- cpu-index: select the CPU number to be used by commands which access CPU
> >> > +             data, like 'info registers'. The Monitor selects CPU 0 if this
> >> > +             argument is not provided (json-int, optional)
> >> > +
> >> > +Example:
> >> > +
> >> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
> >> > +<- { "return": "kvm support: enabled\r\n" }
> >> > +
> >> > +Notes:
> >> > +
> >> > +(1) The Human Monitor is NOT an stable interface, this means that command
> >> > +    names, arguments and responses can change or be removed at ANY time.
> >> > +    Applications that rely on long term stability guarantees should NOT
> >> > +    use this command
> >> > +
> >> > +(2) Limitations:
> >> > +
> >> > +    o This command is stateless, this means that commands that depend
> >> > +      on state information (such as getfd) might not work
> >> > +
> >> > +    o Commands that prompt the user for data (eg. 'cont' when the block
> >> > +      device is encrypted) don't currently work
> >> > +
> >> >  3. Query Commands
> >> >  =================
> >> 
> >> In the real human monitor, cpu-index is state (Monitor member mon_cpu).
> >> For pass through, you shift that state into the client (argument
> >> cpu-index).  Is there any other state that could need shifting?  You
> >> mention getfd.
> >
> > Surprisingly or not, this is a very important question for QMP itself.
> >
> > Anthony has said that we should make it stateless, and I do think this
> > is good because it seems to simplify things considerably.
> >
> > However, I haven't thought about how to make things like getfd stateless.
> 
> Hmm, that sounds like we should investigate the getfd problem sooner
> rather than later.

Absolutely, but this shouldn't prevent this series of being merged, right?

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Anthony Liguori]

On 11/11/2010 10:39 AM, Daniel P. Berrange wrote:
> IIUC, the FDs sent/received via struct cmsghdr are in a strictly
> ordered array, so why not just define a placeholder syntax for
> the commands that maps to the array indexes. eg
>
>    netdev_add tap,fd=$0,vhost_fd=$1,id=hostnet0
>
> The '$' sign is not valid for a normal FD number, so use of a $0,
> $1, $2, etc can reliably be substituted with the real FD number from
> the cmsghdr array elements 0, 1, 2, etc
>    

Character devices are streaming so it's hard to refer to array indexes 
in a single message.  You could count it for the entire session but 
session boundaries aren't really well defined.

I don't think it's an obvious win.

Regards,

Anthony Liguori

> Regards,
> Daniel
>    

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Anthony Liguori]

On 11/11/2010 10:55 AM, Luiz Capitulino wrote:
> On Thu, 11 Nov 2010 16:47:41 +0100
> Markus Armbruster<armbru@redhat.com>  wrote:
>
>    
>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>
>>      
>>> On Wed, 10 Nov 2010 14:20:12 +0100
>>> Markus Armbruster<armbru@redhat.com>  wrote:
>>>
>>>        
>>>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>>>          
>> [...]
>>      
>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>> index 793cf1c..b344096 100644
>>>>> --- a/qmp-commands.hx
>>>>> +++ b/qmp-commands.hx
>>>>> @@ -761,6 +761,51 @@ Example:
>>>>>
>>>>>   Note: This command must be issued before issuing any other command.
>>>>>
>>>>> +EQMP
>>>>> +
>>>>> +    {
>>>>> +        .name       = "hmp_passthrough",
>>>>> +        .args_type  = "command-line:s,cpu-index:i?",
>>>>> +        .params     = "",
>>>>> +        .help       = "",
>>>>> +        .user_print = monitor_user_noop,
>>>>> +        .mhandler.cmd_new = do_hmp_passthrough,
>>>>> +    },
>>>>> +
>>>>> +SQMP
>>>>> +hmp_passthrough
>>>>> +---------------
>>>>> +
>>>>> +Execute a Human Monitor command.
>>>>> +
>>>>> +Arguments:
>>>>> +
>>>>> +- command-line: the command name and its arguments, just like the
>>>>> +                Human Monitor's shell (json-string)
>>>>> +- cpu-index: select the CPU number to be used by commands which access CPU
>>>>> +             data, like 'info registers'. The Monitor selects CPU 0 if this
>>>>> +             argument is not provided (json-int, optional)
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +->  { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
>>>>> +<- { "return": "kvm support: enabled\r\n" }
>>>>> +
>>>>> +Notes:
>>>>> +
>>>>> +(1) The Human Monitor is NOT an stable interface, this means that command
>>>>> +    names, arguments and responses can change or be removed at ANY time.
>>>>> +    Applications that rely on long term stability guarantees should NOT
>>>>> +    use this command
>>>>> +
>>>>> +(2) Limitations:
>>>>> +
>>>>> +    o This command is stateless, this means that commands that depend
>>>>> +      on state information (such as getfd) might not work
>>>>> +
>>>>> +    o Commands that prompt the user for data (eg. 'cont' when the block
>>>>> +      device is encrypted) don't currently work
>>>>> +
>>>>>   3. Query Commands
>>>>>   =================
>>>>>            
>>>> In the real human monitor, cpu-index is state (Monitor member mon_cpu).
>>>> For pass through, you shift that state into the client (argument
>>>> cpu-index).  Is there any other state that could need shifting?  You
>>>> mention getfd.
>>>>          
>>> Surprisingly or not, this is a very important question for QMP itself.
>>>
>>> Anthony has said that we should make it stateless, and I do think this
>>> is good because it seems to simplify things considerably.
>>>
>>> However, I haven't thought about how to make things like getfd stateless.
>>>        
>> Hmm, that sounds like we should investigate the getfd problem sooner
>> rather than later.
>>      
> Absolutely, but this shouldn't prevent this series of being merged, right?
>    

BTW, don't we have all of the fd= commands covered in QMP already?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

On Thu, 11 Nov 2010 16:47:52 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This command allows QMP clients to execute HMP commands.
> >
> > Please, check the documentation added to the qmp-commands.hx file
> > for additional details about the interface and its limitations.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c       |   39 +++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 84 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 8cee35d..89513be 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -491,6 +491,45 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
> >      return 0;
> >  }
> >  
> > +static int mon_set_cpu(int cpu_index);
> > +static void handle_user_command(Monitor *mon, const char *cmdline);
> > +
> > +static int do_hmp_passthrough(Monitor *mon, const QDict *params,
> > +                              QObject **ret_data)
> > +{
> > +    int ret = 0;
> > +    QString *qs;
> > +    Monitor *old_mon, hmp;
> > +    CharDriverState memchr;
> 
> Uh, let's not shadow memchr() from string.h.

Ah, good catch.

> 
> > +
> > +    memset(&hmp, 0, sizeof(hmp));
> > +    hmp.chr = &memchr;
> > +    qemu_chr_init_mem(hmp.chr);
> > +
> > +    old_mon = cur_mon;
> > +    cur_mon = &hmp;
> > +
> > +    if (qdict_haskey(params, "cpu-index")) {
> > +        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
> > +        if (ret < 0) {
> > +            qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index", "a CPU number");
> > +            goto out;
> > +        }
> > +    }
> > +
> > +    handle_user_command(&hmp, qdict_get_str(params, "command-line"));
> 
> It would sure be nice if handle_user_command() returned status...  But
> fixing that is out of this patch series' scope.
> 
> > +
> > +    qs = qemu_chr_mem_to_qs(hmp.chr);
> > +    if (qs) {
> > +        *ret_data = QOBJECT(qs);
> > +    }
> 
> Conditional goes away when qemu_chr_mem_to_qs() is changed not to return
> NULL for empty character device.

If we do this, then our success response is going to be "", like:

{ "execute": "execute": "hmp_passthrough", "arguments": { "command-line": "stop" } }
{ "return": "" }

Today, it's:

{ "execute": "execute": "hmp_passthrough", "arguments": { "command-line": "stop" } }
{ "return": {} }

Which is what we should expect.

In any case, it's not a big deal to fix that.

> > +
> > +out:
> > +    cur_mon = old_mon;
> > +    qemu_chr_close_mem(hmp.chr);
> > +    return ret;
> > +}
> > +
> >  static int compare_cmd(const char *name, const char *list)
> >  {
> >      const char *p, *pstart;
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 793cf1c..b344096 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -761,6 +761,51 @@ Example:
> >  
> >  Note: This command must be issued before issuing any other command.
> >  
> > +EQMP
> > +
> > +    {
> > +        .name       = "hmp_passthrough",
> > +        .args_type  = "command-line:s,cpu-index:i?",
> > +        .params     = "",
> > +        .help       = "",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = do_hmp_passthrough,
> > +    },
> > +
> > +SQMP
> > +hmp_passthrough
> > +---------------
> > +
> > +Execute a Human Monitor command.
> > +
> > +Arguments: 
> > +
> > +- command-line: the command name and its arguments, just like the
> > +                Human Monitor's shell (json-string)
> > +- cpu-index: select the CPU number to be used by commands which access CPU
> > +             data, like 'info registers'. The Monitor selects CPU 0 if this
> > +             argument is not provided (json-int, optional)
> > +
> > +Example:
> > +
> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
> > +<- { "return": "kvm support: enabled\r\n" }
> > +
> > +Notes:
> > +
> > +(1) The Human Monitor is NOT an stable interface, this means that command
> > +    names, arguments and responses can change or be removed at ANY time.
> > +    Applications that rely on long term stability guarantees should NOT
> > +    use this command
> > +
> > +(2) Limitations:
> > +
> > +    o This command is stateless, this means that commands that depend
> > +      on state information (such as getfd) might not work
> > +
> > +    o Commands that prompt the user for data (eg. 'cont' when the block
> > +      device is encrypted) don't currently work
> > +
> >  3. Query Commands
> >  =================
> 

Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 11 Nov 2010 16:47:41 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Wed, 10 Nov 2010 14:20:12 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> [...]
>> >> > diff --git a/qmp-commands.hx b/qmp-commands.hx
>> >> > index 793cf1c..b344096 100644
>> >> > --- a/qmp-commands.hx
>> >> > +++ b/qmp-commands.hx
>> >> > @@ -761,6 +761,51 @@ Example:
>> >> >  
>> >> >  Note: This command must be issued before issuing any other command.
>> >> >  
>> >> > +EQMP
>> >> > +
>> >> > +    {
>> >> > +        .name       = "hmp_passthrough",
>> >> > +        .args_type  = "command-line:s,cpu-index:i?",
>> >> > +        .params     = "",
>> >> > +        .help       = "",
>> >> > +        .user_print = monitor_user_noop,
>> >> > +        .mhandler.cmd_new = do_hmp_passthrough,
>> >> > +    },
>> >> > +
>> >> > +SQMP
>> >> > +hmp_passthrough
>> >> > +---------------
>> >> > +
>> >> > +Execute a Human Monitor command.
>> >> > +
>> >> > +Arguments: 
>> >> > +
>> >> > +- command-line: the command name and its arguments, just like the
>> >> > +                Human Monitor's shell (json-string)
>> >> > +- cpu-index: select the CPU number to be used by commands which access CPU
>> >> > +             data, like 'info registers'. The Monitor selects CPU 0 if this
>> >> > +             argument is not provided (json-int, optional)
>> >> > +
>> >> > +Example:
>> >> > +
>> >> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info kvm" } }
>> >> > +<- { "return": "kvm support: enabled\r\n" }
>> >> > +
>> >> > +Notes:
>> >> > +
>> >> > +(1) The Human Monitor is NOT an stable interface, this means that command
>> >> > +    names, arguments and responses can change or be removed at ANY time.
>> >> > +    Applications that rely on long term stability guarantees should NOT
>> >> > +    use this command
>> >> > +
>> >> > +(2) Limitations:
>> >> > +
>> >> > +    o This command is stateless, this means that commands that depend
>> >> > +      on state information (such as getfd) might not work
>> >> > +
>> >> > +    o Commands that prompt the user for data (eg. 'cont' when the block
>> >> > +      device is encrypted) don't currently work
>> >> > +
>> >> >  3. Query Commands
>> >> >  =================
>> >> 
>> >> In the real human monitor, cpu-index is state (Monitor member mon_cpu).
>> >> For pass through, you shift that state into the client (argument
>> >> cpu-index).  Is there any other state that could need shifting?  You
>> >> mention getfd.
>> >
>> > Surprisingly or not, this is a very important question for QMP itself.
>> >
>> > Anthony has said that we should make it stateless, and I do think this
>> > is good because it seems to simplify things considerably.
>> >
>> > However, I haven't thought about how to make things like getfd stateless.
>> 
>> Hmm, that sounds like we should investigate the getfd problem sooner
>> rather than later.
>
> Absolutely, but this shouldn't prevent this series of being merged, right?

Right.

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Luiz Capitulino]

On Thu, 11 Nov 2010 17:32:06 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 11 Nov 2010 16:30:26 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > This driver handles in-memory chardev operations. That's, all writes
> >> > to this driver are stored in an internal buffer and it doesn't talk
> >> > to the external world in any way.
> >> >
> >> > Right now it's very simple: it supports only writes. But it can be
> >> > easily extended to support more operations.
> >> >
> >> > This is going to be used by the monitor's "HMP passthrough via QMP"
> >> > feature, which needs to run monitor handlers without a backing
> >> > device.
> >> >
> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> > ---
> >> >  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  qemu-char.h |    6 +++++
> >> >  2 files changed, 72 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/qemu-char.c b/qemu-char.c
> >> > index 88997f9..896df14 100644
> >> > --- a/qemu-char.c
> >> > +++ b/qemu-char.c
> >> > @@ -2275,6 +2275,72 @@ 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) {
> >> > +        /* grown outbuf */
> >> 
> >> Used to say "grow" (sans n) here.  Intentional change?
> >
> > Hum, no. I think I've squashed an older commit while rebasing (but this seems
> > to be the only problem).
> >
> >> 
> >> > +        d->outbuf_capacity += len;
> >> > +        d->outbuf_capacity *= 2;
> >> > +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
> >> > +    }
> >> > +
> >> > +    memcpy(d->outbuf + d->outbuf_size, buf, len);
> >> > +    d->outbuf_size += len;
> >> > +
> >> > +    return len;
> >> > +}
> >> > +
> >> > +#define DEFAULT_BUF_SIZE 4096
> >> 
> >> It's the *initial* buffer size, isn't it?
> >
> > Yes.
> 
> Could we make the name reflect that then?
> 
> >> Doubt it's worth a #define (there's just one user), but that's a matter
> >> of taste.
> >> 
> >> > +
> >> > +void qemu_chr_init_mem(CharDriverState *chr)
> >> > +{
> >> > +    MemoryDriver *d;
> >> > +
> >> > +    d = qemu_malloc(sizeof(*d));
> >> > +    d->outbuf_size = 0;
> >> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
> >> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
> >> > +
> >> > +    memset(chr, 0, sizeof(*chr));
> >> > +    chr->opaque = d;
> >> > +    chr->chr_write = mem_chr_write;
> >> > +}
> >> > +
> >> > +/* assumes the stored data is a string */
> >> 
> >> What else could it be?  Worrying about embedded '\0's?
> >
> > Yes, as the driver itself doesn't interpret the contents of its
> > buffer.
> 
> What happens if there are embedded '\0's?

The string will be shorter than expected? And what if it contains
non-printable characters?

It's just a cautionary comment to help the user identify such problems, I think
we're making a whole argument about a quite minor thing.

> 
> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
> >> > +{
> >> > +    MemoryDriver *d = chr->opaque;
> >> > +
> >> > +    if (d->outbuf_size == 0) {
> >> > +        return NULL;
> >> > +    }
> >> 
> >> Did you forget to change this?  We agreed to return an empty QString
> >> when chr contains an empty string.
> >
> > I've changed my mind and forgot to mention it: I thought that we would
> > need to return NULL on error conditions, but turns out that this function
> > never fails.
> >
> > So, I do think it's better to let it that way for two reasons:
> >
> >  1. An empty has at least the '\0' character, but in this case the buffer
> >     is really empty
> 
> qstring_from_substr() copies the contents of the buffer (any length
> works, including 0), then appends a '\0'.  I'm afraid I don't get the
> problem here...
> 
> >  2. Returning an empty string for this case will add unneeded complexity
> >     to the caller, ie. checking if the QString's length is 0 and decref'ing it
> 
> I strongly recommend not to screw up the interface of a generally useful
> function like qemu_chr_mem_to_qs() just to make its initial user
> marginally simpler.

Okay, found a different way of doing this that should make us both happy.

Very exciting changes in v3!

> 
> If you decide not to follow my recommendation, please document the
> unusual mapping of empty string to null pointer in a function comment.
> 
> >> > +
> >> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
> >> > +}
> >> > +
> >> > +/* NOTE: this driver can not be closed with qemu_chr_close()! */
> >> > +void qemu_chr_close_mem(CharDriverState *chr)
> >> > +{
> >> > +    MemoryDriver *d = chr->opaque;
> >> > +
> >> > +    qemu_free(d->outbuf);
> >> > +    qemu_free(chr->opaque);
> >> > +    chr->opaque = NULL;
> >> > +    chr->chr_write = NULL;
> >> > +}
> >> > +
> >> 
> >> Unlike normal character drivers, this one can't be closed with
> >> qemu_chr_close().  It probably explodes if you try.  Please add a
> >> suitable assertion to qemu_chr_close() to document the fact, and to
> >> ensure misuse fails in a controlled, obvious manner.
> >
> > Ah forgot, but that can be done as a separate patch, so if I don't respin
> > this series I'll send an additional patch for that.
> 
> Okay.

Btw, what should we assert() for? We're going to have to access QTAILQ
member I guess.

> 
> >> 
> >> >  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> >> >  {
> >> >      char host[65], port[33], width[8], height[8];
> >> > diff --git a/qemu-char.h b/qemu-char.h
> >> > index 18ad12b..c4e55b4 100644
> >> > --- a/qemu-char.h
> >> > +++ b/qemu-char.h
> >> > @@ -6,6 +6,7 @@
> >> >  #include "qemu-option.h"
> >> >  #include "qemu-config.h"
> >> >  #include "qobject.h"
> >> > +#include "qstring.h"
> >> >  
> >> >  /* character device */
> >> >  
> >> > @@ -100,6 +101,11 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
> >> >  
> >> >  extern int term_escape_char;
> >> >  
> >> > +/* memory chardev */
> >> > +void qemu_chr_init_mem(CharDriverState *chr);
> >> > +void qemu_chr_close_mem(CharDriverState *chr);
> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr);
> >> > +
> >> >  /* async I/O support */
> >> >  
> >> >  int qemu_set_fd_handler2(int fd,
> >> 
> 

[Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

This command allows QMP clients to execute HMP commands.

Please, check the documentation added to the qmp-commands.hx file
for additional details about the interface and its limitations.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   38 ++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8cee35d..ec31eac 100644
--- a/monitor.c
+++ b/monitor.c
@@ -491,6 +491,44 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
     return 0;
 }
 
+static int mon_set_cpu(int cpu_index);
+static void handle_user_command(Monitor *mon, const char *cmdline);
+
+static int do_hmp_passthrough(Monitor *mon, const QDict *params,
+                              QObject **ret_data)
+{
+    int ret = 0;
+    Monitor *old_mon, hmp;
+    CharDriverState mchar;
+
+    memset(&hmp, 0, sizeof(hmp));
+    qemu_chr_init_mem(&mchar);
+    hmp.chr = &mchar;
+
+    old_mon = cur_mon;
+    cur_mon = &hmp;
+
+    if (qdict_haskey(params, "cpu-index")) {
+        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
+        if (ret < 0) {
+            cur_mon = old_mon;
+            qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index", "a CPU number");
+            goto out;
+        }
+    }
+
+    handle_user_command(&hmp, qdict_get_str(params, "command-line"));
+    cur_mon = old_mon;
+
+    if (qemu_chr_mem_osize(hmp.chr) > 0) {
+        *ret_data = QOBJECT(qemu_chr_mem_to_qs(hmp.chr));
+    }
+
+out:
+    qemu_chr_close_mem(hmp.chr);
+    return ret;
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..e5f157f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -761,6 +761,51 @@ Example:
 
 Note: This command must be issued before issuing any other command.
 
+EQMP
+
+    {
+        .name       = "human-monitor-command",
+        .args_type  = "command-line:s,cpu-index:i?",
+        .params     = "",
+        .help       = "",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hmp_passthrough,
+    },
+
+SQMP
+human-monitor-command
+---------------------
+
+Execute a Human Monitor command.
+
+Arguments: 
+
+- command-line: the command name and its arguments, just like the
+                Human Monitor's shell (json-string)
+- cpu-index: select the CPU number to be used by commands which access CPU
+             data, like 'info registers'. The Monitor selects CPU 0 if this
+             argument is not provided (json-int, optional)
+
+Example:
+
+-> { "execute": "human-monitor-command", "arguments": { "command-line": "info kvm" } }
+<- { "return": "kvm support: enabled\r\n" }
+
+Notes:
+
+(1) The Human Monitor is NOT an stable interface, this means that command
+    names, arguments and responses can change or be removed at ANY time.
+    Applications that rely on long term stability guarantees should NOT
+    use this command
+
+(2) Limitations:
+
+    o This command is stateless, this means that commands that depend
+      on state information (such as getfd) might not work
+
+    o Commands that prompt the user for data (eg. 'cont' when the block
+      device is encrypted) don't currently work
+
 3. Query Commands
 =================
 
-- 
1.7.3.2.164.g6f10c

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 11 Nov 2010 17:32:06 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Thu, 11 Nov 2010 16:30:26 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > This driver handles in-memory chardev operations. That's, all writes
>> >> > to this driver are stored in an internal buffer and it doesn't talk
>> >> > to the external world in any way.
>> >> >
>> >> > Right now it's very simple: it supports only writes. But it can be
>> >> > easily extended to support more operations.
>> >> >
>> >> > This is going to be used by the monitor's "HMP passthrough via QMP"
>> >> > feature, which needs to run monitor handlers without a backing
>> >> > device.
>> >> >
>> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >> > ---
>> >> >  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  qemu-char.h |    6 +++++
>> >> >  2 files changed, 72 insertions(+), 0 deletions(-)
>> >> >
>> >> > diff --git a/qemu-char.c b/qemu-char.c
>> >> > index 88997f9..896df14 100644
>> >> > --- a/qemu-char.c
>> >> > +++ b/qemu-char.c
>> >> > @@ -2275,6 +2275,72 @@ 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) {
>> >> > +        /* grown outbuf */
>> >> 
>> >> Used to say "grow" (sans n) here.  Intentional change?
>> >
>> > Hum, no. I think I've squashed an older commit while rebasing (but this seems
>> > to be the only problem).
>> >
>> >> 
>> >> > +        d->outbuf_capacity += len;
>> >> > +        d->outbuf_capacity *= 2;
>> >> > +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
>> >> > +    }
>> >> > +
>> >> > +    memcpy(d->outbuf + d->outbuf_size, buf, len);
>> >> > +    d->outbuf_size += len;
>> >> > +
>> >> > +    return len;
>> >> > +}
>> >> > +
>> >> > +#define DEFAULT_BUF_SIZE 4096
>> >> 
>> >> It's the *initial* buffer size, isn't it?
>> >
>> > Yes.
>> 
>> Could we make the name reflect that then?
>> 
>> >> Doubt it's worth a #define (there's just one user), but that's a matter
>> >> of taste.
>> >> 
>> >> > +
>> >> > +void qemu_chr_init_mem(CharDriverState *chr)
>> >> > +{
>> >> > +    MemoryDriver *d;
>> >> > +
>> >> > +    d = qemu_malloc(sizeof(*d));
>> >> > +    d->outbuf_size = 0;
>> >> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
>> >> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
>> >> > +
>> >> > +    memset(chr, 0, sizeof(*chr));
>> >> > +    chr->opaque = d;
>> >> > +    chr->chr_write = mem_chr_write;
>> >> > +}
>> >> > +
>> >> > +/* assumes the stored data is a string */
>> >> 
>> >> What else could it be?  Worrying about embedded '\0's?
>> >
>> > Yes, as the driver itself doesn't interpret the contents of its
>> > buffer.
>> 
>> What happens if there are embedded '\0's?
>
> The string will be shorter than expected? And what if it contains
> non-printable characters?
>
> It's just a cautionary comment to help the user identify such problems, I think
> we're making a whole argument about a quite minor thing.

When I see "assumes X" in a function comment, I immediately ask "and
what happens when !X?"  The default answer is "it explodes, so don't do
that".  That answer is wrong here.  Therefore, I find the comment
misleading.

Let's figure out what really happens.  The human command's output is
sent to the client as a JSON string (response object member return).
JSON strings can consist of Unicode characters, "except for the
characters that must be escaped: quotation mark, reverse solidus, and
the control characters (U+0000 through U+001F)" (RFC 4627, section 2.5).

Do we escape these characters?  Where in the code?

>> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
>> >> > +{
>> >> > +    MemoryDriver *d = chr->opaque;
>> >> > +
>> >> > +    if (d->outbuf_size == 0) {
>> >> > +        return NULL;
>> >> > +    }
>> >> 
>> >> Did you forget to change this?  We agreed to return an empty QString
>> >> when chr contains an empty string.
>> >
>> > I've changed my mind and forgot to mention it: I thought that we would
>> > need to return NULL on error conditions, but turns out that this function
>> > never fails.
>> >
>> > So, I do think it's better to let it that way for two reasons:
>> >
>> >  1. An empty has at least the '\0' character, but in this case the buffer
>> >     is really empty
>> 
>> qstring_from_substr() copies the contents of the buffer (any length
>> works, including 0), then appends a '\0'.  I'm afraid I don't get the
>> problem here...
>> 
>> >  2. Returning an empty string for this case will add unneeded complexity
>> >     to the caller, ie. checking if the QString's length is 0 and decref'ing it
>> 
>> I strongly recommend not to screw up the interface of a generally useful
>> function like qemu_chr_mem_to_qs() just to make its initial user
>> marginally simpler.
>
> Okay, found a different way of doing this that should make us both happy.
>
> Very exciting changes in v3!
>
>> 
>> If you decide not to follow my recommendation, please document the
>> unusual mapping of empty string to null pointer in a function comment.
>> 
>> >> > +
>> >> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
>> >> > +}
>> >> > +
>> >> > +/* NOTE: this driver can not be closed with qemu_chr_close()! */
>> >> > +void qemu_chr_close_mem(CharDriverState *chr)
>> >> > +{
>> >> > +    MemoryDriver *d = chr->opaque;
>> >> > +
>> >> > +    qemu_free(d->outbuf);
>> >> > +    qemu_free(chr->opaque);
>> >> > +    chr->opaque = NULL;
>> >> > +    chr->chr_write = NULL;
>> >> > +}
>> >> > +
>> >> 
>> >> Unlike normal character drivers, this one can't be closed with
>> >> qemu_chr_close().  It probably explodes if you try.  Please add a
>> >> suitable assertion to qemu_chr_close() to document the fact, and to
>> >> ensure misuse fails in a controlled, obvious manner.
>> >
>> > Ah forgot, but that can be done as a separate patch, so if I don't respin
>> > this series I'll send an additional patch for that.
>> 
>> Okay.
>
> Btw, what should we assert() for? We're going to have to access QTAILQ
> member I guess.

Fair question.

Semantically, we assert that close is safe and does the job.

For your memory driver, it's not safe, because the QTAILQ_REMOVE() is
safe only when chr is in chardevs, which it isn't.  And it doesn't do
the job, because it doesn't free resources.

We can detect the "not safe" condition: search chardevs for chr.  Might
want to put it in a function qemu_chr_is_internal().

If we don't want to search, we can add a flag that reflects "is in
chardevs".

Taking a step back: "external" character devices are in chardevs, and
are to be closed with qemu_chr_close().  "internal" ones are not, and
are to be closed differently.

[...]

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Luiz Capitulino]

On Fri, 12 Nov 2010 11:16:54 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 11 Nov 2010 17:32:06 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 11 Nov 2010 16:30:26 +0100
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > This driver handles in-memory chardev operations. That's, all writes
> >> >> > to this driver are stored in an internal buffer and it doesn't talk
> >> >> > to the external world in any way.
> >> >> >
> >> >> > Right now it's very simple: it supports only writes. But it can be
> >> >> > easily extended to support more operations.
> >> >> >
> >> >> > This is going to be used by the monitor's "HMP passthrough via QMP"
> >> >> > feature, which needs to run monitor handlers without a backing
> >> >> > device.
> >> >> >
> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> >> > ---
> >> >> >  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >  qemu-char.h |    6 +++++
> >> >> >  2 files changed, 72 insertions(+), 0 deletions(-)
> >> >> >
> >> >> > diff --git a/qemu-char.c b/qemu-char.c
> >> >> > index 88997f9..896df14 100644
> >> >> > --- a/qemu-char.c
> >> >> > +++ b/qemu-char.c
> >> >> > @@ -2275,6 +2275,72 @@ 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) {
> >> >> > +        /* grown outbuf */
> >> >> 
> >> >> Used to say "grow" (sans n) here.  Intentional change?
> >> >
> >> > Hum, no. I think I've squashed an older commit while rebasing (but this seems
> >> > to be the only problem).
> >> >
> >> >> 
> >> >> > +        d->outbuf_capacity += len;
> >> >> > +        d->outbuf_capacity *= 2;
> >> >> > +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
> >> >> > +    }
> >> >> > +
> >> >> > +    memcpy(d->outbuf + d->outbuf_size, buf, len);
> >> >> > +    d->outbuf_size += len;
> >> >> > +
> >> >> > +    return len;
> >> >> > +}
> >> >> > +
> >> >> > +#define DEFAULT_BUF_SIZE 4096
> >> >> 
> >> >> It's the *initial* buffer size, isn't it?
> >> >
> >> > Yes.
> >> 
> >> Could we make the name reflect that then?
> >> 
> >> >> Doubt it's worth a #define (there's just one user), but that's a matter
> >> >> of taste.
> >> >> 
> >> >> > +
> >> >> > +void qemu_chr_init_mem(CharDriverState *chr)
> >> >> > +{
> >> >> > +    MemoryDriver *d;
> >> >> > +
> >> >> > +    d = qemu_malloc(sizeof(*d));
> >> >> > +    d->outbuf_size = 0;
> >> >> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
> >> >> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
> >> >> > +
> >> >> > +    memset(chr, 0, sizeof(*chr));
> >> >> > +    chr->opaque = d;
> >> >> > +    chr->chr_write = mem_chr_write;
> >> >> > +}
> >> >> > +
> >> >> > +/* assumes the stored data is a string */
> >> >> 
> >> >> What else could it be?  Worrying about embedded '\0's?
> >> >
> >> > Yes, as the driver itself doesn't interpret the contents of its
> >> > buffer.
> >> 
> >> What happens if there are embedded '\0's?
> >
> > The string will be shorter than expected? And what if it contains
> > non-printable characters?
> >
> > It's just a cautionary comment to help the user identify such problems, I think
> > we're making a whole argument about a quite minor thing.
> 
> When I see "assumes X" in a function comment, I immediately ask "and
> what happens when !X?"  The default answer is "it explodes, so don't do
> that".  That answer is wrong here.  Therefore, I find the comment
> misleading.

That's how you interpret it, my interpretation is that I might not get
the expected behavior.

> Let's figure out what really happens.  The human command's output is
> sent to the client as a JSON string (response object member return).
> JSON strings can consist of Unicode characters, "except for the
> characters that must be escaped: quotation mark, reverse solidus, and
> the control characters (U+0000 through U+001F)" (RFC 4627, section 2.5).
> 
> Do we escape these characters?  Where in the code?

Should be in the json parser, but qemu_chr_mem_to_qs() doesn't assume its
users (and it obviously shouldn't).

> 
> >> >> > +QString *qemu_chr_mem_to_qs(CharDriverState *chr)
> >> >> > +{
> >> >> > +    MemoryDriver *d = chr->opaque;
> >> >> > +
> >> >> > +    if (d->outbuf_size == 0) {
> >> >> > +        return NULL;
> >> >> > +    }
> >> >> 
> >> >> Did you forget to change this?  We agreed to return an empty QString
> >> >> when chr contains an empty string.
> >> >
> >> > I've changed my mind and forgot to mention it: I thought that we would
> >> > need to return NULL on error conditions, but turns out that this function
> >> > never fails.
> >> >
> >> > So, I do think it's better to let it that way for two reasons:
> >> >
> >> >  1. An empty has at least the '\0' character, but in this case the buffer
> >> >     is really empty
> >> 
> >> qstring_from_substr() copies the contents of the buffer (any length
> >> works, including 0), then appends a '\0'.  I'm afraid I don't get the
> >> problem here...
> >> 
> >> >  2. Returning an empty string for this case will add unneeded complexity
> >> >     to the caller, ie. checking if the QString's length is 0 and decref'ing it
> >> 
> >> I strongly recommend not to screw up the interface of a generally useful
> >> function like qemu_chr_mem_to_qs() just to make its initial user
> >> marginally simpler.
> >
> > Okay, found a different way of doing this that should make us both happy.
> >
> > Very exciting changes in v3!
> >
> >> 
> >> If you decide not to follow my recommendation, please document the
> >> unusual mapping of empty string to null pointer in a function comment.
> >> 
> >> >> > +
> >> >> > +    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
> >> >> > +}
> >> >> > +
> >> >> > +/* NOTE: this driver can not be closed with qemu_chr_close()! */
> >> >> > +void qemu_chr_close_mem(CharDriverState *chr)
> >> >> > +{
> >> >> > +    MemoryDriver *d = chr->opaque;
> >> >> > +
> >> >> > +    qemu_free(d->outbuf);
> >> >> > +    qemu_free(chr->opaque);
> >> >> > +    chr->opaque = NULL;
> >> >> > +    chr->chr_write = NULL;
> >> >> > +}
> >> >> > +
> >> >> 
> >> >> Unlike normal character drivers, this one can't be closed with
> >> >> qemu_chr_close().  It probably explodes if you try.  Please add a
> >> >> suitable assertion to qemu_chr_close() to document the fact, and to
> >> >> ensure misuse fails in a controlled, obvious manner.
> >> >
> >> > Ah forgot, but that can be done as a separate patch, so if I don't respin
> >> > this series I'll send an additional patch for that.
> >> 
> >> Okay.
> >
> > Btw, what should we assert() for? We're going to have to access QTAILQ
> > member I guess.
> 
> Fair question.
> 
> Semantically, we assert that close is safe and does the job.
> 
> For your memory driver, it's not safe, because the QTAILQ_REMOVE() is
> safe only when chr is in chardevs, which it isn't.  And it doesn't do
> the job, because it doesn't free resources.
> 
> We can detect the "not safe" condition: search chardevs for chr.  Might
> want to put it in a function qemu_chr_is_internal().
> 
> If we don't want to search, we can add a flag that reflects "is in
> chardevs".
> 
> Taking a step back: "external" character devices are in chardevs, and
> are to be closed with qemu_chr_close().  "internal" ones are not, and
> are to be closed differently.
> 
> [...]
> 

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 12 Nov 2010 11:16:54 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Thu, 11 Nov 2010 17:32:06 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Thu, 11 Nov 2010 16:30:26 +0100
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> 
>> >> >> > This driver handles in-memory chardev operations. That's, all writes
>> >> >> > to this driver are stored in an internal buffer and it doesn't talk
>> >> >> > to the external world in any way.
>> >> >> >
>> >> >> > Right now it's very simple: it supports only writes. But it can be
>> >> >> > easily extended to support more operations.
>> >> >> >
>> >> >> > This is going to be used by the monitor's "HMP passthrough via QMP"
>> >> >> > feature, which needs to run monitor handlers without a backing
>> >> >> > device.
>> >> >> >
>> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >> >> > ---
>> >> >> >  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >> >  qemu-char.h |    6 +++++
>> >> >> >  2 files changed, 72 insertions(+), 0 deletions(-)
>> >> >> >
>> >> >> > diff --git a/qemu-char.c b/qemu-char.c
>> >> >> > index 88997f9..896df14 100644
>> >> >> > --- a/qemu-char.c
>> >> >> > +++ b/qemu-char.c
>> >> >> > @@ -2275,6 +2275,72 @@ 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) {
>> >> >> > +        /* grown outbuf */
>> >> >> 
>> >> >> Used to say "grow" (sans n) here.  Intentional change?
>> >> >
>> >> > Hum, no. I think I've squashed an older commit while rebasing (but this seems
>> >> > to be the only problem).
>> >> >
>> >> >> 
>> >> >> > +        d->outbuf_capacity += len;
>> >> >> > +        d->outbuf_capacity *= 2;
>> >> >> > +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
>> >> >> > +    }
>> >> >> > +
>> >> >> > +    memcpy(d->outbuf + d->outbuf_size, buf, len);
>> >> >> > +    d->outbuf_size += len;
>> >> >> > +
>> >> >> > +    return len;
>> >> >> > +}
>> >> >> > +
>> >> >> > +#define DEFAULT_BUF_SIZE 4096
>> >> >> 
>> >> >> It's the *initial* buffer size, isn't it?
>> >> >
>> >> > Yes.
>> >> 
>> >> Could we make the name reflect that then?
>> >> 
>> >> >> Doubt it's worth a #define (there's just one user), but that's a matter
>> >> >> of taste.
>> >> >> 
>> >> >> > +
>> >> >> > +void qemu_chr_init_mem(CharDriverState *chr)
>> >> >> > +{
>> >> >> > +    MemoryDriver *d;
>> >> >> > +
>> >> >> > +    d = qemu_malloc(sizeof(*d));
>> >> >> > +    d->outbuf_size = 0;
>> >> >> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
>> >> >> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
>> >> >> > +
>> >> >> > +    memset(chr, 0, sizeof(*chr));
>> >> >> > +    chr->opaque = d;
>> >> >> > +    chr->chr_write = mem_chr_write;
>> >> >> > +}
>> >> >> > +
>> >> >> > +/* assumes the stored data is a string */
>> >> >> 
>> >> >> What else could it be?  Worrying about embedded '\0's?
>> >> >
>> >> > Yes, as the driver itself doesn't interpret the contents of its
>> >> > buffer.
>> >> 
>> >> What happens if there are embedded '\0's?
>> >
>> > The string will be shorter than expected? And what if it contains
>> > non-printable characters?
>> >
>> > It's just a cautionary comment to help the user identify such problems, I think
>> > we're making a whole argument about a quite minor thing.
>> 
>> When I see "assumes X" in a function comment, I immediately ask "and
>> what happens when !X?"  The default answer is "it explodes, so don't do
>> that".  That answer is wrong here.  Therefore, I find the comment
>> misleading.
>
> That's how you interpret it, my interpretation is that I might not get
> the expected behavior.

Actually, this function works just fine for embedded '\0's (I tested
it): it returns the correct QString, with full length and '\0' embedded.

Only later, when we attempt to put that QString on the wire do we screw
up, in to_json().  It fails to consider the length, and stops at the
first 0.  In fact, there's not even a way to get the length of a
QString!  There's only qstring_get_str().  I'd call that an API bug.
You might call it a restriction instead ;)

If anything needs a comment, it's qobject_to_json().  But I think that
one needs a bug fix instead.

Alternatively, we could document that QString and its users can't cope
with embedded '\0'.

>> Let's figure out what really happens.  The human command's output is
>> sent to the client as a JSON string (response object member return).
>> JSON strings can consist of Unicode characters, "except for the
>> characters that must be escaped: quotation mark, reverse solidus, and
>> the control characters (U+0000 through U+001F)" (RFC 4627, section 2.5).
>> 
>> Do we escape these characters?  Where in the code?
>
> Should be in the json parser, but qemu_chr_mem_to_qs() doesn't assume its
> users (and it obviously shouldn't).

It's in to_json().

[...]

Re: [Qemu-devel] [PATCH 1/3] qemu-char: Introduce Memory driver

[Luiz Capitulino]

On Fri, 12 Nov 2010 16:54:14 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 12 Nov 2010 11:16:54 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 11 Nov 2010 17:32:06 +0100
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Thu, 11 Nov 2010 16:30:26 +0100
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> 
> >> >> >> > This driver handles in-memory chardev operations. That's, all writes
> >> >> >> > to this driver are stored in an internal buffer and it doesn't talk
> >> >> >> > to the external world in any way.
> >> >> >> >
> >> >> >> > Right now it's very simple: it supports only writes. But it can be
> >> >> >> > easily extended to support more operations.
> >> >> >> >
> >> >> >> > This is going to be used by the monitor's "HMP passthrough via QMP"
> >> >> >> > feature, which needs to run monitor handlers without a backing
> >> >> >> > device.
> >> >> >> >
> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> >> >> > ---
> >> >> >> >  qemu-char.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >> >  qemu-char.h |    6 +++++
> >> >> >> >  2 files changed, 72 insertions(+), 0 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/qemu-char.c b/qemu-char.c
> >> >> >> > index 88997f9..896df14 100644
> >> >> >> > --- a/qemu-char.c
> >> >> >> > +++ b/qemu-char.c
> >> >> >> > @@ -2275,6 +2275,72 @@ 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) {
> >> >> >> > +        /* grown outbuf */
> >> >> >> 
> >> >> >> Used to say "grow" (sans n) here.  Intentional change?
> >> >> >
> >> >> > Hum, no. I think I've squashed an older commit while rebasing (but this seems
> >> >> > to be the only problem).
> >> >> >
> >> >> >> 
> >> >> >> > +        d->outbuf_capacity += len;
> >> >> >> > +        d->outbuf_capacity *= 2;
> >> >> >> > +        d->outbuf = qemu_realloc(d->outbuf, d->outbuf_capacity);
> >> >> >> > +    }
> >> >> >> > +
> >> >> >> > +    memcpy(d->outbuf + d->outbuf_size, buf, len);
> >> >> >> > +    d->outbuf_size += len;
> >> >> >> > +
> >> >> >> > +    return len;
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > +#define DEFAULT_BUF_SIZE 4096
> >> >> >> 
> >> >> >> It's the *initial* buffer size, isn't it?
> >> >> >
> >> >> > Yes.
> >> >> 
> >> >> Could we make the name reflect that then?
> >> >> 
> >> >> >> Doubt it's worth a #define (there's just one user), but that's a matter
> >> >> >> of taste.
> >> >> >> 
> >> >> >> > +
> >> >> >> > +void qemu_chr_init_mem(CharDriverState *chr)
> >> >> >> > +{
> >> >> >> > +    MemoryDriver *d;
> >> >> >> > +
> >> >> >> > +    d = qemu_malloc(sizeof(*d));
> >> >> >> > +    d->outbuf_size = 0;
> >> >> >> > +    d->outbuf_capacity = DEFAULT_BUF_SIZE;
> >> >> >> > +    d->outbuf = qemu_mallocz(d->outbuf_capacity);
> >> >> >> > +
> >> >> >> > +    memset(chr, 0, sizeof(*chr));
> >> >> >> > +    chr->opaque = d;
> >> >> >> > +    chr->chr_write = mem_chr_write;
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > +/* assumes the stored data is a string */
> >> >> >> 
> >> >> >> What else could it be?  Worrying about embedded '\0's?
> >> >> >
> >> >> > Yes, as the driver itself doesn't interpret the contents of its
> >> >> > buffer.
> >> >> 
> >> >> What happens if there are embedded '\0's?
> >> >
> >> > The string will be shorter than expected? And what if it contains
> >> > non-printable characters?
> >> >
> >> > It's just a cautionary comment to help the user identify such problems, I think
> >> > we're making a whole argument about a quite minor thing.
> >> 
> >> When I see "assumes X" in a function comment, I immediately ask "and
> >> what happens when !X?"  The default answer is "it explodes, so don't do
> >> that".  That answer is wrong here.  Therefore, I find the comment
> >> misleading.
> >
> > That's how you interpret it, my interpretation is that I might not get
> > the expected behavior.
> 
> Actually, this function works just fine for embedded '\0's (I tested
> it): it returns the correct QString, with full length and '\0' embedded.

Good.

> Only later, when we attempt to put that QString on the wire do we screw
> up, in to_json().  It fails to consider the length, and stops at the
> first 0.  In fact, there's not even a way to get the length of a
> QString!  There's only qstring_get_str().  I'd call that an API bug.
> You might call it a restriction instead ;)

Whatever it is, let's do what has to be done: just add it.

> If anything needs a comment, it's qobject_to_json().  But I think that
> one needs a bug fix instead.

Care to send a patch then?

> Alternatively, we could document that QString and its users can't cope
> with embedded '\0'.

That depend on QString users, doesn't it?

> 
> >> Let's figure out what really happens.  The human command's output is
> >> sent to the client as a JSON string (response object member return).
> >> JSON strings can consist of Unicode characters, "except for the
> >> characters that must be escaped: quotation mark, reverse solidus, and
> >> the control characters (U+0000 through U+001F)" (RFC 4627, section 2.5).
> >> 
> >> Do we escape these characters?  Where in the code?
> >
> > Should be in the json parser, but qemu_chr_mem_to_qs() doesn't assume its
> > users (and it obviously shouldn't).
> 
> It's in to_json().
> 
> [...]
> 

[Qemu-devel] [PATCH 2/3] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino]

This command allows QMP clients to execute HMP commands.

Please, check the documentation added to the qmp-commands.hx file
for additional details about the interface and its limitations.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   38 ++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8cee35d..ec31eac 100644
--- a/monitor.c
+++ b/monitor.c
@@ -491,6 +491,44 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
     return 0;
 }
 
+static int mon_set_cpu(int cpu_index);
+static void handle_user_command(Monitor *mon, const char *cmdline);
+
+static int do_hmp_passthrough(Monitor *mon, const QDict *params,
+                              QObject **ret_data)
+{
+    int ret = 0;
+    Monitor *old_mon, hmp;
+    CharDriverState mchar;
+
+    memset(&hmp, 0, sizeof(hmp));
+    qemu_chr_init_mem(&mchar);
+    hmp.chr = &mchar;
+
+    old_mon = cur_mon;
+    cur_mon = &hmp;
+
+    if (qdict_haskey(params, "cpu-index")) {
+        ret = mon_set_cpu(qdict_get_int(params, "cpu-index"));
+        if (ret < 0) {
+            cur_mon = old_mon;
+            qerror_report(QERR_INVALID_PARAMETER_VALUE, "cpu-index", "a CPU number");
+            goto out;
+        }
+    }
+
+    handle_user_command(&hmp, qdict_get_str(params, "command-line"));
+    cur_mon = old_mon;
+
+    if (qemu_chr_mem_osize(hmp.chr) > 0) {
+        *ret_data = QOBJECT(qemu_chr_mem_to_qs(hmp.chr));
+    }
+
+out:
+    qemu_chr_close_mem(hmp.chr);
+    return ret;
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..e5f157f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -761,6 +761,51 @@ Example:
 
 Note: This command must be issued before issuing any other command.
 
+EQMP
+
+    {
+        .name       = "human-monitor-command",
+        .args_type  = "command-line:s,cpu-index:i?",
+        .params     = "",
+        .help       = "",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hmp_passthrough,
+    },
+
+SQMP
+human-monitor-command
+---------------------
+
+Execute a Human Monitor command.
+
+Arguments: 
+
+- command-line: the command name and its arguments, just like the
+                Human Monitor's shell (json-string)
+- cpu-index: select the CPU number to be used by commands which access CPU
+             data, like 'info registers'. The Monitor selects CPU 0 if this
+             argument is not provided (json-int, optional)
+
+Example:
+
+-> { "execute": "human-monitor-command", "arguments": { "command-line": "info kvm" } }
+<- { "return": "kvm support: enabled\r\n" }
+
+Notes:
+
+(1) The Human Monitor is NOT an stable interface, this means that command
+    names, arguments and responses can change or be removed at ANY time.
+    Applications that rely on long term stability guarantees should NOT
+    use this command
+
+(2) Limitations:
+
+    o This command is stateless, this means that commands that depend
+      on state information (such as getfd) might not work
+
+    o Commands that prompt the user for data (eg. 'cont' when the block
+      device is encrypted) don't currently work
+
 3. Query Commands
 =================
 
-- 
1.7.3.2.168.gd6b63