[Qemu-devel] [RFC v3 ATCH 0/5] char: expose CirMemCharDriver and provide QMP interface

[Qemu-devel] [RFC v3 ATCH 0/5] char: expose CirMemCharDriver and provide QMP interface

[Lei Li]

This RFC series attempts to convert the MemCharDriver to use a circular
buffer for input and output, expose it to users by introducing QMP commands
memchar_write and memchar_read and via the command line like the other
CharDriverStates.

Serial ports in qemu always use CharDriverStates as there backends,
Right now, all of our backends always try to write the data from the
guest to a socket or file. The concern from OpenStack is that this could
lead to unbounded disk space usage since they log the serial output.
For more detail of the background info:
https://bugs.launchpad.net/nova/+bug/832507

So we want to use a circular buffer in QEMU instead, and then OpenStack
can periodically read the buffer in QEMU and log it.

The QMP commands introduced like:

{ 'command': 'memchar-write',
  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
           'format': 'str', 'control': 'str' } }

{ 'command': 'memchar-read',
  'data': {'chardev': 'str', 'size': 'int',
           'format': 'str', 'control': 'str' },
  'returns': 'str' }

Expose CirMemCharDriver via the command line like:

qemu -chardev memchr,id=foo,maxcapacity=640k -serial chardev:foo

Introduce HMP command 'console' like:

(qemu) console foo

Note:
Now all of the feature were implemented, this series is just a
sketch and not completely tested. Please comment and let me know
if this seems like the direction we should be headed, your suggestion
would be very appreciated!

Known issues:
There are still some problems I am fixing,
  - The circularMemoryDriver need to be improved.
  - For the 'block' option as sync command, will support it later when
    we gain the necessary infrastructure.
  - The HMP command 'console' still have problems to be fixed. 

Changes since v2:
  - Add congestion mechanism. For the 'block' option as sync command,
    will support it later when we gain the necessary infrastructure
    enhancement.
  - Add HMP 'console' command so that can interact with multiple
    chardevs via a single monitor socket.
  - Make the circular buffer backend and the current MemCharDriver
    live in parallel, expose a new char backend with circular buffer
    CirMemCharDriver suggested by Luiz.
  - Other fixs from Eric and Markus.

Changes since v1:
  - Exposing the MemCharDriver via command line.
  - Support base64 data format suggested by Anthony and Eric.
  - Follow the new rule for the name of qmp command from Eric.


Lei Li (5):
  qemu-char: Add new char device CirMemCharDriver
  Expose CirMemCharDriver via command line
  QAPI: Introduce memchar-write QMP command
  QAPI: Introduce memchar-read QMP command
  HMP: Introduce console command

 hmp-commands.hx  |   72 ++++++++++++++++++
 hmp.c            |   79 ++++++++++++++++++++
 hmp.h            |    3 +
 monitor.c        |   18 +++++
 monitor.h        |    2 +
 qapi-schema.json |   96 ++++++++++++++++++++++++
 qemu-char.c      |  212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-config.c    |    3 +
 qemu-options.hx  |   10 +++
 qmp-commands.hx  |   76 +++++++++++++++++++
 10 files changed, 571 insertions(+), 0 deletions(-)

[Qemu-devel] [PATCH 1/5] qemu-char: Add new char device CirMemCharDriver

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qemu-char.c |   84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 84 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 767da93..0470085 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2591,6 +2591,90 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
     return d->outbuf_size;
 }
 
+/*********************************************************/
+/*CircularMemory chardev*/
+
+typedef struct {
+    size_t cbuf_capacity;
+    size_t cbuf_in;
+    size_t cbuf_out;
+    size_t cbuf_count;
+    uint8_t *cbuf;
+} CircMemCharDriver;
+
+static int cirmem_chr_is_empty(CharDriverState *chr)
+{
+    CircMemCharDriver *d = chr->opaque;
+
+    return d->cbuf_count == 0;
+}
+
+static int cirmem_chr_is_full(CharDriverState *chr)
+{
+    CircMemCharDriver *d = chr->opaque;
+
+    return d->cbuf_count == d->cbuf_capacity;
+}
+
+static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    CircMemCharDriver *d = chr->opaque;
+    int left;
+
+    if (d->cbuf_capacity < len) {
+        return -1;
+    }
+
+    left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity;
+
+    /* Some of cbuf need to be overwrited */
+    if (left < len) {
+        memcpy(d->cbuf + d->cbuf_in, buf, left);
+        memcpy(d->cbuf + d->cbuf_out, buf + left, len - left);
+        d->cbuf_out = (d->cbuf_out + len - left) % d->cbuf_capacity;
+        d->cbuf_count = d->cbuf_count + left;
+    } else {
+        /* Completely overwrite */
+        if (cirmem_chr_is_full(chr)) {
+            d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
+        } else {
+        /* Enough cbuf to write */
+        d->cbuf_count += len;
+        }
+        memcpy(d->cbuf + d->cbuf_in, buf, len);
+    }
+
+    d->cbuf_in = (d->cbuf_in + len) % d->cbuf_capacity;
+
+    return len;
+}
+
+static void cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
+{
+    CircMemCharDriver *d = chr->opaque;
+    int left;
+
+    if (cirmem_chr_is_empty(chr)) {
+        return;
+    }
+
+    left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity;
+
+    if (d->cbuf_capacity < len) {
+        len = d->cbuf_capacity;
+    }
+
+    if (left < len) {
+        memcpy(buf, d->cbuf + d->cbuf_out, left);
+        memcpy(buf + left, d->cbuf + d->cbuf_out + left, len - left);
+    } else {
+        memcpy(buf, d->cbuf + d->cbuf_out, len);
+    }
+
+    d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
+    d->cbuf_count -= len;
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/5] qemu-char: Add new char device CirMemCharDriver

[Luiz Capitulino]

On Wed, 12 Sep 2012 19:57:22 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qemu-char.c |   84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 84 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 767da93..0470085 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2591,6 +2591,90 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
>      return d->outbuf_size;
>  }
>  
> +/*********************************************************/
> +/*CircularMemory chardev*/
> +
> +typedef struct {
> +    size_t cbuf_capacity;
> +    size_t cbuf_in;
> +    size_t cbuf_out;
> +    size_t cbuf_count;
> +    uint8_t *cbuf;
> +} CircMemCharDriver;
> +
> +static int cirmem_chr_is_empty(CharDriverState *chr)
> +{
> +    CircMemCharDriver *d = chr->opaque;
> +
> +    return d->cbuf_count == 0;
> +}
> +
> +static int cirmem_chr_is_full(CharDriverState *chr)
> +{
> +    CircMemCharDriver *d = chr->opaque;
> +
> +    return d->cbuf_count == d->cbuf_capacity;
> +}

Both argument's from the functions above can be const.

> +
> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    CircMemCharDriver *d = chr->opaque;
> +    int left;
> +
> +    if (d->cbuf_capacity < len) {
> +        return -1;
> +    }
> +
> +    left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity;
> +
> +    /* Some of cbuf need to be overwrited */
> +    if (left < len) {
> +        memcpy(d->cbuf + d->cbuf_in, buf, left);
> +        memcpy(d->cbuf + d->cbuf_out, buf + left, len - left);
> +        d->cbuf_out = (d->cbuf_out + len - left) % d->cbuf_capacity;
> +        d->cbuf_count = d->cbuf_count + left;
> +    } else {
> +        /* Completely overwrite */
> +        if (cirmem_chr_is_full(chr)) {
> +            d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
> +        } else {
> +        /* Enough cbuf to write */
> +        d->cbuf_count += len;
> +        }
> +        memcpy(d->cbuf + d->cbuf_in, buf, len);
> +    }
> +
> +    d->cbuf_in = (d->cbuf_in + len) % d->cbuf_capacity;
> +
> +    return len;
> +}

I remember I made a few comments about using pointers instead
of indexes, did you consider doing that?

Btw, does this patch even build by its own? You're adding static
functions but no users.

> +
> +static void cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
> +{
> +    CircMemCharDriver *d = chr->opaque;
> +    int left;
> +
> +    if (cirmem_chr_is_empty(chr)) {
> +        return;
> +    }
> +
> +    left = d->cbuf_capacity - d->cbuf_count % d->cbuf_capacity;
> +
> +    if (d->cbuf_capacity < len) {
> +        len = d->cbuf_capacity;
> +    }
> +
> +    if (left < len) {
> +        memcpy(buf, d->cbuf + d->cbuf_out, left);
> +        memcpy(buf + left, d->cbuf + d->cbuf_out + left, len - left);
> +    } else {
> +        memcpy(buf, d->cbuf + d->cbuf_out, len);
> +    }
> +
> +    d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
> +    d->cbuf_count -= len;
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];

[Qemu-devel] [PATCH 2/5] Expose CirMemCharDriver via command line

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qemu-char.c     |   31 +++++++++++++++++++++++++++++++
 qemu-config.c   |    3 +++
 qemu-options.hx |   10 ++++++++++
 3 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 0470085..6e84acc 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2675,6 +2675,31 @@ static void cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
     d->cbuf_count -= len;
 }
 
+static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
+{
+    CharDriverState *chr;
+    CircMemCharDriver *d;
+
+    chr = g_malloc0(sizeof(CharDriverState));
+    d = g_malloc(sizeof(*d));
+
+    d->cbuf_capacity = qemu_opt_get_number(opts, "maxcapacity", 0);
+    if (d->cbuf_capacity == 0) {
+        d->cbuf_capacity = CBUFF_SIZE;
+    }
+
+    d->cbuf_in = 0;
+    d->cbuf_out = 0;
+    d->cbuf_count = 0;
+    d->cbuf = g_malloc0(d->cbuf_capacity);
+
+    memset(chr, 0, sizeof(*chr));
+    chr->opaque = d;
+    chr->chr_write = mem_chr_write;
+
+    return chr;
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
@@ -2739,6 +2764,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
         qemu_opt_set(opts, "path", p);
         return opts;
     }
+    if (strstart(filename, "memchr", &p)) {
+        qemu_opt_set(opts, "backend", "memchr");
+        qemu_opt_set(opts, "maxcapacity", p);
+        return opts;
+    }
     if (strstart(filename, "tcp:", &p) ||
         strstart(filename, "telnet:", &p)) {
         if (sscanf(p, "%64[^:]:%32[^,]%n", host, port, &pos) < 2) {
@@ -2812,6 +2842,7 @@ static const struct {
     { .name = "udp",       .open = qemu_chr_open_udp },
     { .name = "msmouse",   .open = qemu_chr_open_msmouse },
     { .name = "vc",        .open = text_console_init },
+    { .name = "memchr",    .open = qemu_chr_open_cirmemchr },
 #ifdef _WIN32
     { .name = "file",      .open = qemu_chr_open_win_file_out },
     { .name = "pipe",      .open = qemu_chr_open_win_pipe },
diff --git a/qemu-config.c b/qemu-config.c
index eba977e..5cb6dcb 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -213,6 +213,9 @@ static QemuOptsList qemu_chardev_opts = {
         },{
             .name = "debug",
             .type = QEMU_OPT_NUMBER,
+        },{
+            .name = "maxcapacity",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end of list */ }
     },
diff --git a/qemu-options.hx b/qemu-options.hx
index 804a2d1..3a7384d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1666,6 +1666,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
     "-chardev msmouse,id=id[,mux=on|off]\n"
     "-chardev vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]\n"
     "         [,mux=on|off]\n"
+    "-chardev memchr,id=id,maxcapacity=maxcapacity\n"
     "-chardev file,id=id,path=path[,mux=on|off]\n"
     "-chardev pipe,id=id,path=path[,mux=on|off]\n"
 #ifdef _WIN32
@@ -1704,6 +1705,7 @@ Backend is one of:
 @option{udp},
 @option{msmouse},
 @option{vc},
+@option{memchr},
 @option{file},
 @option{pipe},
 @option{console},
@@ -1810,6 +1812,14 @@ the console, in pixels.
 @option{cols} and @option{rows} specify that the console be sized to fit a text
 console with the given dimensions.
 
+@item -chardev memchr ,id=@var{id} ,maxcapacity=@var{maxcapacity}
+
+Create a circular buffer with fixed size indicated by optionally @option{maxcapacity}
+which will be default 64K if it is not given.
+
+@option{maxcapacity} specify the max capacity of the size of circular buffer
+want to create.
+
 @item -chardev file ,id=@var{id} ,path=@var{path}
 
 Log all traffic received from the guest to a file.
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/5] Expose CirMemCharDriver via command line

[Luiz Capitulino]

On Wed, 12 Sep 2012 19:57:23 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>

I think this patch should be merged with the previous one.

> ---
>  qemu-char.c     |   31 +++++++++++++++++++++++++++++++
>  qemu-config.c   |    3 +++
>  qemu-options.hx |   10 ++++++++++
>  3 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 0470085..6e84acc 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2675,6 +2675,31 @@ static void cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
>      d->cbuf_count -= len;
>  }
>  
> +static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
> +{
> +    CharDriverState *chr;
> +    CircMemCharDriver *d;
> +
> +    chr = g_malloc0(sizeof(CharDriverState));
> +    d = g_malloc(sizeof(*d));
> +
> +    d->cbuf_capacity = qemu_opt_get_number(opts, "maxcapacity", 0);
> +    if (d->cbuf_capacity == 0) {
> +        d->cbuf_capacity = CBUFF_SIZE;
> +    }
> +
> +    d->cbuf_in = 0;
> +    d->cbuf_out = 0;
> +    d->cbuf_count = 0;
> +    d->cbuf = g_malloc0(d->cbuf_capacity);
> +
> +    memset(chr, 0, sizeof(*chr));
> +    chr->opaque = d;
> +    chr->chr_write = mem_chr_write;
> +
> +    return chr;
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];
> @@ -2739,6 +2764,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>          qemu_opt_set(opts, "path", p);
>          return opts;
>      }
> +    if (strstart(filename, "memchr", &p)) {
> +        qemu_opt_set(opts, "backend", "memchr");
> +        qemu_opt_set(opts, "maxcapacity", p);
> +        return opts;

Is this really needed?

> +    }
>      if (strstart(filename, "tcp:", &p) ||
>          strstart(filename, "telnet:", &p)) {
>          if (sscanf(p, "%64[^:]:%32[^,]%n", host, port, &pos) < 2) {
> @@ -2812,6 +2842,7 @@ static const struct {
>      { .name = "udp",       .open = qemu_chr_open_udp },
>      { .name = "msmouse",   .open = qemu_chr_open_msmouse },
>      { .name = "vc",        .open = text_console_init },
> +    { .name = "memchr",    .open = qemu_chr_open_cirmemchr },
>  #ifdef _WIN32
>      { .name = "file",      .open = qemu_chr_open_win_file_out },
>      { .name = "pipe",      .open = qemu_chr_open_win_pipe },
> diff --git a/qemu-config.c b/qemu-config.c
> index eba977e..5cb6dcb 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -213,6 +213,9 @@ static QemuOptsList qemu_chardev_opts = {
>          },{
>              .name = "debug",
>              .type = QEMU_OPT_NUMBER,
> +        },{
> +            .name = "maxcapacity",
> +            .type = QEMU_OPT_NUMBER,
>          },
>          { /* end of list */ }
>      },
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 804a2d1..3a7384d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1666,6 +1666,7 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>      "-chardev msmouse,id=id[,mux=on|off]\n"
>      "-chardev vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]\n"
>      "         [,mux=on|off]\n"
> +    "-chardev memchr,id=id,maxcapacity=maxcapacity\n"
>      "-chardev file,id=id,path=path[,mux=on|off]\n"
>      "-chardev pipe,id=id,path=path[,mux=on|off]\n"
>  #ifdef _WIN32
> @@ -1704,6 +1705,7 @@ Backend is one of:
>  @option{udp},
>  @option{msmouse},
>  @option{vc},
> +@option{memchr},
>  @option{file},
>  @option{pipe},
>  @option{console},
> @@ -1810,6 +1812,14 @@ the console, in pixels.
>  @option{cols} and @option{rows} specify that the console be sized to fit a text
>  console with the given dimensions.
>  
> +@item -chardev memchr ,id=@var{id} ,maxcapacity=@var{maxcapacity}
> +
> +Create a circular buffer with fixed size indicated by optionally @option{maxcapacity}
> +which will be default 64K if it is not given.
> +
> +@option{maxcapacity} specify the max capacity of the size of circular buffer
> +want to create.
> +
>  @item -chardev file ,id=@var{id} ,path=@var{path}
>  
>  Log all traffic received from the guest to a file.

[Qemu-devel] [PATCH 3/5] QAPI: Introduce memchar-write QMP command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 hmp-commands.hx  |   23 ++++++++++++++++++
 hmp.c            |   19 +++++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-char.c      |   48 +++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   39 ++++++++++++++++++++++++++++++
 6 files changed, 199 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index ed67e99..fe11926 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -796,6 +796,29 @@ Inject an NMI on the given CPU (x86 only).
 ETEXI
 
     {
+        .name       = "memchar_write",
+        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
+        .params     = "chardev size data [format] [control]",
+        .help       = "Provide writing interface for CirMemCharDriver. Write"
+                      "'data' to it with size 'size'",
+        .mhandler.cmd = hmp_memchar_write,
+    },
+
+STEXI
+@item memchar_write @var{chardev} @var{size} @var{data} [@var{format}] [@var{control}]
+@findex memchar_write
+Provide writing interface for CirMemCharDriver. Write @var{data}
+to cirmemchr char device with size @var{size}.
+
+@var{format} is the format of the data write to CirMemCharDriver.
+Support 'utf8' and 'base64', by default is 'utf8'.
+
+@var{control} is option('block', 'drop') for read and write command
+that specifies behavior when the queue is full/empty. By default is
+'drop'. Note that the 'block' option is not supported now.
+ETEXI
+
+    {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
         .params     = "[-d] [-b] [-i] uri",
diff --git a/hmp.c b/hmp.c
index ba6fbd3..97f5058 100644
--- a/hmp.c
+++ b/hmp.c
@@ -671,6 +671,25 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_memchar_write(Monitor *mon, const QDict *qdict)
+{
+    uint32_t size = qdict_get_int(qdict, "size");
+    const char *chardev = qdict_get_str(qdict, "chardev");
+    const char *data = qdict_get_str(qdict, "data");
+    int val = qdict_get_try_bool(qdict, "base64", 0);
+    enum DataFormat format;
+    int con = qdict_get_try_bool(qdict, "block", 0);
+    enum CongestionControl control;
+    Error *errp = NULL;
+
+    format = val ? DATA_FORMAT_BASE64 : DATA_FORMAT_UTF8;
+    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
+    qmp_memchar_write(chardev, size, data, true, format,
+                      true, control, &errp);
+
+    hmp_handle_error(mon, &errp);
+}
+
 static void hmp_cont_cb(void *opaque, int err)
 {
     if (!err) {
diff --git a/hmp.h b/hmp.h
index 48b9c59..44b6463 100644
--- a/hmp.h
+++ b/hmp.h
@@ -43,6 +43,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
 void hmp_pmemsave(Monitor *mon, const QDict *qdict);
+void hmp_memchar_write(Monitor *mon, const QDict *qdict);
 void hmp_cont(Monitor *mon, const QDict *qdict);
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
 void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index a9f465a..371239a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -236,6 +236,75 @@
 { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
 
 ##
+# @DataFormat:
+#
+# An enumeration of data format write to or read from
+# memchardev. The default value would be utf8.
+#
+# @utf8: The data format is 'utf8'.
+#
+# @base64: The data format is 'base64'.
+#
+# Note: The data format start with 'utf8' and 'base64', will support
+#       other data format as well.
+#
+# Since: 1.3
+##
+{ 'enum': 'DataFormat'
+  'data': [ 'utf8', 'base64' ] }
+
+##
+# @CongestionControl
+#
+# An enumeration of options for the read and write command that
+# specifies behavior when the queue is full/empty. The default
+# option would be dropped.
+#
+# @drop: Would result in reads returning empty strings and writes
+#        dropping queued data.
+#
+# @block: Would make the session block until data was available
+#         or the queue had space available.
+#
+# Note: The option 'block' is not supported now due to the miss
+#       feature in qmp. Will add it later when we gain the necessary
+#       infrastructure enhancement.
+#
+# Since: 1.3
+##
+{'enum': 'CongestionControl'
+ 'data': [ 'drop', 'block' ] }
+
+##
+# @memchar-write:
+#
+# Provide writing interface for CirMemCharDriver. Write data to cirmemchar
+# char device.
+#
+# @chardev: the name of the cirmemchar char device.
+#
+# @size: the size to write in bytes.
+#
+# @data: the source data write to cirmemchar.
+#
+# @format: #optional the format of the data write to cirmemchar, by
+#          default is 'utf8'.
+#
+# @control: #optional options for read and write command that specifies
+#           behavior when the queue is full/empty.
+#
+# Returns: Nothing on success
+#          If @chardev is not a valid cirmemchr device, DeviceNotFound
+#          If an I/O error occurs while writing, IOError
+#
+# Since: 1.3
+##
+{ 'command': 'memchar-write',
+  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
+           '*format': 'DataFormat',
+           '*control': 'CongestionControl'} }
+
+##
 # @CommandInfo:
 #
 # Information about a QMP command
diff --git a/qemu-char.c b/qemu-char.c
index 6e84acc..be1d79a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2700,6 +2700,54 @@ static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
     return chr;
 }
 
+void qmp_memchar_write(const char *chardev, int64_t size,
+                       const char *data, bool has_format,
+                       enum DataFormat format, bool has_control,
+                       enum CongestionControl control,
+                       Error **errp)
+{
+    CharDriverState *chr;
+    guchar *write_data;
+    int ret;
+    gsize write_count;
+
+    chr = qemu_chr_find(chardev);
+
+    if(!chr) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
+        return;
+    }
+
+    /* XXX: For the sync command as 'block', waiting for the qmp
+ *      * to support necessary feature. Now just act as 'drop'. */
+    if (cirmem_chr_is_full(chr)) {
+        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
+            g_free((char *)data);
+            return;
+        } else {
+            g_free((char *)data);
+            error_setg(errp, "Failed to write to memchr %s", chardev);
+            return;
+        }
+    }
+
+    write_count = (gsize)size;
+
+    if (has_format && (format == DATA_FORMAT_BASE64)) {
+        write_data = g_base64_decode(data, &write_count);
+    } else {
+        write_data = (uint8_t *)data;
+    }
+
+    ret = cirmem_chr_write(chr, write_data, write_count);
+    /* */
+    if (ret <= 0) {
+        error_setg(errp, "Failed to write to memchr %s", chardev);
+    }
+
+    g_free(write_data);
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6e21ddb..5a1b86b 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -466,6 +466,45 @@ Note: inject-nmi fails when the guest doesn't support injecting.
 EQMP
 
     {
+        .name       = "memchar-write",
+        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
+        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
+    },
+
+SQMP
+memchar-write
+-------------
+
+Provide writing interface for CirMemCharDriver. Write data to cirmemchar
+char device.
+
+Arguments:
+
+- "chardev": the name of the char device, must be unique (json-string)
+- "size": the memory size, in bytes (json-int)
+- "data": the source data writed to cirmemchar (json-string)
+- "format": the data format write to CirMemCharDriver, default is
+            utf8. (json-string, optional)
+          - Possible values: "utf8", "base64"
+
+- "control": options for read and write command that specifies
+             behavior when the queue is full/empty, default is
+             drop. (json-string, optional)
+          - Possible values: "drop", "block"
+
+Example:
+
+-> { "execute": "memchar-write",
+                "arguments": { "chardev": foo,
+                               "size": 7,
+                               "data": "abcdefg",
+                               "format": "utf8",
+                               "control": "drop" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "xen-save-devices-state",
         .args_type  = "filename:F",
     .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 3/5] QAPI: Introduce memchar-write QMP command

[Blue Swirl]

On Wed, Sep 12, 2012 at 11:57 AM, Lei Li <lilei@linux.vnet.ibm.com> wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |   23 ++++++++++++++++++
>  hmp.c            |   19 +++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-char.c      |   48 +++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   39 ++++++++++++++++++++++++++++++
>  6 files changed, 199 insertions(+), 0 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed67e99..fe11926 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -796,6 +796,29 @@ Inject an NMI on the given CPU (x86 only).
>  ETEXI
>
>      {
> +        .name       = "memchar_write",
> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
> +        .params     = "chardev size data [format] [control]",
> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
> +                      "'data' to it with size 'size'",
> +        .mhandler.cmd = hmp_memchar_write,
> +    },
> +
> +STEXI
> +@item memchar_write @var{chardev} @var{size} @var{data} [@var{format}] [@var{control}]
> +@findex memchar_write
> +Provide writing interface for CirMemCharDriver. Write @var{data}
> +to cirmemchr char device with size @var{size}.
> +
> +@var{format} is the format of the data write to CirMemCharDriver.
> +Support 'utf8' and 'base64', by default is 'utf8'.
> +
> +@var{control} is option('block', 'drop') for read and write command
> +that specifies behavior when the queue is full/empty. By default is
> +'drop'. Note that the 'block' option is not supported now.
> +ETEXI
> +
> +    {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
>          .params     = "[-d] [-b] [-i] uri",
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..97f5058 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -671,6 +671,25 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>
> +void hmp_memchar_write(Monitor *mon, const QDict *qdict)
> +{
> +    uint32_t size = qdict_get_int(qdict, "size");
> +    const char *chardev = qdict_get_str(qdict, "chardev");
> +    const char *data = qdict_get_str(qdict, "data");
> +    int val = qdict_get_try_bool(qdict, "base64", 0);
> +    enum DataFormat format;
> +    int con = qdict_get_try_bool(qdict, "block", 0);
> +    enum CongestionControl control;
> +    Error *errp = NULL;
> +
> +    format = val ? DATA_FORMAT_BASE64 : DATA_FORMAT_UTF8;
> +    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
> +    qmp_memchar_write(chardev, size, data, true, format,
> +                      true, control, &errp);
> +
> +    hmp_handle_error(mon, &errp);
> +}
> +
>  static void hmp_cont_cb(void *opaque, int err)
>  {
>      if (!err) {
> diff --git a/hmp.h b/hmp.h
> index 48b9c59..44b6463 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -43,6 +43,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_memchar_write(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>  void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a9f465a..371239a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -236,6 +236,75 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>
>  ##
> +# @DataFormat:
> +#
> +# An enumeration of data format write to or read from
> +# memchardev. The default value would be utf8.
> +#
> +# @utf8: The data format is 'utf8'.
> +#
> +# @base64: The data format is 'base64'.
> +#
> +# Note: The data format start with 'utf8' and 'base64', will support
> +#       other data format as well.
> +#
> +# Since: 1.3
> +##
> +{ 'enum': 'DataFormat'
> +  'data': [ 'utf8', 'base64' ] }
> +
> +##
> +# @CongestionControl
> +#
> +# An enumeration of options for the read and write command that
> +# specifies behavior when the queue is full/empty. The default
> +# option would be dropped.
> +#
> +# @drop: Would result in reads returning empty strings and writes
> +#        dropping queued data.
> +#
> +# @block: Would make the session block until data was available
> +#         or the queue had space available.
> +#
> +# Note: The option 'block' is not supported now due to the miss
> +#       feature in qmp. Will add it later when we gain the necessary
> +#       infrastructure enhancement.
> +#
> +# Since: 1.3
> +##
> +{'enum': 'CongestionControl'
> + 'data': [ 'drop', 'block' ] }
> +
> +##
> +# @memchar-write:
> +#
> +# Provide writing interface for CirMemCharDriver. Write data to cirmemchar
> +# char device.
> +#
> +# @chardev: the name of the cirmemchar char device.
> +#
> +# @size: the size to write in bytes.
> +#
> +# @data: the source data write to cirmemchar.
> +#
> +# @format: #optional the format of the data write to cirmemchar, by
> +#          default is 'utf8'.
> +#
> +# @control: #optional options for read and write command that specifies
> +#           behavior when the queue is full/empty.
> +#
> +# Returns: Nothing on success
> +#          If @chardev is not a valid cirmemchr device, DeviceNotFound
> +#          If an I/O error occurs while writing, IOError
> +#
> +# Since: 1.3
> +##
> +{ 'command': 'memchar-write',
> +  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
> +           '*format': 'DataFormat',
> +           '*control': 'CongestionControl'} }
> +
> +##
>  # @CommandInfo:
>  #
>  # Information about a QMP command
> diff --git a/qemu-char.c b/qemu-char.c
> index 6e84acc..be1d79a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2700,6 +2700,54 @@ static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
>      return chr;
>  }
>
> +void qmp_memchar_write(const char *chardev, int64_t size,
> +                       const char *data, bool has_format,
> +                       enum DataFormat format, bool has_control,
> +                       enum CongestionControl control,
> +                       Error **errp)
> +{
> +    CharDriverState *chr;
> +    guchar *write_data;
> +    int ret;
> +    gsize write_count;
> +
> +    chr = qemu_chr_find(chardev);
> +
> +    if(!chr) {

Please add a space between 'if' and '('.

> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
> +        return;
> +    }
> +
> +    /* XXX: For the sync command as 'block', waiting for the qmp
> + *      * to support necessary feature. Now just act as 'drop'. */
> +    if (cirmem_chr_is_full(chr)) {
> +        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
> +            g_free((char *)data);
> +            return;
> +        } else {
> +            g_free((char *)data);
> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> +            return;
> +        }
> +    }
> +
> +    write_count = (gsize)size;
> +
> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
> +        write_data = g_base64_decode(data, &write_count);
> +    } else {
> +        write_data = (uint8_t *)data;
> +    }
> +
> +    ret = cirmem_chr_write(chr, write_data, write_count);
> +    /* */

Missing comment contents.

> +    if (ret <= 0) {
> +        error_setg(errp, "Failed to write to memchr %s", chardev);
> +    }
> +
> +    g_free(write_data);
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6e21ddb..5a1b86b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -466,6 +466,45 @@ Note: inject-nmi fails when the guest doesn't support injecting.
>  EQMP
>
>      {
> +        .name       = "memchar-write",
> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
> +    },
> +
> +SQMP
> +memchar-write
> +-------------
> +
> +Provide writing interface for CirMemCharDriver. Write data to cirmemchar
> +char device.
> +
> +Arguments:
> +
> +- "chardev": the name of the char device, must be unique (json-string)
> +- "size": the memory size, in bytes (json-int)
> +- "data": the source data writed to cirmemchar (json-string)
> +- "format": the data format write to CirMemCharDriver, default is
> +            utf8. (json-string, optional)
> +          - Possible values: "utf8", "base64"
> +
> +- "control": options for read and write command that specifies
> +             behavior when the queue is full/empty, default is
> +             drop. (json-string, optional)
> +          - Possible values: "drop", "block"
> +
> +Example:
> +
> +-> { "execute": "memchar-write",
> +                "arguments": { "chardev": foo,
> +                               "size": 7,
> +                               "data": "abcdefg",
> +                               "format": "utf8",
> +                               "control": "drop" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "xen-save-devices-state",
>          .args_type  = "filename:F",
>      .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
> --
> 1.7.7.6
>
>

Re: [Qemu-devel] [PATCH 3/5] QAPI: Introduce memchar-write QMP command

[Luiz Capitulino]

On Wed, 12 Sep 2012 19:57:24 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |   23 ++++++++++++++++++
>  hmp.c            |   19 +++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-char.c      |   48 +++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   39 ++++++++++++++++++++++++++++++
>  6 files changed, 199 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed67e99..fe11926 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -796,6 +796,29 @@ Inject an NMI on the given CPU (x86 only).
>  ETEXI
>  
>      {
> +        .name       = "memchar_write",
> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
> +        .params     = "chardev size data [format] [control]",
> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
> +                      "'data' to it with size 'size'",
> +        .mhandler.cmd = hmp_memchar_write,
> +    },

That's a too low level command for hmp. I'd actually consider dropping
it in favor of the console command, but if you really want to have this
then I suggest making the following changes:

 o drop 'size', as this can be calculated from the user input string
 o drop 'format', as base64 doesn't make much sense for hmp
 o turn 'control' into a flag, like -b for block and -d for drop

Also applies for memchar_read.

> +
> +STEXI
> +@item memchar_write @var{chardev} @var{size} @var{data} [@var{format}] [@var{control}]
> +@findex memchar_write
> +Provide writing interface for CirMemCharDriver. Write @var{data}
> +to cirmemchr char device with size @var{size}.
> +
> +@var{format} is the format of the data write to CirMemCharDriver.
> +Support 'utf8' and 'base64', by default is 'utf8'.
> +
> +@var{control} is option('block', 'drop') for read and write command
> +that specifies behavior when the queue is full/empty. By default is
> +'drop'. Note that the 'block' option is not supported now.
> +ETEXI
> +
> +    {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
>          .params     = "[-d] [-b] [-i] uri",
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..97f5058 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -671,6 +671,25 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>  
> +void hmp_memchar_write(Monitor *mon, const QDict *qdict)
> +{
> +    uint32_t size = qdict_get_int(qdict, "size");
> +    const char *chardev = qdict_get_str(qdict, "chardev");
> +    const char *data = qdict_get_str(qdict, "data");
> +    int val = qdict_get_try_bool(qdict, "base64", 0);
> +    enum DataFormat format;
> +    int con = qdict_get_try_bool(qdict, "block", 0);
> +    enum CongestionControl control;
> +    Error *errp = NULL;
> +
> +    format = val ? DATA_FORMAT_BASE64 : DATA_FORMAT_UTF8;
> +    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
> +    qmp_memchar_write(chardev, size, data, true, format,
> +                      true, control, &errp);
> +
> +    hmp_handle_error(mon, &errp);
> +}
> +
>  static void hmp_cont_cb(void *opaque, int err)
>  {
>      if (!err) {
> diff --git a/hmp.h b/hmp.h
> index 48b9c59..44b6463 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -43,6 +43,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_memchar_write(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>  void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a9f465a..371239a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -236,6 +236,75 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>  
>  ##
> +# @DataFormat:
> +#
> +# An enumeration of data format write to or read from
> +# memchardev. The default value would be utf8.
> +#
> +# @utf8: The data format is 'utf8'.
> +#
> +# @base64: The data format is 'base64'.
> +#
> +# Note: The data format start with 'utf8' and 'base64', will support
> +#       other data format as well.
> +#
> +# Since: 1.3
> +##
> +{ 'enum': 'DataFormat'
> +  'data': [ 'utf8', 'base64' ] }
> +
> +##
> +# @CongestionControl
> +#
> +# An enumeration of options for the read and write command that
> +# specifies behavior when the queue is full/empty. The default
> +# option would be dropped.
> +#
> +# @drop: Would result in reads returning empty strings and writes
> +#        dropping queued data.
> +#
> +# @block: Would make the session block until data was available
> +#         or the queue had space available.
> +#
> +# Note: The option 'block' is not supported now due to the miss
> +#       feature in qmp. Will add it later when we gain the necessary
> +#       infrastructure enhancement.

That's not acceptable. We either, hold the inclusion of this series 
until this is ready or we hardcode drop for now and add new commands
supporting @CongestionControl later.

> +#
> +# Since: 1.3
> +##
> +{'enum': 'CongestionControl'
> + 'data': [ 'drop', 'block' ] }
> +
> +##
> +# @memchar-write:
> +#
> +# Provide writing interface for CirMemCharDriver. Write data to cirmemchar
> +# char device.
> +#
> +# @chardev: the name of the cirmemchar char device.
> +#
> +# @size: the size to write in bytes.
> +#
> +# @data: the source data write to cirmemchar.
> +#
> +# @format: #optional the format of the data write to cirmemchar, by
> +#          default is 'utf8'.
> +#
> +# @control: #optional options for read and write command that specifies
> +#           behavior when the queue is full/empty.
> +#
> +# Returns: Nothing on success
> +#          If @chardev is not a valid cirmemchr device, DeviceNotFound
> +#          If an I/O error occurs while writing, IOError

This last line can be dropped.

> +#
> +# Since: 1.3
> +##
> +{ 'command': 'memchar-write',
> +  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
> +           '*format': 'DataFormat',

I don't think format should be optional.

> +           '*control': 'CongestionControl'} }
> +
> +##
>  # @CommandInfo:
>  #
>  # Information about a QMP command
> diff --git a/qemu-char.c b/qemu-char.c
> index 6e84acc..be1d79a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2700,6 +2700,54 @@ static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
>      return chr;
>  }
>  
> +void qmp_memchar_write(const char *chardev, int64_t size,
> +                       const char *data, bool has_format,
> +                       enum DataFormat format, bool has_control,
> +                       enum CongestionControl control,
> +                       Error **errp)
> +{
> +    CharDriverState *chr;
> +    guchar *write_data;
> +    int ret;
> +    gsize write_count;
> +
> +    chr = qemu_chr_find(chardev);
> +
> +    if(!chr) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
> +        return;
> +    }
> +
> +    /* XXX: For the sync command as 'block', waiting for the qmp
> + *      * to support necessary feature. Now just act as 'drop'. */
> +    if (cirmem_chr_is_full(chr)) {
> +        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
> +            g_free((char *)data);
> +            return;
> +        } else {
> +            g_free((char *)data);
> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> +            return;
> +        }
> +    }

Why do you free data above?

> +
> +    write_count = (gsize)size;
> +
> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
> +        write_data = g_base64_decode(data, &write_count);
> +    } else {
> +        write_data = (uint8_t *)data;
> +    }
> +
> +    ret = cirmem_chr_write(chr, write_data, write_count);
> +    /* */
> +    if (ret <= 0) {
> +        error_setg(errp, "Failed to write to memchr %s", chardev);
> +    }
> +
> +    g_free(write_data);
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6e21ddb..5a1b86b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -466,6 +466,45 @@ Note: inject-nmi fails when the guest doesn't support injecting.
>  EQMP
>  
>      {
> +        .name       = "memchar-write",
> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
> +    },
> +
> +SQMP
> +memchar-write
> +-------------
> +
> +Provide writing interface for CirMemCharDriver. Write data to cirmemchar
> +char device.
> +
> +Arguments:
> +
> +- "chardev": the name of the char device, must be unique (json-string)
> +- "size": the memory size, in bytes (json-int)
> +- "data": the source data writed to cirmemchar (json-string)
> +- "format": the data format write to CirMemCharDriver, default is
> +            utf8. (json-string, optional)
> +          - Possible values: "utf8", "base64"
> +
> +- "control": options for read and write command that specifies
> +             behavior when the queue is full/empty, default is
> +             drop. (json-string, optional)
> +          - Possible values: "drop", "block"
> +
> +Example:
> +
> +-> { "execute": "memchar-write",
> +                "arguments": { "chardev": foo,
> +                               "size": 7,
> +                               "data": "abcdefg",
> +                               "format": "utf8",
> +                               "control": "drop" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "xen-save-devices-state",
>          .args_type  = "filename:F",
>      .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,

Re: [Qemu-devel] [PATCH 3/5] QAPI: Introduce memchar-write QMP command

[Lei Li]

On 09/20/2012 02:05 AM, Luiz Capitulino wrote:
> On Wed, 12 Sep 2012 19:57:24 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx  |   23 ++++++++++++++++++
>>   hmp.c            |   19 +++++++++++++++
>>   hmp.h            |    1 +
>>   qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qemu-char.c      |   48 +++++++++++++++++++++++++++++++++++++
>>   qmp-commands.hx  |   39 ++++++++++++++++++++++++++++++
>>   6 files changed, 199 insertions(+), 0 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index ed67e99..fe11926 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -796,6 +796,29 @@ Inject an NMI on the given CPU (x86 only).
>>   ETEXI
>>   
>>       {
>> +        .name       = "memchar_write",
>> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
>> +        .params     = "chardev size data [format] [control]",
>> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
>> +                      "'data' to it with size 'size'",
>> +        .mhandler.cmd = hmp_memchar_write,
>> +    },
> That's a too low level command for hmp. I'd actually consider dropping
> it in favor of the console command, but if you really want to have this
> then I suggest making the following changes:
>
>   o drop 'size', as this can be calculated from the user input string
>   o drop 'format', as base64 doesn't make much sense for hmp
>   o turn 'control' into a flag, like -b for block and -d for drop
>
> Also applies for memchar_read.
>
>> +
>> +STEXI
>> +@item memchar_write @var{chardev} @var{size} @var{data} [@var{format}] [@var{control}]
>> +@findex memchar_write
>> +Provide writing interface for CirMemCharDriver. Write @var{data}
>> +to cirmemchr char device with size @var{size}.
>> +
>> +@var{format} is the format of the data write to CirMemCharDriver.
>> +Support 'utf8' and 'base64', by default is 'utf8'.
>> +
>> +@var{control} is option('block', 'drop') for read and write command
>> +that specifies behavior when the queue is full/empty. By default is
>> +'drop'. Note that the 'block' option is not supported now.
>> +ETEXI
>> +
>> +    {
>>           .name       = "migrate",
>>           .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
>>           .params     = "[-d] [-b] [-i] uri",
>> diff --git a/hmp.c b/hmp.c
>> index ba6fbd3..97f5058 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -671,6 +671,25 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
>>       hmp_handle_error(mon, &errp);
>>   }
>>   
>> +void hmp_memchar_write(Monitor *mon, const QDict *qdict)
>> +{
>> +    uint32_t size = qdict_get_int(qdict, "size");
>> +    const char *chardev = qdict_get_str(qdict, "chardev");
>> +    const char *data = qdict_get_str(qdict, "data");
>> +    int val = qdict_get_try_bool(qdict, "base64", 0);
>> +    enum DataFormat format;
>> +    int con = qdict_get_try_bool(qdict, "block", 0);
>> +    enum CongestionControl control;
>> +    Error *errp = NULL;
>> +
>> +    format = val ? DATA_FORMAT_BASE64 : DATA_FORMAT_UTF8;
>> +    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
>> +    qmp_memchar_write(chardev, size, data, true, format,
>> +                      true, control, &errp);
>> +
>> +    hmp_handle_error(mon, &errp);
>> +}
>> +
>>   static void hmp_cont_cb(void *opaque, int err)
>>   {
>>       if (!err) {
>> diff --git a/hmp.h b/hmp.h
>> index 48b9c59..44b6463 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -43,6 +43,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>>   void hmp_cpu(Monitor *mon, const QDict *qdict);
>>   void hmp_memsave(Monitor *mon, const QDict *qdict);
>>   void hmp_pmemsave(Monitor *mon, const QDict *qdict);
>> +void hmp_memchar_write(Monitor *mon, const QDict *qdict);
>>   void hmp_cont(Monitor *mon, const QDict *qdict);
>>   void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>>   void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index a9f465a..371239a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -236,6 +236,75 @@
>>   { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>>   
>>   ##
>> +# @DataFormat:
>> +#
>> +# An enumeration of data format write to or read from
>> +# memchardev. The default value would be utf8.
>> +#
>> +# @utf8: The data format is 'utf8'.
>> +#
>> +# @base64: The data format is 'base64'.
>> +#
>> +# Note: The data format start with 'utf8' and 'base64', will support
>> +#       other data format as well.
>> +#
>> +# Since: 1.3
>> +##
>> +{ 'enum': 'DataFormat'
>> +  'data': [ 'utf8', 'base64' ] }
>> +
>> +##
>> +# @CongestionControl
>> +#
>> +# An enumeration of options for the read and write command that
>> +# specifies behavior when the queue is full/empty. The default
>> +# option would be dropped.
>> +#
>> +# @drop: Would result in reads returning empty strings and writes
>> +#        dropping queued data.
>> +#
>> +# @block: Would make the session block until data was available
>> +#         or the queue had space available.
>> +#
>> +# Note: The option 'block' is not supported now due to the miss
>> +#       feature in qmp. Will add it later when we gain the necessary
>> +#       infrastructure enhancement.
> That's not acceptable. We either, hold the inclusion of this series
> until this is ready or we hardcode drop for now and add new commands
> supporting @CongestionControl later.
>
>> +#
>> +# Since: 1.3
>> +##
>> +{'enum': 'CongestionControl'
>> + 'data': [ 'drop', 'block' ] }
>> +
>> +##
>> +# @memchar-write:
>> +#
>> +# Provide writing interface for CirMemCharDriver. Write data to cirmemchar
>> +# char device.
>> +#
>> +# @chardev: the name of the cirmemchar char device.
>> +#
>> +# @size: the size to write in bytes.
>> +#
>> +# @data: the source data write to cirmemchar.
>> +#
>> +# @format: #optional the format of the data write to cirmemchar, by
>> +#          default is 'utf8'.
>> +#
>> +# @control: #optional options for read and write command that specifies
>> +#           behavior when the queue is full/empty.
>> +#
>> +# Returns: Nothing on success
>> +#          If @chardev is not a valid cirmemchr device, DeviceNotFound
>> +#          If an I/O error occurs while writing, IOError
> This last line can be dropped.
>
>> +#
>> +# Since: 1.3
>> +##
>> +{ 'command': 'memchar-write',
>> +  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
>> +           '*format': 'DataFormat',
> I don't think format should be optional.
>
>> +           '*control': 'CongestionControl'} }
>> +
>> +##
>>   # @CommandInfo:
>>   #
>>   # Information about a QMP command
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 6e84acc..be1d79a 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2700,6 +2700,54 @@ static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
>>       return chr;
>>   }
>>   
>> +void qmp_memchar_write(const char *chardev, int64_t size,
>> +                       const char *data, bool has_format,
>> +                       enum DataFormat format, bool has_control,
>> +                       enum CongestionControl control,
>> +                       Error **errp)
>> +{
>> +    CharDriverState *chr;
>> +    guchar *write_data;
>> +    int ret;
>> +    gsize write_count;
>> +
>> +    chr = qemu_chr_find(chardev);
>> +
>> +    if(!chr) {
>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
>> +        return;
>> +    }
>> +
>> +    /* XXX: For the sync command as 'block', waiting for the qmp
>> + *      * to support necessary feature. Now just act as 'drop'. */
>> +    if (cirmem_chr_is_full(chr)) {
>> +        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
>> +            g_free((char *)data);
>> +            return;
>> +        } else {
>> +            g_free((char *)data);
>> +            error_setg(errp, "Failed to write to memchr %s", chardev);
>> +            return;
>> +        }
>> +    }
> Why do you free data above?

Like the document motioned, we have two options { 'drop', 'block' } as congestion
mechanism suggested by Anthony, when set 'drop' would result in reads returning
empty strings and writes dropping queued data. Set 'block' then we could make the
session block until data was available or the queue had space available.
Now free the data for option 'block' too since we lack such infrastructure now,
will act as 'block' later when we gain the necessary infrastructure enhancement.

>> +
>> +    write_count = (gsize)size;
>> +
>> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
>> +        write_data = g_base64_decode(data, &write_count);
>> +    } else {
>> +        write_data = (uint8_t *)data;
>> +    }
>> +
>> +    ret = cirmem_chr_write(chr, write_data, write_count);
>> +    /* */
>> +    if (ret <= 0) {
>> +        error_setg(errp, "Failed to write to memchr %s", chardev);
>> +    }
>> +
>> +    g_free(write_data);
>> +}
>> +
>>   QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>>   {
>>       char host[65], port[33], width[8], height[8];
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 6e21ddb..5a1b86b 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -466,6 +466,45 @@ Note: inject-nmi fails when the guest doesn't support injecting.
>>   EQMP
>>   
>>       {
>> +        .name       = "memchar-write",
>> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
>> +        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
>> +    },
>> +
>> +SQMP
>> +memchar-write
>> +-------------
>> +
>> +Provide writing interface for CirMemCharDriver. Write data to cirmemchar
>> +char device.
>> +
>> +Arguments:
>> +
>> +- "chardev": the name of the char device, must be unique (json-string)
>> +- "size": the memory size, in bytes (json-int)
>> +- "data": the source data writed to cirmemchar (json-string)
>> +- "format": the data format write to CirMemCharDriver, default is
>> +            utf8. (json-string, optional)
>> +          - Possible values: "utf8", "base64"
>> +
>> +- "control": options for read and write command that specifies
>> +             behavior when the queue is full/empty, default is
>> +             drop. (json-string, optional)
>> +          - Possible values: "drop", "block"
>> +
>> +Example:
>> +
>> +-> { "execute": "memchar-write",
>> +                "arguments": { "chardev": foo,
>> +                               "size": 7,
>> +                               "data": "abcdefg",
>> +                               "format": "utf8",
>> +                               "control": "drop" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>>           .name       = "xen-save-devices-state",
>>           .args_type  = "filename:F",
>>       .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
>


-- 
Lei

Re: [Qemu-devel] [PATCH 3/5] QAPI: Introduce memchar-write QMP command

[Luiz Capitulino]

On Thu, 20 Sep 2012 15:42:30 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> On 09/20/2012 02:05 AM, Luiz Capitulino wrote:
> > On Wed, 12 Sep 2012 19:57:24 +0800
> > Lei Li <lilei@linux.vnet.ibm.com> wrote:
> >
> >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> >> ---
> >>   hmp-commands.hx  |   23 ++++++++++++++++++
> >>   hmp.c            |   19 +++++++++++++++
> >>   hmp.h            |    1 +
> >>   qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   qemu-char.c      |   48 +++++++++++++++++++++++++++++++++++++
> >>   qmp-commands.hx  |   39 ++++++++++++++++++++++++++++++
> >>   6 files changed, 199 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index ed67e99..fe11926 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -796,6 +796,29 @@ Inject an NMI on the given CPU (x86 only).
> >>   ETEXI
> >>   
> >>       {
> >> +        .name       = "memchar_write",
> >> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
> >> +        .params     = "chardev size data [format] [control]",
> >> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
> >> +                      "'data' to it with size 'size'",
> >> +        .mhandler.cmd = hmp_memchar_write,
> >> +    },
> > That's a too low level command for hmp. I'd actually consider dropping
> > it in favor of the console command, but if you really want to have this
> > then I suggest making the following changes:
> >
> >   o drop 'size', as this can be calculated from the user input string
> >   o drop 'format', as base64 doesn't make much sense for hmp
> >   o turn 'control' into a flag, like -b for block and -d for drop
> >
> > Also applies for memchar_read.
> >
> >> +
> >> +STEXI
> >> +@item memchar_write @var{chardev} @var{size} @var{data} [@var{format}] [@var{control}]
> >> +@findex memchar_write
> >> +Provide writing interface for CirMemCharDriver. Write @var{data}
> >> +to cirmemchr char device with size @var{size}.
> >> +
> >> +@var{format} is the format of the data write to CirMemCharDriver.
> >> +Support 'utf8' and 'base64', by default is 'utf8'.
> >> +
> >> +@var{control} is option('block', 'drop') for read and write command
> >> +that specifies behavior when the queue is full/empty. By default is
> >> +'drop'. Note that the 'block' option is not supported now.
> >> +ETEXI
> >> +
> >> +    {
> >>           .name       = "migrate",
> >>           .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> >>           .params     = "[-d] [-b] [-i] uri",
> >> diff --git a/hmp.c b/hmp.c
> >> index ba6fbd3..97f5058 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -671,6 +671,25 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
> >>       hmp_handle_error(mon, &errp);
> >>   }
> >>   
> >> +void hmp_memchar_write(Monitor *mon, const QDict *qdict)
> >> +{
> >> +    uint32_t size = qdict_get_int(qdict, "size");
> >> +    const char *chardev = qdict_get_str(qdict, "chardev");
> >> +    const char *data = qdict_get_str(qdict, "data");
> >> +    int val = qdict_get_try_bool(qdict, "base64", 0);
> >> +    enum DataFormat format;
> >> +    int con = qdict_get_try_bool(qdict, "block", 0);
> >> +    enum CongestionControl control;
> >> +    Error *errp = NULL;
> >> +
> >> +    format = val ? DATA_FORMAT_BASE64 : DATA_FORMAT_UTF8;
> >> +    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
> >> +    qmp_memchar_write(chardev, size, data, true, format,
> >> +                      true, control, &errp);
> >> +
> >> +    hmp_handle_error(mon, &errp);
> >> +}
> >> +
> >>   static void hmp_cont_cb(void *opaque, int err)
> >>   {
> >>       if (!err) {
> >> diff --git a/hmp.h b/hmp.h
> >> index 48b9c59..44b6463 100644
> >> --- a/hmp.h
> >> +++ b/hmp.h
> >> @@ -43,6 +43,7 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> >>   void hmp_cpu(Monitor *mon, const QDict *qdict);
> >>   void hmp_memsave(Monitor *mon, const QDict *qdict);
> >>   void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> >> +void hmp_memchar_write(Monitor *mon, const QDict *qdict);
> >>   void hmp_cont(Monitor *mon, const QDict *qdict);
> >>   void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
> >>   void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index a9f465a..371239a 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -236,6 +236,75 @@
> >>   { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
> >>   
> >>   ##
> >> +# @DataFormat:
> >> +#
> >> +# An enumeration of data format write to or read from
> >> +# memchardev. The default value would be utf8.
> >> +#
> >> +# @utf8: The data format is 'utf8'.
> >> +#
> >> +# @base64: The data format is 'base64'.
> >> +#
> >> +# Note: The data format start with 'utf8' and 'base64', will support
> >> +#       other data format as well.
> >> +#
> >> +# Since: 1.3
> >> +##
> >> +{ 'enum': 'DataFormat'
> >> +  'data': [ 'utf8', 'base64' ] }
> >> +
> >> +##
> >> +# @CongestionControl
> >> +#
> >> +# An enumeration of options for the read and write command that
> >> +# specifies behavior when the queue is full/empty. The default
> >> +# option would be dropped.
> >> +#
> >> +# @drop: Would result in reads returning empty strings and writes
> >> +#        dropping queued data.
> >> +#
> >> +# @block: Would make the session block until data was available
> >> +#         or the queue had space available.
> >> +#
> >> +# Note: The option 'block' is not supported now due to the miss
> >> +#       feature in qmp. Will add it later when we gain the necessary
> >> +#       infrastructure enhancement.
> > That's not acceptable. We either, hold the inclusion of this series
> > until this is ready or we hardcode drop for now and add new commands
> > supporting @CongestionControl later.
> >
> >> +#
> >> +# Since: 1.3
> >> +##
> >> +{'enum': 'CongestionControl'
> >> + 'data': [ 'drop', 'block' ] }
> >> +
> >> +##
> >> +# @memchar-write:
> >> +#
> >> +# Provide writing interface for CirMemCharDriver. Write data to cirmemchar
> >> +# char device.
> >> +#
> >> +# @chardev: the name of the cirmemchar char device.
> >> +#
> >> +# @size: the size to write in bytes.
> >> +#
> >> +# @data: the source data write to cirmemchar.
> >> +#
> >> +# @format: #optional the format of the data write to cirmemchar, by
> >> +#          default is 'utf8'.
> >> +#
> >> +# @control: #optional options for read and write command that specifies
> >> +#           behavior when the queue is full/empty.
> >> +#
> >> +# Returns: Nothing on success
> >> +#          If @chardev is not a valid cirmemchr device, DeviceNotFound
> >> +#          If an I/O error occurs while writing, IOError
> > This last line can be dropped.
> >
> >> +#
> >> +# Since: 1.3
> >> +##
> >> +{ 'command': 'memchar-write',
> >> +  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
> >> +           '*format': 'DataFormat',
> > I don't think format should be optional.
> >
> >> +           '*control': 'CongestionControl'} }
> >> +
> >> +##
> >>   # @CommandInfo:
> >>   #
> >>   # Information about a QMP command
> >> diff --git a/qemu-char.c b/qemu-char.c
> >> index 6e84acc..be1d79a 100644
> >> --- a/qemu-char.c
> >> +++ b/qemu-char.c
> >> @@ -2700,6 +2700,54 @@ static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
> >>       return chr;
> >>   }
> >>   
> >> +void qmp_memchar_write(const char *chardev, int64_t size,
> >> +                       const char *data, bool has_format,
> >> +                       enum DataFormat format, bool has_control,
> >> +                       enum CongestionControl control,
> >> +                       Error **errp)
> >> +{
> >> +    CharDriverState *chr;
> >> +    guchar *write_data;
> >> +    int ret;
> >> +    gsize write_count;
> >> +
> >> +    chr = qemu_chr_find(chardev);
> >> +
> >> +    if(!chr) {
> >> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
> >> +        return;
> >> +    }
> >> +
> >> +    /* XXX: For the sync command as 'block', waiting for the qmp
> >> + *      * to support necessary feature. Now just act as 'drop'. */
> >> +    if (cirmem_chr_is_full(chr)) {
> >> +        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
> >> +            g_free((char *)data);
> >> +            return;
> >> +        } else {
> >> +            g_free((char *)data);
> >> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> >> +            return;
> >> +        }
> >> +    }
> > Why do you free data above?
> 
> Like the document motioned, we have two options { 'drop', 'block' } as congestion
> mechanism suggested by Anthony, when set 'drop' would result in reads returning
> empty strings and writes dropping queued data. Set 'block' then we could make the
> session block until data was available or the queue had space available.
> Now free the data for option 'block' too since we lack such infrastructure now,
> will act as 'block' later when we gain the necessary infrastructure enhancement.

The qapi will free 'data' as soon as the command returns, you don't
have/need to do it by hand. You should have seen a double free while
testing this...

> 
> >> +
> >> +    write_count = (gsize)size;
> >> +
> >> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
> >> +        write_data = g_base64_decode(data, &write_count);
> >> +    } else {
> >> +        write_data = (uint8_t *)data;
> >> +    }
> >> +
> >> +    ret = cirmem_chr_write(chr, write_data, write_count);
> >> +    /* */
> >> +    if (ret <= 0) {
> >> +        error_setg(errp, "Failed to write to memchr %s", chardev);
> >> +    }
> >> +
> >> +    g_free(write_data);
> >> +}
> >> +
> >>   QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> >>   {
> >>       char host[65], port[33], width[8], height[8];
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index 6e21ddb..5a1b86b 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -466,6 +466,45 @@ Note: inject-nmi fails when the guest doesn't support injecting.
> >>   EQMP
> >>   
> >>       {
> >> +        .name       = "memchar-write",
> >> +        .args_type  = "chardev:s,size:i,data:s,format:s?,control:s?",
> >> +        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
> >> +    },
> >> +
> >> +SQMP
> >> +memchar-write
> >> +-------------
> >> +
> >> +Provide writing interface for CirMemCharDriver. Write data to cirmemchar
> >> +char device.
> >> +
> >> +Arguments:
> >> +
> >> +- "chardev": the name of the char device, must be unique (json-string)
> >> +- "size": the memory size, in bytes (json-int)
> >> +- "data": the source data writed to cirmemchar (json-string)
> >> +- "format": the data format write to CirMemCharDriver, default is
> >> +            utf8. (json-string, optional)
> >> +          - Possible values: "utf8", "base64"
> >> +
> >> +- "control": options for read and write command that specifies
> >> +             behavior when the queue is full/empty, default is
> >> +             drop. (json-string, optional)
> >> +          - Possible values: "drop", "block"
> >> +
> >> +Example:
> >> +
> >> +-> { "execute": "memchar-write",
> >> +                "arguments": { "chardev": foo,
> >> +                               "size": 7,
> >> +                               "data": "abcdefg",
> >> +                               "format": "utf8",
> >> +                               "control": "drop" } }
> >> +<- { "return": {} }
> >> +
> >> +EQMP
> >> +
> >> +    {
> >>           .name       = "xen-save-devices-state",
> >>           .args_type  = "filename:F",
> >>       .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
> >
> 
> 

[Qemu-devel] [PATCH 4/5] QAPI: Introduce memchar-read QMP command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 hmp-commands.hx  |   25 +++++++++++++++++++++++++
 hmp.c            |   18 ++++++++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   27 +++++++++++++++++++++++++++
 qemu-char.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   37 +++++++++++++++++++++++++++++++++++++
 6 files changed, 156 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index fe11926..1d2fccc 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -816,6 +816,31 @@ Support 'utf8' and 'base64', by default is 'utf8'.
 @var{control} is option('block', 'drop') for read and write command
 that specifies behavior when the queue is full/empty. By default is
 'drop'. Note that the 'block' option is not supported now.
+
+ETEXI
+
+    {
+        .name       = "memchar_read",
+        .args_type  = "chardev:s,size:i,format:s?,control:s?",
+        .params     = "chardev size [format] [control]",
+        .help       = "Provide read interface for CirMemCharDriver. Read from"
+                      "it and return 'size' of the data",
+        .mhandler.cmd = hmp_memchar_read,
+    },
+
+STEXI
+@item memchar_read @var{chardev} @var{size}
+@findex memchar_read
+Provide read interface for CirMemCharDriver. Read from cirmemchr
+char device and return @var{size} of the data.
+
+@var{format} is the format of the data read from CirMemCharDriver.
+Support 'utf8' and 'base64', by default is 'utf8'.
+
+@var{control} is option['block', 'drop'] for read and write command
+that specifies behavior when the queue is full/empty. By default is
+'drop'. Note that the 'block' option is not supported now.
+
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index 97f5058..4397981 100644
--- a/hmp.c
+++ b/hmp.c
@@ -690,6 +690,24 @@ void hmp_memchar_write(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_memchar_read(Monitor *mon, const QDict *qdict)
+{
+    uint32_t size = qdict_get_int(qdict, "size");
+    const char *chardev = qdict_get_str(qdict, "chardev");
+    char *data;
+    int val = qdict_get_try_bool(qdict, "base64", 0);
+    enum DataFormat format;
+    int con = qdict_get_try_bool(qdict, "block", 0);
+    enum CongestionControl control;
+    Error *errp = NULL;
+
+    format = val ? DATA_FORMAT_BASE64 : DATA_FORMAT_UTF8;
+    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
+    data = qmp_memchar_read(chardev, size, true, format,
+                            true, control, &errp);
+    monitor_printf(mon, "%s\n", data);
+}
+
 static void hmp_cont_cb(void *opaque, int err)
 {
     if (!err) {
diff --git a/hmp.h b/hmp.h
index 44b6463..ed2cda4 100644
--- a/hmp.h
+++ b/hmp.h
@@ -44,6 +44,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
 void hmp_pmemsave(Monitor *mon, const QDict *qdict);
 void hmp_memchar_write(Monitor *mon, const QDict *qdict);
+void hmp_memchar_read(Monitor *mon, const QDict *qdict);
 void hmp_cont(Monitor *mon, const QDict *qdict);
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
 void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index 371239a..5274b86 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -305,6 +305,33 @@
            '*control': 'CongestionControl'} }
 
 ##
+# @memchar-read:
+#
+# Provide read interface for CirMemCharDriver. Read from cirmemchar
+# char device and return the data.
+#
+# @chardev: the name of the cirmemchar char device.
+#
+# @size: the size to read in bytes.
+#
+# @format: #optional the format of the data want to read from
+#          CirMemCharDriver, by default is 'utf8'.
+#
+# @control: #optional options for read and write command that specifies
+#           behavior when the queue is full/empty.
+#
+# Returns: The data read from cirmemchar as string.
+#          If @chardev is not a valid memchr device, DeviceNotFound
+#          If an I/O error occurs while reading, IOError
+#
+# Since: 1.3
+##
+{ 'command': 'memchar-read',
+  'data': {'chardev': 'str', 'size': 'int', '*format': 'DataFormat',
+           '*control': 'CongestionControl'},
+           'returns': 'str' }
+
+##
 # @CommandInfo:
 #
 # Information about a QMP command
diff --git a/qemu-char.c b/qemu-char.c
index be1d79a..bb3ddb9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2748,6 +2748,54 @@ void qmp_memchar_write(const char *chardev, int64_t size,
     g_free(write_data);
 }
 
+char *qmp_memchar_read(const char *chardev, int64_t size,
+                       bool has_format, enum DataFormat format,
+                       bool has_control, enum CongestionControl control,
+                       Error **errp)
+{
+    CharDriverState *chr;
+    guchar *read_data;
+    char *data = NULL;
+
+    read_data = g_malloc0(size);
+
+    chr = qemu_chr_find(chardev);
+    if(!chr) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
+        return NULL;
+    }
+
+    /* XXX: For the sync command as 'block', waiting for the qmp
+ *  *      * to support necessary feature. Now just act as 'drop'. */
+    if (cirmem_chr_is_empty(chr)) {
+        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
+            g_free(data);
+            return NULL;
+        } else {
+            error_setg(errp, "Failed to read from memchr %s", chardev);
+            return NULL;
+        }
+    }
+
+    if (size == 0) {
+        size = CBUFF_SIZE;
+    }
+
+    cirmem_chr_read(chr, read_data, size);
+
+    if (has_format && (format == DATA_FORMAT_BASE64)) {
+       if (read_data) {
+           data = g_base64_encode(read_data, (size_t)size);
+       }
+    } else {
+        data = (char *)read_data;
+    }
+
+    g_free(read_data);
+
+    return data;
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5a1b86b..fab3e4e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -505,6 +505,43 @@ Example:
 EQMP
 
     {
+        .name       = "memchar-read",
+        .args_type  = "chardev:s,size:i",
+        .mhandler.cmd_new = qmp_marshal_input_memchar_read,
+    },
+
+SQMP
+memchar-read
+-------------
+
+Provide read interface for CirMemCharDriver. Read from cirmemchar
+char device and return the data.
+
+Arguments:
+
+- "chardev": the name of the char device, must be unique (json-string)
+- "size": the memory size in bytes, init size of the cirmemchar
+          by default (json-int)
+- "format": the data format write to CirMemCharDriver, default is
+            utf8. (json-string, optional)
+          - Possible values: "utf8", "base64"
+
+- "control": options for read and write command that specifies
+             behavior when the queue is full/empty, default is
+             drop. (json-string, optional)
+          - Possible values: "drop", "block"
+
+Example:
+
+-> { "execute": "memchar-read",
+                "arguments": { "chardev": foo,
+                               "size": 1000,
+                               "format": "utf8" }
+<- { "return": "data string..." }
+
+EQMP
+
+    {
         .name       = "xen-save-devices-state",
         .args_type  = "filename:F",
     .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 4/5] QAPI: Introduce memchar-read QMP command

[Eric Blake]

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

On 09/12/2012 05:57 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |   25 +++++++++++++++++++++++++
>  hmp.c            |   18 ++++++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   27 +++++++++++++++++++++++++++
>  qemu-char.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   37 +++++++++++++++++++++++++++++++++++++
>  6 files changed, 156 insertions(+), 0 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -305,6 +305,33 @@
>             '*control': 'CongestionControl'} }
>  
>  ##
> +# @memchar-read:
> +#
> +# Provide read interface for CirMemCharDriver. Read from cirmemchar
> +# char device and return the data.
> +#
> +# @chardev: the name of the cirmemchar char device.
> +#
> +# @size: the size to read in bytes.
> +#
> +# @format: #optional the format of the data want to read from
> +#          CirMemCharDriver, by default is 'utf8'.
> +#
> +# @control: #optional options for read and write command that specifies
> +#           behavior when the queue is full/empty.
> +#
> +# Returns: The data read from cirmemchar as string.
> +#          If @chardev is not a valid memchr device, DeviceNotFound
> +#          If an I/O error occurs while reading, IOError
> +#
> +# Since: 1.3
> +##
> +{ 'command': 'memchar-read',
> +  'data': {'chardev': 'str', 'size': 'int', '*format': 'DataFormat',
> +           '*control': 'CongestionControl'},
> +           'returns': 'str' }

What happens if the data to be read contains embedded NUL, but the
requested 'format' can't express that?  What happens if there is less
data available than the maximum requested size?  I'm wondering if the
return should be a JSON struct, { 'data':'str', 'size':'int' }, in order
to allow for the case of short read returns.

> +- "chardev": the name of the char device, must be unique (json-string)
> +- "size": the memory size in bytes, init size of the cirmemchar
> +          by default (json-int)
> +- "format": the data format write to CirMemCharDriver, default is
> +            utf8. (json-string, optional)
> +          - Possible values: "utf8", "base64"

Also, you probably want to make it crystal-clear whether size is
referring to the unencoded size of the raw data, or the encoded size
after conversion to utf8 or base64.

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


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

Re: [Qemu-devel] [PATCH 4/5] QAPI: Introduce memchar-read QMP command

[Blue Swirl]

On Wed, Sep 12, 2012 at 11:57 AM, Lei Li <lilei@linux.vnet.ibm.com> wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |   25 +++++++++++++++++++++++++
>  hmp.c            |   18 ++++++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   27 +++++++++++++++++++++++++++
>  qemu-char.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   37 +++++++++++++++++++++++++++++++++++++
>  6 files changed, 156 insertions(+), 0 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index fe11926..1d2fccc 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -816,6 +816,31 @@ Support 'utf8' and 'base64', by default is 'utf8'.
>  @var{control} is option('block', 'drop') for read and write command
>  that specifies behavior when the queue is full/empty. By default is
>  'drop'. Note that the 'block' option is not supported now.
> +
> +ETEXI
> +
> +    {
> +        .name       = "memchar_read",
> +        .args_type  = "chardev:s,size:i,format:s?,control:s?",
> +        .params     = "chardev size [format] [control]",
> +        .help       = "Provide read interface for CirMemCharDriver. Read from"
> +                      "it and return 'size' of the data",
> +        .mhandler.cmd = hmp_memchar_read,
> +    },
> +
> +STEXI
> +@item memchar_read @var{chardev} @var{size}
> +@findex memchar_read
> +Provide read interface for CirMemCharDriver. Read from cirmemchr
> +char device and return @var{size} of the data.
> +
> +@var{format} is the format of the data read from CirMemCharDriver.
> +Support 'utf8' and 'base64', by default is 'utf8'.
> +
> +@var{control} is option['block', 'drop'] for read and write command
> +that specifies behavior when the queue is full/empty. By default is
> +'drop'. Note that the 'block' option is not supported now.
> +
>  ETEXI
>
>      {
> diff --git a/hmp.c b/hmp.c
> index 97f5058..4397981 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -690,6 +690,24 @@ void hmp_memchar_write(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>
> +void hmp_memchar_read(Monitor *mon, const QDict *qdict)
> +{
> +    uint32_t size = qdict_get_int(qdict, "size");
> +    const char *chardev = qdict_get_str(qdict, "chardev");
> +    char *data;
> +    int val = qdict_get_try_bool(qdict, "base64", 0);
> +    enum DataFormat format;
> +    int con = qdict_get_try_bool(qdict, "block", 0);
> +    enum CongestionControl control;
> +    Error *errp = NULL;
> +
> +    format = val ? DATA_FORMAT_BASE64 : DATA_FORMAT_UTF8;
> +    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
> +    data = qmp_memchar_read(chardev, size, true, format,
> +                            true, control, &errp);
> +    monitor_printf(mon, "%s\n", data);
> +}
> +
>  static void hmp_cont_cb(void *opaque, int err)
>  {
>      if (!err) {
> diff --git a/hmp.h b/hmp.h
> index 44b6463..ed2cda4 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -44,6 +44,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
>  void hmp_memchar_write(Monitor *mon, const QDict *qdict);
> +void hmp_memchar_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>  void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 371239a..5274b86 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -305,6 +305,33 @@
>             '*control': 'CongestionControl'} }
>
>  ##
> +# @memchar-read:
> +#
> +# Provide read interface for CirMemCharDriver. Read from cirmemchar
> +# char device and return the data.
> +#
> +# @chardev: the name of the cirmemchar char device.
> +#
> +# @size: the size to read in bytes.
> +#
> +# @format: #optional the format of the data want to read from
> +#          CirMemCharDriver, by default is 'utf8'.
> +#
> +# @control: #optional options for read and write command that specifies
> +#           behavior when the queue is full/empty.
> +#
> +# Returns: The data read from cirmemchar as string.
> +#          If @chardev is not a valid memchr device, DeviceNotFound
> +#          If an I/O error occurs while reading, IOError
> +#
> +# Since: 1.3
> +##
> +{ 'command': 'memchar-read',
> +  'data': {'chardev': 'str', 'size': 'int', '*format': 'DataFormat',
> +           '*control': 'CongestionControl'},
> +           'returns': 'str' }
> +
> +##
>  # @CommandInfo:
>  #
>  # Information about a QMP command
> diff --git a/qemu-char.c b/qemu-char.c
> index be1d79a..bb3ddb9 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2748,6 +2748,54 @@ void qmp_memchar_write(const char *chardev, int64_t size,
>      g_free(write_data);
>  }
>
> +char *qmp_memchar_read(const char *chardev, int64_t size,
> +                       bool has_format, enum DataFormat format,
> +                       bool has_control, enum CongestionControl control,
> +                       Error **errp)
> +{
> +    CharDriverState *chr;
> +    guchar *read_data;
> +    char *data = NULL;
> +
> +    read_data = g_malloc0(size);
> +
> +    chr = qemu_chr_find(chardev);
> +    if(!chr) {

Space between 'if' and '('

> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
> +        return NULL;
> +    }
> +
> +    /* XXX: For the sync command as 'block', waiting for the qmp
> + *  *      * to support necessary feature. Now just act as 'drop'. */

Wrong indentation, * * looks spurious.

> +    if (cirmem_chr_is_empty(chr)) {
> +        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
> +            g_free(data);
> +            return NULL;
> +        } else {
> +            error_setg(errp, "Failed to read from memchr %s", chardev);
> +            return NULL;
> +        }
> +    }
> +
> +    if (size == 0) {
> +        size = CBUFF_SIZE;
> +    }
> +
> +    cirmem_chr_read(chr, read_data, size);
> +
> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
> +       if (read_data) {
> +           data = g_base64_encode(read_data, (size_t)size);
> +       }
> +    } else {
> +        data = (char *)read_data;
> +    }
> +
> +    g_free(read_data);
> +
> +    return data;
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 5a1b86b..fab3e4e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -505,6 +505,43 @@ Example:
>  EQMP
>
>      {
> +        .name       = "memchar-read",
> +        .args_type  = "chardev:s,size:i",
> +        .mhandler.cmd_new = qmp_marshal_input_memchar_read,
> +    },
> +
> +SQMP
> +memchar-read
> +-------------
> +
> +Provide read interface for CirMemCharDriver. Read from cirmemchar
> +char device and return the data.
> +
> +Arguments:
> +
> +- "chardev": the name of the char device, must be unique (json-string)
> +- "size": the memory size in bytes, init size of the cirmemchar
> +          by default (json-int)
> +- "format": the data format write to CirMemCharDriver, default is
> +            utf8. (json-string, optional)
> +          - Possible values: "utf8", "base64"
> +
> +- "control": options for read and write command that specifies
> +             behavior when the queue is full/empty, default is
> +             drop. (json-string, optional)
> +          - Possible values: "drop", "block"
> +
> +Example:
> +
> +-> { "execute": "memchar-read",
> +                "arguments": { "chardev": foo,
> +                               "size": 1000,
> +                               "format": "utf8" }
> +<- { "return": "data string..." }
> +
> +EQMP
> +
> +    {
>          .name       = "xen-save-devices-state",
>          .args_type  = "filename:F",
>      .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
> --
> 1.7.7.6
>
>

[Qemu-devel] [PATCH 5/5] HMP: Introduce console command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 hmp.c     |   42 ++++++++++++++++++++++++++++++++++++++++++
 monitor.c |   18 ++++++++++++++++++
 monitor.h |    2 ++
 3 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/hmp.c b/hmp.c
index 4397981..a016a5c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1205,3 +1205,45 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
     qmp_screendump(filename, &err);
     hmp_handle_error(mon, &err);
 }
+
+int console_escape_char = 0x1d; /* ctrl-] is used for escape */
+
+static void hmp_read_console(Monitor *mon, const char *data,
+                             void *opaque)
+{
+    CharDriverState *chr = opaque;
+    uint32_t size = strlen(data);
+    enum DataFormat format = DATA_FORMAT_UTF8;
+    enum CongestionControl control = CONGESTION_CONTROL_DROP;
+
+    Error *err = NULL;
+
+    if (*data == console_escape_char) {
+        monitor_resume(mon);
+        return;
+    }
+
+    qmp_memchar_write(chr->label, size, data, 0, format,
+                      0, control, &err);
+    monitor_read_command(mon, 1);
+}
+
+void hmp_console(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "chardev");
+    CharDriverState *chr;
+    Error *err = NULL;
+
+    chr = qemu_chr_find(device);
+
+    if (!chr) {
+        error_set(&err, QERR_DEVICE_NOT_FOUND, device);
+        hmp_handle_error(mon, &err);
+        return;
+    }
+
+    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
+        monitor_printf(mon, "Connect to console %s failed\n", device);
+    }
+    g_free(chr);
+}
diff --git a/monitor.c b/monitor.c
index 67064e2..285dc7b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -256,6 +256,24 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
     }
 }
 
+int monitor_read_console(Monitor *mon, const char *device,
+                         ReadLineFunc *readline_func, void *opaque)
+{
+    char prompt[60];
+
+    if (!mon->rs)
+        return -1;
+
+    if (monitor_ctrl_mode(mon)) {
+        qerror_report(QERR_MISSING_PARAMETER, "console");
+        return -EINVAL;
+    }
+
+    snprintf(prompt, sizeof(prompt), "%s: ", device);
+    readline_start(mon->rs, prompt, 0, readline_func, opaque);
+    return 0;
+}
+
 void monitor_flush(Monitor *mon)
 {
     if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
diff --git a/monitor.h b/monitor.h
index 64c1561..924a042 100644
--- a/monitor.h
+++ b/monitor.h
@@ -84,6 +84,8 @@ void monitor_read_command(Monitor *mon, int show_prompt);
 ReadLineState *monitor_get_rs(Monitor *mon);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque);
+int monitor_read_console(Monitor *mon, const char *device,
+                         ReadLineFunc *readline_func, void *opaque);
 
 int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 5/5] HMP: Introduce console command

[Blue Swirl]

On Wed, Sep 12, 2012 at 11:57 AM, Lei Li <lilei@linux.vnet.ibm.com> wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp.c     |   42 ++++++++++++++++++++++++++++++++++++++++++
>  monitor.c |   18 ++++++++++++++++++
>  monitor.h |    2 ++
>  3 files changed, 62 insertions(+), 0 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 4397981..a016a5c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1205,3 +1205,45 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>      qmp_screendump(filename, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +int console_escape_char = 0x1d; /* ctrl-] is used for escape */

'static', also 'const' if it's not changed (I didn't spot where it
would be). This is also compared against 'char', so that would be a
better choice here too. But using an enum could generate better code.

If we supported several monitor connections, each with different
escape characters, this should be put to some console data structure
instead.

> +
> +static void hmp_read_console(Monitor *mon, const char *data,
> +                             void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    uint32_t size = strlen(data);
> +    enum DataFormat format = DATA_FORMAT_UTF8;
> +    enum CongestionControl control = CONGESTION_CONTROL_DROP;
> +
> +    Error *err = NULL;
> +
> +    if (*data == console_escape_char) {
> +        monitor_resume(mon);
> +        return;
> +    }
> +
> +    qmp_memchar_write(chr->label, size, data, 0, format,
> +                      0, control, &err);
> +    monitor_read_command(mon, 1);
> +}
> +
> +void hmp_console(Monitor *mon, const QDict *qdict)
> +{
> +    const char *device = qdict_get_str(qdict, "chardev");
> +    CharDriverState *chr;
> +    Error *err = NULL;
> +
> +    chr = qemu_chr_find(device);
> +
> +    if (!chr) {
> +        error_set(&err, QERR_DEVICE_NOT_FOUND, device);
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }
> +
> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
> +        monitor_printf(mon, "Connect to console %s failed\n", device);
> +    }
> +    g_free(chr);
> +}
> diff --git a/monitor.c b/monitor.c
> index 67064e2..285dc7b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -256,6 +256,24 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>      }
>  }
>
> +int monitor_read_console(Monitor *mon, const char *device,
> +                         ReadLineFunc *readline_func, void *opaque)
> +{
> +    char prompt[60];
> +
> +    if (!mon->rs)
> +        return -1;

Missing braces, please read CODING_STYLE.

> +
> +    if (monitor_ctrl_mode(mon)) {
> +        qerror_report(QERR_MISSING_PARAMETER, "console");
> +        return -EINVAL;
> +    }
> +
> +    snprintf(prompt, sizeof(prompt), "%s: ", device);
> +    readline_start(mon->rs, prompt, 0, readline_func, opaque);
> +    return 0;
> +}
> +
>  void monitor_flush(Monitor *mon)
>  {
>      if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
> diff --git a/monitor.h b/monitor.h
> index 64c1561..924a042 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -84,6 +84,8 @@ void monitor_read_command(Monitor *mon, int show_prompt);
>  ReadLineState *monitor_get_rs(Monitor *mon);
>  int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>                            void *opaque);
> +int monitor_read_console(Monitor *mon, const char *device,
> +                         ReadLineFunc *readline_func, void *opaque);
>
>  int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
>
> --
> 1.7.7.6
>
>

Re: [Qemu-devel] [PATCH 5/5] HMP: Introduce console command

[Luiz Capitulino]

On Wed, 12 Sep 2012 19:57:26 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp.c     |   42 ++++++++++++++++++++++++++++++++++++++++++
>  monitor.c |   18 ++++++++++++++++++
>  monitor.h |    2 ++
>  3 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 4397981..a016a5c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1205,3 +1205,45 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>      qmp_screendump(filename, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +int console_escape_char = 0x1d; /* ctrl-] is used for escape */
> +
> +static void hmp_read_console(Monitor *mon, const char *data,
> +                             void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    uint32_t size = strlen(data);
> +    enum DataFormat format = DATA_FORMAT_UTF8;
> +    enum CongestionControl control = CONGESTION_CONTROL_DROP;
> +
> +    Error *err = NULL;
> +
> +    if (*data == console_escape_char) {
> +        monitor_resume(mon);
> +        return;
> +    }
> +
> +    qmp_memchar_write(chr->label, size, data, 0, format,
> +                      0, control, &err);

You should print the error to the user.

> +    monitor_read_command(mon, 1);
> +}
> +
> +void hmp_console(Monitor *mon, const QDict *qdict)
> +{
> +    const char *device = qdict_get_str(qdict, "chardev");
> +    CharDriverState *chr;
> +    Error *err = NULL;
> +
> +    chr = qemu_chr_find(device);
> +
> +    if (!chr) {
> +        error_set(&err, QERR_DEVICE_NOT_FOUND, device);
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }

Don't both commands (memchr_read/write) already do this? Why don't you
just pass 'device' to them and let them fail if it doesn't exist?

> +
> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
> +        monitor_printf(mon, "Connect to console %s failed\n", device);
> +    }
> +    g_free(chr);
> +}
> diff --git a/monitor.c b/monitor.c
> index 67064e2..285dc7b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -256,6 +256,24 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>      }
>  }
>  
> +int monitor_read_console(Monitor *mon, const char *device,
> +                         ReadLineFunc *readline_func, void *opaque)
> +{
> +    char prompt[60];
> +
> +    if (!mon->rs)
> +        return -1;
> +
> +    if (monitor_ctrl_mode(mon)) {
> +        qerror_report(QERR_MISSING_PARAMETER, "console");
> +        return -EINVAL;
> +    }

That check doesn't make sense.

> +
> +    snprintf(prompt, sizeof(prompt), "%s: ", device);
> +    readline_start(mon->rs, prompt, 0, readline_func, opaque);
> +    return 0;
> +}
> +
>  void monitor_flush(Monitor *mon)
>  {
>      if (mon && mon->outbuf_index != 0 && !mon->mux_out) {
> diff --git a/monitor.h b/monitor.h
> index 64c1561..924a042 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -84,6 +84,8 @@ void monitor_read_command(Monitor *mon, int show_prompt);
>  ReadLineState *monitor_get_rs(Monitor *mon);
>  int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>                            void *opaque);
> +int monitor_read_console(Monitor *mon, const char *device,
> +                         ReadLineFunc *readline_func, void *opaque);
>  
>  int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
>  

Re: [Qemu-devel] [RFC v3 ATCH 0/5] char: expose CirMemCharDriver and provide QMP interface

[Avi Kivity]

On 09/12/2012 02:57 PM, Lei Li wrote:
> This RFC series attempts to convert the MemCharDriver to use a circular
> buffer for input and output, expose it to users by introducing QMP commands
> memchar_write and memchar_read and via the command line like the other
> CharDriverStates.
> 
> Serial ports in qemu always use CharDriverStates as there backends,
> Right now, all of our backends always try to write the data from the
> guest to a socket or file. The concern from OpenStack is that this could
> lead to unbounded disk space usage since they log the serial output.
> For more detail of the background info:
> https://bugs.launchpad.net/nova/+bug/832507
> 
> So we want to use a circular buffer in QEMU instead, and then OpenStack
> can periodically read the buffer in QEMU and log it.

Can't they do it themselves?  Have qemu write to a pipe, and on the
other side, do whatever rate limiting is needed.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RFC v3 ATCH 0/5] char: expose CirMemCharDriver and provide QMP interface

[Daniel P. Berrange]

On Wed, Sep 12, 2012 at 07:57:21PM +0800, Lei Li wrote:
> This RFC series attempts to convert the MemCharDriver to use a circular
> buffer for input and output, expose it to users by introducing QMP commands
> memchar_write and memchar_read and via the command line like the other
> CharDriverStates.
> 
> Serial ports in qemu always use CharDriverStates as there backends,
> Right now, all of our backends always try to write the data from the
> guest to a socket or file. The concern from OpenStack is that this could
> lead to unbounded disk space usage since they log the serial output.
> For more detail of the background info:
> https://bugs.launchpad.net/nova/+bug/832507

Unbounded disk usage is only relevant if telling QEMU to write directly
to its file backend. If you use a socket backend the mgmt app can provide
whatever policy it desires.

> So we want to use a circular buffer in QEMU instead, and then OpenStack
> can periodically read the buffer in QEMU and log it.

With any circular buffer you obviously have a race condition where
the buffer may overflow before the application can consume the data.
By implementing the circular buffer in QEMU you are imposing a
specific policy for overflow on the applications using QEMU, namely
that data gets overwritten/lost.

If the circular buffering is done outside QEMU, then the application
can implement a variety of policies on overflow. At the very least
they can detect when overflow would occur, and insert a marker to
the effect that there is a log discontinuity. Or they can pause the
VM for a period of time, or reduce its scheduling priority, or any
number of different things.

The further advantage of doing it outside QEMU, is that OpenStack will
work with all existing QEMU/KVM/libvirt versions.

I think most of the discussion in the quoted OpenStack bug is rather
short sighted only focusing on the immediate needs for their current
'get_log_output' API. I don't think this will be satisfactory for
OpenStack in the medium-long term. IMHO OpenStack needs to provide
a more comprensive logging capability than what is does today, and
thus this proposed QEMU feature would have a short lifetime of usefulness
to OpenStack. I've already recommended that OpenStack take advantage
of conserver to setup logging of all VMs, though there were some
issues around that. It is entirely possible for OpenStack to provide
its own logging system to process VM logs into fixed size files, as
well as satisfying the needs of its get_log_output API for circular
buffers.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [RFC v3 ATCH 0/5] char: expose CirMemCharDriver and provide QMP interface

[Anthony Liguori]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Wed, Sep 12, 2012 at 07:57:21PM +0800, Lei Li wrote:
>> This RFC series attempts to convert the MemCharDriver to use a circular
>> buffer for input and output, expose it to users by introducing QMP commands
>> memchar_write and memchar_read and via the command line like the other
>> CharDriverStates.
>> 
>> Serial ports in qemu always use CharDriverStates as there backends,
>> Right now, all of our backends always try to write the data from the
>> guest to a socket or file. The concern from OpenStack is that this could
>> lead to unbounded disk space usage since they log the serial output.
>> For more detail of the background info:
>> https://bugs.launchpad.net/nova/+bug/832507
>
> Unbounded disk usage is only relevant if telling QEMU to write directly
> to its file backend. If you use a socket backend the mgmt app can provide
> whatever policy it desires.
>
>> So we want to use a circular buffer in QEMU instead, and then OpenStack
>> can periodically read the buffer in QEMU and log it.
>
> With any circular buffer you obviously have a race condition where
> the buffer may overflow before the application can consume the data.
> By implementing the circular buffer in QEMU you are imposing a
> specific policy for overflow on the applications using QEMU, namely
> that data gets overwritten/lost.
>
> If the circular buffering is done outside QEMU, then the application
> can implement a variety of policies on overflow. At the very least
> they can detect when overflow would occur, and insert a marker to
> the effect that there is a log discontinuity. Or they can pause the
> VM for a period of time, or reduce its scheduling priority, or any
> number of different things.
>
> The further advantage of doing it outside QEMU, is that OpenStack will
> work with all existing QEMU/KVM/libvirt versions.

I'm not sure what is the best solution for libvirt and OpenStack but I
think you're missing a few key points.

CDS doesn't propagate flow control to the guest (in some cases, it
simply can't because hardware doesn't).  That means that there has to be
some policy in QEMU about what to do with data that cannot be written to
a socket.

Today we simply drop new data.  This adds a mechanism where we drop old
data.  For some cases, dropping old data is much nicer than dropping new
data.

But there are other compelling use-cases for this beyond libvirt.  This
will allow us to implement a 'console' command which will be pretty nice
within HMP.

It also provides a very nice way to write tests.  It's much easier to
use something like this from qtest than it is to setup a socket in in
listen mode and then queue incoming data to be read.

Regards,

Anthony Liguori


> I think most of the discussion in the quoted OpenStack bug is rather
> short sighted only focusing on the immediate needs for their current
> 'get_log_output' API. I don't think this will be satisfactory for
> OpenStack in the medium-long term. IMHO OpenStack needs to provide
> a more comprensive logging capability than what is does today, and
> thus this proposed QEMU feature would have a short lifetime of usefulness
> to OpenStack. I've already recommended that OpenStack take advantage
> of conserver to setup logging of all VMs, though there were some
> issues around that. It is entirely possible for OpenStack to provide
> its own logging system to process VM logs into fixed size files, as
> well as satisfying the needs of its get_log_output API for circular
> buffers.
>
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [RFC v3 ATCH 0/5] char: expose CirMemCharDriver and provide QMP interface

[Luiz Capitulino]

On Wed, 12 Sep 2012 20:58:00 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Wed, Sep 12, 2012 at 07:57:21PM +0800, Lei Li wrote:
> >> This RFC series attempts to convert the MemCharDriver to use a circular
> >> buffer for input and output, expose it to users by introducing QMP commands
> >> memchar_write and memchar_read and via the command line like the other
> >> CharDriverStates.
> >> 
> >> Serial ports in qemu always use CharDriverStates as there backends,
> >> Right now, all of our backends always try to write the data from the
> >> guest to a socket or file. The concern from OpenStack is that this could
> >> lead to unbounded disk space usage since they log the serial output.
> >> For more detail of the background info:
> >> https://bugs.launchpad.net/nova/+bug/832507
> >
> > Unbounded disk usage is only relevant if telling QEMU to write directly
> > to its file backend. If you use a socket backend the mgmt app can provide
> > whatever policy it desires.
> >
> >> So we want to use a circular buffer in QEMU instead, and then OpenStack
> >> can periodically read the buffer in QEMU and log it.
> >
> > With any circular buffer you obviously have a race condition where
> > the buffer may overflow before the application can consume the data.
> > By implementing the circular buffer in QEMU you are imposing a
> > specific policy for overflow on the applications using QEMU, namely
> > that data gets overwritten/lost.
> >
> > If the circular buffering is done outside QEMU, then the application
> > can implement a variety of policies on overflow. At the very least
> > they can detect when overflow would occur, and insert a marker to
> > the effect that there is a log discontinuity. Or they can pause the
> > VM for a period of time, or reduce its scheduling priority, or any
> > number of different things.
> >
> > The further advantage of doing it outside QEMU, is that OpenStack will
> > work with all existing QEMU/KVM/libvirt versions.
> 
> I'm not sure what is the best solution for libvirt and OpenStack but I
> think you're missing a few key points.
> 
> CDS doesn't propagate flow control to the guest (in some cases, it
> simply can't because hardware doesn't).  That means that there has to be
> some policy in QEMU about what to do with data that cannot be written to
> a socket.
> 
> Today we simply drop new data.  This adds a mechanism where we drop old
> data.  For some cases, dropping old data is much nicer than dropping new
> data.
> 
> But there are other compelling use-cases for this beyond libvirt.  This
> will allow us to implement a 'console' command which will be pretty nice
> within HMP.
> 
> It also provides a very nice way to write tests.  It's much easier to
> use something like this from qtest than it is to setup a socket in in
> listen mode and then queue incoming data to be read.

Sorry for catching up with this late, and hopefully my idea won't be
stupid.

But what about adding a fd backend chardev, which allows for i/o through
an fd passed by the client. Then we could add monitor_fd_add(), so that
internal qemu code (ie. hmp_console()) could add an fd to the monitor's poll.

That should eliminate the internal buffer and allow for the console
command.

[Qemu-devel] [PATCH 0/5 V4] char: add CirMemCharDriver and provide QMP interface

[Lei Li]

This RFC series attempts to convert the MemCharDriver to use a circular
buffer for input and output, expose it to users by introducing QMP commands
memchar_write and memchar_read and via the command line like the other
CharDriverStates.

Serial ports in qemu always use CharDriverStates as there backends,
Right now, all of our backends always try to write the data from the
guest to a socket or file. The concern from OpenStack is that this could
lead to unbounded disk space usage since they log the serial output.
For more detail of the background info:
https://bugs.launchpad.net/nova/+bug/832507

So we want to use a circular buffer in QEMU instead, and then OpenStack
can periodically read the buffer in QEMU and log it.

The QMP commands introduced like:

{ 'command': 'memchar-write',
  'data': {'chardev': 'str', 'size': 'int', 'data': 'str',
           'format': 'str', 'control': 'str' } }

{ 'command': 'memchar-read',
  'data': {'chardev': 'str', 'size': 'int',
           'format': 'str', 'control': 'str' },
  'returns': 'str' }

Expose CirMemCharDriver via the command line like:

qemu -chardev memchr,id=foo,maxcapacity=640k -serial chardev:foo

Introduce HMP command 'console' like:

(qemu) console foo

Note:
Now all of the feature were implemented, and the pervious comments
are fixed up too. Please comment and let me know if there is anything
else need to be improved. Your suggestion would be very appreciated!

Changes since v3:
  - Improve the algorithm of circular buffer based on Anthony's
    suggestion.
  - Some changes suggested by Luiz and Blue.
  - And other fixups.

Changes since v2:
  - Add congestion mechanism. For the 'block' option as sync command,
    will support it later when we gain the necessary infrastructure
    enhancement.
  - Add HMP 'console' command so that can interact with multiple
    chardevs via a single monitor socket.
  - Make the circular buffer backend and the current MemCharDriver
    live in parallel, expose a new char backend with circular buffer
    CirMemCharDriver suggested by Luiz.
  - Other fixs from Eric and Markus.

Changes since v1:
  - Exposing the MemCharDriver via command line.
  - Support base64 data format suggested by Anthony and Eric.
  - Follow the new rule for the name of qmp command from Eric.


Lei Li (5):
  qemu-char: Add new char backend CirMemCharDriver
  Expose CirMemCharDriver via command line
  QAPI: Introduce memchar-write QMP command
  QAPI: Introduce memchar-read QMP command
  HMP: Introduce console command

 hmp-commands.hx  |   72 +++++++++++++++++++
 hmp.c            |   99 +++++++++++++++++++++++
 hmp.h            |    3 +
 monitor.c        |   15 ++++
 monitor.h        |    3 +
 qapi-schema.json |   96 +++++++++++++++++++++++++
 qemu-char.c      |  217 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-config.c    |    3 +
 qemu-options.hx  |   10 +++
 qmp-commands.hx  |   89 +++++++++++++++++++++
 10 files changed, 607 insertions(+), 0 deletions(-)

[Qemu-devel] [PATCH 4/5] QAPI: Introduce memchar-read QMP command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 hmp-commands.hx  |   25 +++++++++++++++++++++++++
 hmp.c            |   17 +++++++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   27 +++++++++++++++++++++++++++
 qemu-char.c      |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   40 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 162 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 753aab3..5f91428 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -847,6 +847,31 @@ that specifies behavior when the queue is full/empty. By default is
 ETEXI
 
     {
+        .name       = "memchar_read",
+        .args_type  = "chardev:s,control:-b,size:i",
+        .params     = "chardev [-b] size",
+        .help       = "Provide read interface for CirMemCharDriver. Read from"
+                      "it and return 'size' of the data. (Use -b for 'block'"
+                      "option)",
+        .mhandler.cmd = hmp_memchar_read,
+    },
+
+STEXI
+@item memchar_read [-b] @var{chardev}
+@findex memchar_read
+Provide read interface for CirMemCharDriver. Read from cirmemchr
+char device and return @var{size} of the data.
+
+@var{size} is the size of data want to read from.
+
+@var{control} is option['block', 'drop'] for read and write command
+that specifies behavior when the queue is full/empty. By default is
+'drop'. Note that the 'block' option is not supported now.
+        -b for 'block' option. None for 'drop' option.
+
+ETEXI
+
+    {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
         .params     = "[-d] [-b] [-i] uri",
diff --git a/hmp.c b/hmp.c
index 18ca61b..fa858c4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -690,6 +690,23 @@ void hmp_memchar_write(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_memchar_read(Monitor *mon, const QDict *qdict)
+{
+    uint32_t size = qdict_get_int(qdict, "size");
+    const char *chardev = qdict_get_str(qdict, "chardev");
+    char *data;
+    enum DataFormat format;
+    int con = qdict_get_try_bool(qdict, "block", 0);
+    enum CongestionControl control;
+    Error *errp = NULL;
+
+    format = DATA_FORMAT_UTF8;
+    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
+    data = qmp_memchar_read(chardev, size, true, format,
+                            true, control, &errp);
+    monitor_printf(mon, "%s\n", data);
+}
+
 static void hmp_cont_cb(void *opaque, int err)
 {
     if (!err) {
diff --git a/hmp.h b/hmp.h
index 406ebb1..a5a0cfe 100644
--- a/hmp.h
+++ b/hmp.h
@@ -44,6 +44,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
 void hmp_pmemsave(Monitor *mon, const QDict *qdict);
 void hmp_memchar_write(Monitor *mon, const QDict *qdict);
+void hmp_memchar_read(Monitor *mon, const QDict *qdict);
 void hmp_cont(Monitor *mon, const QDict *qdict);
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
 void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index a908aa6..4db91b0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -394,6 +394,33 @@
            '*control': 'CongestionControl'} }
 
 ##
+# @memchar-read:
+#
+# Provide read interface for memchardev. Read from memchar
+# char device and return the data.
+#
+# @chardev: the name of the memchar char device.
+#
+# @size: the size to read in bytes.
+#
+# @format: #optional the format of the data want to read from
+#          memchardev, by default is 'utf8'.
+#
+# @control: #optional options for read and write command that specifies
+#           behavior when the queue is full/empty.
+#
+# Returns: The data read from memchar as string
+#          If @chardev is not a valid memchr device, DeviceNotFound
+#          If an I/O error occurs while reading, IOError
+#
+# Since: 1.3
+##
+{ 'command': 'memchar-read',
+  'data': {'chardev': 'str', 'size': 'int', '*format': 'DataFormat',
+           '*control': 'CongestionControl'},
+  'returns': 'str' }
+
+##
 # @CommandInfo:
 #
 # Information about a QMP command
diff --git a/qemu-char.c b/qemu-char.c
index 133d282..a8bcead 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2736,6 +2736,58 @@ void qmp_memchar_write(const char *chardev, int64_t size,
     }
 }
 
+char *qmp_memchar_read(const char *chardev, int64_t size,
+                       bool has_format, enum DataFormat format,
+                       bool has_control, enum CongestionControl control,
+                       Error **errp)
+{
+    CharDriverState *chr;
+    guchar *read_data;
+    char *data = NULL;
+    int ret;
+
+    read_data = g_malloc0(size);
+
+    chr = qemu_chr_find(chardev);
+    if (!chr) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
+        return NULL;
+    }
+
+    /* XXX: For the sync command as 'block', waiting for the qmp
+ *      * to support necessary feature. Now just act as 'drop'. */
+    if (cirmem_chr_is_empty(chr)) {
+        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
+            error_setg(errp, "Failed to read from memchr %s", chardev);
+            return NULL;
+        } else {
+            error_setg(errp, "Failed to read from memchr %s", chardev);
+            return NULL;
+        }
+    }
+
+    if (size == 0) {
+        size = CBUFF_SIZE;
+    }
+
+    ret = cirmem_chr_read(chr, read_data, size);
+
+    if (ret < 0) {
+        error_setg(errp, "Failed to read from memchr %s", chardev);
+        return NULL;
+    }
+
+    if (has_format && (format == DATA_FORMAT_BASE64)) {
+       if (read_data) {
+           data = g_base64_encode(read_data, (size_t)size);
+       }
+    } else {
+        data = (char *)read_data;
+    }
+
+    return data;
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 502ed57..08ea798 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -506,6 +506,46 @@ Example:
 EQMP
 
     {
+        .name       = "memchar-read",
+        .args_type  = "chardev:s,size:i,format:s?,control:s?",
+        .help       = "return the size of data from memchar chardev",
+        .mhandler.cmd_new = qmp_marshal_input_memchar_read,
+    },
+
+SQMP
+memchar-read
+-------------
+
+Provide read interface for memchardev. Read from memchar
+char device and return the data.
+
+Arguments:
+
+- "chardev": the name of the char device, must be unique (json-string)
+- "size": the memory size wanted to read in bytes(refer to unencoded
+          size of the raw data), would adjust to the init size of the
+          memchar if the requested size is larger than it. (json-int)
+- "format": the data format write to memchardev, default is
+            utf8. (json-string, optional)
+          - Possible values: "utf8", "base64"
+
+- "control": options for read and write command that specifies
+             behavior when the queue is full/empty, default is
+             drop. (json-string, optional)
+          - Possible values: "drop", "block"
+
+Example:
+
+-> { "execute": "memchar-read",
+                "arguments": { "chardev": foo,
+                               "size": 1000,
+                               "format": "utf8",
+                               "control": "drop" } }
+<- { "return": "data string..." }
+
+EQMP
+
+    {
         .name       = "xen-save-devices-state",
         .args_type  = "filename:F",
     .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 4/5] QAPI: Introduce memchar-read QMP command

[Luiz Capitulino]

On Mon, 22 Oct 2012 00:48:00 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |   25 +++++++++++++++++++++++++
>  hmp.c            |   17 +++++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   27 +++++++++++++++++++++++++++
>  qemu-char.c      |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   40 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 162 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 753aab3..5f91428 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -847,6 +847,31 @@ that specifies behavior when the queue is full/empty. By default is
>  ETEXI
>  
>      {
> +        .name       = "memchar_read",
> +        .args_type  = "chardev:s,control:-b,size:i",
> +        .params     = "chardev [-b] size",
> +        .help       = "Provide read interface for CirMemCharDriver. Read from"
> +                      "it and return 'size' of the data. (Use -b for 'block'"
> +                      "option)",
> +        .mhandler.cmd = hmp_memchar_read,
> +    },
> +
> +STEXI
> +@item memchar_read [-b] @var{chardev}
> +@findex memchar_read
> +Provide read interface for CirMemCharDriver. Read from cirmemchr
> +char device and return @var{size} of the data.
> +
> +@var{size} is the size of data want to read from.
> +
> +@var{control} is option['block', 'drop'] for read and write command
> +that specifies behavior when the queue is full/empty. By default is
> +'drop'. Note that the 'block' option is not supported now.
> +        -b for 'block' option. None for 'drop' option.
> +
> +ETEXI
> +
> +    {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
>          .params     = "[-d] [-b] [-i] uri",
> diff --git a/hmp.c b/hmp.c
> index 18ca61b..fa858c4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -690,6 +690,23 @@ void hmp_memchar_write(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>  
> +void hmp_memchar_read(Monitor *mon, const QDict *qdict)
> +{
> +    uint32_t size = qdict_get_int(qdict, "size");
> +    const char *chardev = qdict_get_str(qdict, "chardev");
> +    char *data;
> +    enum DataFormat format;
> +    int con = qdict_get_try_bool(qdict, "block", 0);
> +    enum CongestionControl control;
> +    Error *errp = NULL;
> +
> +    format = DATA_FORMAT_UTF8;
> +    control = con ? CONGESTION_CONTROL_BLOCK : CONGESTION_CONTROL_DROP;
> +    data = qmp_memchar_read(chardev, size, true, format,
> +                            true, control, &errp);
> +    monitor_printf(mon, "%s\n", data);

Doesn't handle errors, leaks 'data' and may leak 'errp' if an error happens.

> +}
> +
>  static void hmp_cont_cb(void *opaque, int err)
>  {
>      if (!err) {
> diff --git a/hmp.h b/hmp.h
> index 406ebb1..a5a0cfe 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -44,6 +44,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
>  void hmp_memchar_write(Monitor *mon, const QDict *qdict);
> +void hmp_memchar_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
>  void hmp_inject_nmi(Monitor *mon, const QDict *qdict);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a908aa6..4db91b0 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -394,6 +394,33 @@
>             '*control': 'CongestionControl'} }
>  
>  ##
> +# @memchar-read:
> +#
> +# Provide read interface for memchardev. Read from memchar
> +# char device and return the data.
> +#
> +# @chardev: the name of the memchar char device.
> +#
> +# @size: the size to read in bytes.
> +#
> +# @format: #optional the format of the data want to read from
> +#          memchardev, by default is 'utf8'.
> +#
> +# @control: #optional options for read and write command that specifies
> +#           behavior when the queue is full/empty.
> +#
> +# Returns: The data read from memchar as string
> +#          If @chardev is not a valid memchr device, DeviceNotFound
> +#          If an I/O error occurs while reading, IOError

Please, drop IOError.

> +#
> +# Since: 1.3
> +##
> +{ 'command': 'memchar-read',
> +  'data': {'chardev': 'str', 'size': 'int', '*format': 'DataFormat',
> +           '*control': 'CongestionControl'},
> +  'returns': 'str' }
> +
> +##
>  # @CommandInfo:
>  #
>  # Information about a QMP command
> diff --git a/qemu-char.c b/qemu-char.c
> index 133d282..a8bcead 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2736,6 +2736,58 @@ void qmp_memchar_write(const char *chardev, int64_t size,
>      }
>  }
>  
> +char *qmp_memchar_read(const char *chardev, int64_t size,
> +                       bool has_format, enum DataFormat format,
> +                       bool has_control, enum CongestionControl control,
> +                       Error **errp)
> +{
> +    CharDriverState *chr;
> +    guchar *read_data;
> +    char *data = NULL;
> +    int ret;
> +
> +    read_data = g_malloc0(size);
> +
> +    chr = qemu_chr_find(chardev);
> +    if (!chr) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
> +        return NULL;
> +    }

Leaks read_data on error.

> +
> +    /* XXX: For the sync command as 'block', waiting for the qmp
> + *      * to support necessary feature. Now just act as 'drop'. */
> +    if (cirmem_chr_is_empty(chr)) {
> +        if (has_control && (control == CONGESTION_CONTROL_BLOCK)) {
> +            error_setg(errp, "Failed to read from memchr %s", chardev);
> +            return NULL;
> +        } else {
> +            error_setg(errp, "Failed to read from memchr %s", chardev);
> +            return NULL;
> +        }
> +    }
> +
> +    if (size == 0) {
> +        size = CBUFF_SIZE;
> +    }
> +
> +    ret = cirmem_chr_read(chr, read_data, size);
> +
> +    if (ret < 0) {
> +        error_setg(errp, "Failed to read from memchr %s", chardev);
> +        return NULL;
> +    }

You should free read_data on error.

> +
> +    if (has_format && (format == DATA_FORMAT_BASE64)) {
> +       if (read_data) {

Why is this test needed?

> +           data = g_base64_encode(read_data, (size_t)size);

Leaks read_data.

> +       }
> +    } else {
> +        data = (char *)read_data;
> +    }
> +
> +    return data;
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 502ed57..08ea798 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -506,6 +506,46 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "memchar-read",
> +        .args_type  = "chardev:s,size:i,format:s?,control:s?",
> +        .help       = "return the size of data from memchar chardev",

Help doesn't make sense for HMP.

> +        .mhandler.cmd_new = qmp_marshal_input_memchar_read,
> +    },
> +
> +SQMP
> +memchar-read
> +-------------
> +
> +Provide read interface for memchardev. Read from memchar
> +char device and return the data.
> +
> +Arguments:
> +
> +- "chardev": the name of the char device, must be unique (json-string)
> +- "size": the memory size wanted to read in bytes(refer to unencoded
> +          size of the raw data), would adjust to the init size of the
> +          memchar if the requested size is larger than it. (json-int)
> +- "format": the data format write to memchardev, default is
> +            utf8. (json-string, optional)
> +          - Possible values: "utf8", "base64"
> +
> +- "control": options for read and write command that specifies
> +             behavior when the queue is full/empty, default is
> +             drop. (json-string, optional)
> +          - Possible values: "drop", "block"
> +
> +Example:
> +
> +-> { "execute": "memchar-read",
> +                "arguments": { "chardev": foo,
> +                               "size": 1000,
> +                               "format": "utf8",
> +                               "control": "drop" } }
> +<- { "return": "data string..." }
> +
> +EQMP
> +
> +    {
>          .name       = "xen-save-devices-state",
>          .args_type  = "filename:F",
>      .mhandler.cmd_new = qmp_marshal_input_xen_save_devices_state,