[Qemu-devel] [RFC PATCH 0/4] char: expose MemoryCharDriver to users and provide QMP interface

[Qemu-devel] [RFC PATCH 0/4] char: expose MemoryCharDriver to users 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'} }

{ 'command': 'memchar_read',
  'data': {'chardev': 'str', 'size': 'int'},
  'returns': 'str' }

Expose MemCharDriver via the command line like:

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

Note:
This series is just a incomplete sketch and not completely tested which
I am still struggling with, but I want to get this out ealier to have
your suggestion. Please comment and let me know if this seems like the
direction we should be headed, thanks!

TODO:
1) Expose the MemCharDriver via the command line like the other
   CharDriverStates.
2) Wherther to add locking or notification mechanism.


Lei Li (4):
  qemu-char: Convert MemCharDriver to circular buffer
  monitor: Adjust qmp_human_monitor_command to new MemCharDriver
  QAPI: Introduce memchar_write QMP command
  QAPI: Introduce memchar_read QMP command

 monitor.c        |    2 +-
 qapi-schema.json |   40 +++++++++++++++++
 qemu-char.c      |  130 ++++++++++++++++++++++++++++++++++++++++++++++--------
 qemu-char.h      |    2 +-
 qmp-commands.hx  |   60 ++++++++++++++++++++++++-
 5 files changed, 212 insertions(+), 22 deletions(-)

[Qemu-devel] [RFC PATCH 1/4] qemu-char: Convert MemCharDriver to circular buffer

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qemu-char.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 qemu-char.h |    2 +-
 2 files changed, 78 insertions(+), 20 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index c2aaaee..087c92d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2517,38 +2517,96 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 /***********************************************************/
 /* Memory chardev */
 typedef struct {
-    size_t outbuf_size;
-    size_t outbuf_capacity;
-    uint8_t *outbuf;
+    size_t cbuf_capacity;
+    size_t cbuf_in;
+    size_t cbuf_out;
+    size_t cbuf_count;
+    uint8_t *cbuf;
 } MemoryDriver;
 
+static int mem_chr_is_empty(CharDriverState *chr)
+{
+    MemoryDriver *d = chr->opaque;
+
+    return d->cbuf_count == 0;
+}
+
+static int mem_chr_is_full(CharDriverState *chr)
+{
+    MemoryDriver *d = chr->opaque;
+
+    return d->cbuf_count == d->cbuf_capacity;
+}
+
 static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     MemoryDriver *d = chr->opaque;
+    int left;
 
-    /* TODO: the QString implementation has the same code, we should
-     * introduce a generic way to do this in cutils.c */
-    if (d->outbuf_capacity < d->outbuf_size + len) {
-        /* grow outbuf */
-        d->outbuf_capacity += len;
-        d->outbuf_capacity *= 2;
-        d->outbuf = g_realloc(d->outbuf, d->outbuf_capacity);
+    if (d->cbuf_capacity < len) {
+        return -1;
     }
 
-    memcpy(d->outbuf + d->outbuf_size, buf, len);
-    d->outbuf_size += len;
+    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 (mem_chr_is_full(chr)) {
+            d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
+        } else {
+        /* Enough cbuf to write */
+        memcpy(d->cbuf + d->cbuf_in, buf, len);
+        d->cbuf_count += len;
+        }
+    }
+
+    d->cbuf_in = (d->cbuf_in + len) % d->cbuf_capacity;
 
     return len;
 }
 
-void qemu_chr_init_mem(CharDriverState *chr)
+static void mem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
+{
+    MemoryDriver *d = chr->opaque;
+    int left;
+
+    if (mem_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;
+}
+
+void qemu_chr_init_mem(CharDriverState *chr, size_t size)
 {
     MemoryDriver *d;
 
     d = g_malloc(sizeof(*d));
-    d->outbuf_size = 0;
-    d->outbuf_capacity = 4096;
-    d->outbuf = g_malloc0(d->outbuf_capacity);
+    d->cbuf_capacity = 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;
@@ -2558,7 +2616,7 @@ void qemu_chr_init_mem(CharDriverState *chr)
 QString *qemu_chr_mem_to_qs(CharDriverState *chr)
 {
     MemoryDriver *d = chr->opaque;
-    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
+    return qstring_from_substr((char *) d->cbuf, 0, d->cbuf_count - 1);
 }
 
 /* NOTE: this driver can not be closed with qemu_chr_delete()! */
@@ -2566,7 +2624,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
 {
     MemoryDriver *d = chr->opaque;
 
-    g_free(d->outbuf);
+    g_free(d->cbuf);
     g_free(chr->opaque);
     chr->opaque = NULL;
     chr->chr_write = NULL;
@@ -2575,7 +2633,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
 size_t qemu_chr_mem_osize(const CharDriverState *chr)
 {
     const MemoryDriver *d = chr->opaque;
-    return d->outbuf_size;
+    return d->cbuf_count;
 }
 
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
diff --git a/qemu-char.h b/qemu-char.h
index 486644b..d8d90cc 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -243,7 +243,7 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
 extern int term_escape_char;
 
 /* memory chardev */
-void qemu_chr_init_mem(CharDriverState *chr);
+void qemu_chr_init_mem(CharDriverState *chr, size_t size);
 void qemu_chr_close_mem(CharDriverState *chr);
 QString *qemu_chr_mem_to_qs(CharDriverState *chr);
 size_t qemu_chr_mem_osize(const CharDriverState *chr);
-- 
1.7.7.6

[Qemu-devel] [RFC PATCH 2/4] monitor: Adjust qmp_human_monitor_command to new MemCharDriver

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 monitor.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 49dccfe..c1a4d81 100644
--- a/monitor.c
+++ b/monitor.c
@@ -645,7 +645,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     CharDriverState mchar;
 
     memset(&hmp, 0, sizeof(hmp));
-    qemu_chr_init_mem(&mchar);
+
+    /* Since the backend of MemCharDriver convert to a circular
+     * buffer with fixed size, so should indicate the init memory
+     * size.
+     *
+     * XXX:  is 4096 as init memory enough for this? */
+    qemu_chr_init_mem(&mchar, 4096);
     hmp.chr = &mchar;
 
     old_mon = cur_mon;
-- 
1.7.7.6

[Qemu-devel] [RFC PATCH 3/4] qmp: Introduce memchar_write QMP command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qapi-schema.json |   20 ++++++++++++++++++++
 qemu-char.c      |   19 +++++++++++++++++++
 qmp-commands.hx  |   29 +++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index bc55ed2..3c8530f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -206,6 +206,26 @@
 { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
 
 ##
+# @memchar_write:
+#
+# Provide writing interface for memchardev. Write data to memchar
+# char device.
+#
+# @chardev: the name of the memchar char device.
+#
+# @size: the size to write in bytes.
+#
+# @data: the source data write to memchar.
+#
+# Returns: Nothing on success
+#          If an I/O error occurs while writing, IOError
+#
+# Since: 1.2
+##
+{ 'command': 'memchar_write',
+  'data': {'chardev': 'str', 'size': 'int', 'data': 'str'} }
+
+##
 # @CommandInfo:
 #
 # Information about a QMP command
diff --git a/qemu-char.c b/qemu-char.c
index 087c92d..1012e65 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2636,6 +2636,25 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
     return d->cbuf_count;
 }
 
+void qmp_memchar_write(const char *chardev, int64_t size,
+                       uint8_t *data, Error **errp)
+{
+    CharDriverState *chr;
+    int ret;
+
+    chr = qemu_chr_find(chardev);
+
+    if(!chr) {
+        qemu_chr_init_mem(chr, size);
+    }
+
+    ret = mem_chr_write(chr, data, size);
+    if (ret <= 0) {
+        error_set(errp, QERR_IO_ERROR);
+        return;
+    }
+}
+
 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 e3cf3c5..182f1e6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -441,6 +441,35 @@ Note: inject-nmi is only supported for x86 guest currently, it will
 EQMP
 
     {
+        .name       = "memchar_write",
+        .args_type  = "chardev:s,size:i,data:s",
+        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
+    },
+
+SQMP
+memchar_write
+-------------
+
+Provide writing interface for memchardev. Write data to memchar
+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 memchar (json-string)
+
+Example:
+
+-> { "execute": "memchar_write",
+                "arguments": { "chardev": foo,
+                               "size": 1000,
+                               "data": "data string" } }
+<- { "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

[Qemu-devel] [RFC PATCH 4/4] qmp: Introduce memchar_read QMP command

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qapi-schema.json |   20 ++++++++++++++++++++
 qemu-char.c      |   16 ++++++++++++++++
 qmp-commands.hx  |   28 ++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 3c8530f..23edda9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -226,6 +226,26 @@
   'data': {'chardev': 'str', 'size': 'int', 'data': 'str'} }
 
 ##
+# @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 write in bytes.
+#
+# Returns: The data read from memchar as string.
+#          If @chardev is not a valid memchar device, DeviceNotFound
+#          If an I/O error occurs while reading, IOError
+#
+# Since: 1.2
+##
+{ 'command': 'memchar_read',
+  'data': {'chardev': 'str', 'size': 'int'},
+  'returns': 'str' }
+
+##
 # @CommandInfo:
 #
 # Information about a QMP command
diff --git a/qemu-char.c b/qemu-char.c
index 1012e65..545a792 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2655,6 +2655,22 @@ void qmp_memchar_write(const char *chardev, int64_t size,
     }
 }
 
+char *qmp_memchar_read(const char *chardev, int64_t size,
+                       Error **errp)
+{
+    CharDriverState *chr;
+    char *out_data;
+
+    chr = qemu_chr_find(chardev);
+    if(!chr) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, chardev);
+        return NULL;
+    }
+
+    mem_chr_read(chr, (uint8_t *)out_data, size);
+    return out_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 182f1e6..163e547 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -470,6 +470,34 @@ Example:
 EQMP
 
     {
+        .name       = "memchar_read",
+        .args_type  = "id:s,size:i",
+        .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 in bytes, init size of the memchar
+          by default (json-int)
+
+Example:
+
+-> { "execute": "memchar_read",
+                "arguments": { "chardev": foo,
+                               "size": 1000,
+<- { "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] [RFC PATCH 3/4] qmp: Introduce memchar_write QMP command

[Eric Blake]

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

On 08/01/2012 03:48 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qapi-schema.json |   20 ++++++++++++++++++++
>  qemu-char.c      |   19 +++++++++++++++++++
>  qmp-commands.hx  |   29 +++++++++++++++++++++++++++++
>  3 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index bc55ed2..3c8530f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -206,6 +206,26 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>  
>  ##
> +# @memchar_write:

s/memchar_write/memchar-write/

New QMP commands should use '-' not '_'.

> +#
> +# Provide writing interface for memchardev. Write data to memchar
> +# char device.
> +#
> +# @chardev: the name of the memchar char device.
> +#
> +# @size: the size to write in bytes.
> +#
> +# @data: the source data write to memchar.

Does this allow full binary data processing, or does the data need
something like base64 encoding?

> +#
> +# Returns: Nothing on success
> +#          If an I/O error occurs while writing, IOError
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'memchar_write',

s/_/-/

> +++ b/qmp-commands.hx
> @@ -441,6 +441,35 @@ Note: inject-nmi is only supported for x86 guest currently, it will
>  EQMP
>  
>      {
> +        .name       = "memchar_write",

s/_/-/

> +        .args_type  = "chardev:s,size:i,data:s",
> +        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
> +    },
> +
> +SQMP
> +memchar_write
> +-------------
> +
> +Provide writing interface for memchardev. Write data to memchar
> +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 memchar (json-string)

s/writed/written/

> +
> +Example:
> +
> +-> { "execute": "memchar_write",
> +                "arguments": { "chardev": foo,
> +                               "size": 1000,
> +                               "data": "data string" } }

1000 is longer than strlen("data string").  Is there supposed to be a
correlation?  Is there a start offset to worry about, or do memchardevs
always behave like they are in append mode?

-- 
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: 620 bytes --]

Re: [Qemu-devel] [RFC PATCH 4/4] qmp: Introduce memchar_read QMP command

[Eric Blake]

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

On 08/01/2012 03:48 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qapi-schema.json |   20 ++++++++++++++++++++
>  qemu-char.c      |   16 ++++++++++++++++
>  qmp-commands.hx  |   28 ++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3c8530f..23edda9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -226,6 +226,26 @@
>    'data': {'chardev': 'str', 'size': 'int', 'data': 'str'} }
>  
>  ##
> +# @memchar_read:

As in the previous patch, s/_/-/g

> +#
> +# 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 write in bytes.

s/write/read/

> +#
> +# Returns: The data read from memchar as string.
> +#          If @chardev is not a valid memchar device, DeviceNotFound
> +#          If an I/O error occurs while reading, IOError
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'memchar_read',
> +  'data': {'chardev': 'str', 'size': 'int'},
> +  'returns': 'str' }

Again, is there going to be a binary encoding issue?

-- 
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: 620 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/4] qemu-char: Convert MemCharDriver to circular buffer

[Anthony Liguori]

Lei Li <lilei@linux.vnet.ibm.com> writes:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qemu-char.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>  qemu-char.h |    2 +-
>  2 files changed, 78 insertions(+), 20 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index c2aaaee..087c92d 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2517,38 +2517,96 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>  /***********************************************************/
>  /* Memory chardev */
>  typedef struct {
> -    size_t outbuf_size;
> -    size_t outbuf_capacity;
> -    uint8_t *outbuf;
> +    size_t cbuf_capacity;
> +    size_t cbuf_in;
> +    size_t cbuf_out;
> +    size_t cbuf_count;
> +    uint8_t *cbuf;
>  } MemoryDriver;

Probably should move the buffer into a separate structure and then you
can drop the cbuf_ prefixes.

> +static int mem_chr_is_empty(CharDriverState *chr)
> +{
> +    MemoryDriver *d = chr->opaque;
> +
> +    return d->cbuf_count == 0;
> +}
> +
> +static int mem_chr_is_full(CharDriverState *chr)
> +{
> +    MemoryDriver *d = chr->opaque;
> +
> +    return d->cbuf_count == d->cbuf_capacity;
> +}
> +

Typically, you would use a producer and a consumer index.  To test for
empty, you would check if (consumer == producer).

To check for full, you would check if ((producer - consumer) == size).

To get the actual index, you always modulus the indexes with size.  This
only works if size is a power of 2 but that's a reasonable restriction.

>  static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>  {
>      MemoryDriver *d = chr->opaque;
> +    int left;
>  
> -    /* TODO: the QString implementation has the same code, we should
> -     * introduce a generic way to do this in cutils.c */
> -    if (d->outbuf_capacity < d->outbuf_size + len) {
> -        /* grow outbuf */
> -        d->outbuf_capacity += len;
> -        d->outbuf_capacity *= 2;
> -        d->outbuf = g_realloc(d->outbuf, d->outbuf_capacity);
> +    if (d->cbuf_capacity < len) {
> +        return -1;
>      }
>  
> -    memcpy(d->outbuf + d->outbuf_size, buf, len);
> -    d->outbuf_size += len;
> +    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;

Doing a mempcy() like this may work, but seems inefficient to me.  I
think reading like a ring queue works a bit nicer.

> +    } else {
> +        /* Completely overwrite */
> +        if (mem_chr_is_full(chr)) {
> +            d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
> +        } else {
> +        /* Enough cbuf to write */
> +        memcpy(d->cbuf + d->cbuf_in, buf, len);
> +        d->cbuf_count += len;
> +        }

Looks like indenting is off here.

Regards,

Anthony Liguori

> +    }
> +
> +    d->cbuf_in = (d->cbuf_in + len) % d->cbuf_capacity;
>  
>      return len;
>  }
>  
> -void qemu_chr_init_mem(CharDriverState *chr)
> +static void mem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
> +{
> +    MemoryDriver *d = chr->opaque;
> +    int left;
> +
> +    if (mem_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;
> +}
> +
> +void qemu_chr_init_mem(CharDriverState *chr, size_t size)
>  {
>      MemoryDriver *d;
>  
>      d = g_malloc(sizeof(*d));
> -    d->outbuf_size = 0;
> -    d->outbuf_capacity = 4096;
> -    d->outbuf = g_malloc0(d->outbuf_capacity);
> +    d->cbuf_capacity = 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;
> @@ -2558,7 +2616,7 @@ void qemu_chr_init_mem(CharDriverState *chr)
>  QString *qemu_chr_mem_to_qs(CharDriverState *chr)
>  {
>      MemoryDriver *d = chr->opaque;
> -    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
> +    return qstring_from_substr((char *) d->cbuf, 0, d->cbuf_count - 1);
>  }
>  
>  /* NOTE: this driver can not be closed with qemu_chr_delete()! */
> @@ -2566,7 +2624,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
>  {
>      MemoryDriver *d = chr->opaque;
>  
> -    g_free(d->outbuf);
> +    g_free(d->cbuf);
>      g_free(chr->opaque);
>      chr->opaque = NULL;
>      chr->chr_write = NULL;
> @@ -2575,7 +2633,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
>  size_t qemu_chr_mem_osize(const CharDriverState *chr)
>  {
>      const MemoryDriver *d = chr->opaque;
> -    return d->outbuf_size;
> +    return d->cbuf_count;
>  }
>  
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> diff --git a/qemu-char.h b/qemu-char.h
> index 486644b..d8d90cc 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -243,7 +243,7 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
>  extern int term_escape_char;
>  
>  /* memory chardev */
> -void qemu_chr_init_mem(CharDriverState *chr);
> +void qemu_chr_init_mem(CharDriverState *chr, size_t size);
>  void qemu_chr_close_mem(CharDriverState *chr);
>  QString *qemu_chr_mem_to_qs(CharDriverState *chr);
>  size_t qemu_chr_mem_osize(const CharDriverState *chr);
> -- 
> 1.7.7.6

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Introduce memchar_write QMP command

[Anthony Liguori]

Lei Li <lilei@linux.vnet.ibm.com> writes:

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qapi-schema.json |   20 ++++++++++++++++++++
>  qemu-char.c      |   19 +++++++++++++++++++
>  qmp-commands.hx  |   29 +++++++++++++++++++++++++++++
>  3 files changed, 68 insertions(+), 0 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index bc55ed2..3c8530f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -206,6 +206,26 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>  
>  ##
> +# @memchar_write:

memchar-write as Eric suggested.

> +#
> +# Provide writing interface for memchardev. Write data to memchar
> +# char device.
> +#
> +# @chardev: the name of the memchar char device.
> +#
> +# @size: the size to write in bytes.
> +#
> +# @data: the source data write to memchar.
> +#
> +# Returns: Nothing on success
> +#          If an I/O error occurs while writing, IOError
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'memchar_write',
> +  'data': {'chardev': 'str', 'size': 'int', 'data': 'str'} }

You should add another option which is '*format': 'DataFormat'.

DataFormat would be an enum and would include: "utf8", "base64" to start
with.  The default value would be utf8.  You'll need to be careful to
scrub the output of memchar-read of any non-UTF8 character to make sure
what you are returning is UTF8 clean.

> +
> +##
>  # @CommandInfo:
>  #
>  # Information about a QMP command
> diff --git a/qemu-char.c b/qemu-char.c
> index 087c92d..1012e65 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2636,6 +2636,25 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
>      return d->cbuf_count;
>  }
>  
> +void qmp_memchar_write(const char *chardev, int64_t size,
> +                       uint8_t *data, Error **errp)
> +{
> +    CharDriverState *chr;
> +    int ret;
> +
> +    chr = qemu_chr_find(chardev);
> +
> +    if(!chr) {
> +        qemu_chr_init_mem(chr, size);
> +    }

Indenting is off here (need a space after the if).  Plus chr is still
NULL after this returns.  I don't think you should do lazy init here.
If qemu_chr_find() returns NULL, you should throw an error.

Regards,

Anthony Liguori

> +
> +    ret = mem_chr_write(chr, data, size);
> +    if (ret <= 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +}
> +
>  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 e3cf3c5..182f1e6 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -441,6 +441,35 @@ Note: inject-nmi is only supported for x86 guest currently, it will
>  EQMP
>  
>      {
> +        .name       = "memchar_write",
> +        .args_type  = "chardev:s,size:i,data:s",
> +        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
> +    },
> +
> +SQMP
> +memchar_write
> +-------------
> +
> +Provide writing interface for memchardev. Write data to memchar
> +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 memchar (json-string)
> +
> +Example:
> +
> +-> { "execute": "memchar_write",
> +                "arguments": { "chardev": foo,
> +                               "size": 1000,
> +                               "data": "data string" } }
> +<- { "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] [RFC PATCH 0/4] char: expose MemoryCharDriver to users and provide QMP interface

[Anthony Liguori]

Lei Li <lilei@linux.vnet.ibm.com> writes:

> 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

The other neat feature this could allow is an HMP "console" command that
allows you to temporarily connect to the serial console from within the
monitor.  This could provide a cleaner approach to how we currently do
chardev muxing.

It also means you can interact with multiple chardevs via a single
monitor socket (or via the GDB monitor interface).

> 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'} }
>
> { 'command': 'memchar_read',
>   'data': {'chardev': 'str', 'size': 'int'},
>   'returns': 'str' }
>
> Expose MemCharDriver via the command line like:
>
> qemu -chardev memchr,max-capacity=640k,id=foo -serial chardev:foo
>
> Note:
> This series is just a incomplete sketch and not completely tested which
> I am still struggling with, but I want to get this out ealier to have
> your suggestion. Please comment and let me know if this seems like the
> direction we should be headed, thanks!
>
> TODO:
> 1) Expose the MemCharDriver via the command line like the other
>    CharDriverStates.
> 2) Wherther to add locking or notification mechanism.

Notification is an interesting topic.  I thought a lot about it and I
believe that we should simply avoid notification for now.

When we introduce async commands, we could make the read command block
which I think is a lot nicer.

BTW, we should introduce options for the read and write command that
specifies behavior when the queue is full/empty.  For instance,

..., '*control': 'CongestionControl' }

{ 'enum': 'CongestionControl', 'data': [ 'drop', 'block' ] }

We would default to control=drop which would result in reads returning
empty strings and writes dropping queued data.

OTOH, when we get sync commands, we would add the 'block' enum option
and then we could make the session block until data was available or the
queue had space available.

Regards,

Anthony Liguori

>
>
> Lei Li (4):
>   qemu-char: Convert MemCharDriver to circular buffer
>   monitor: Adjust qmp_human_monitor_command to new MemCharDriver
>   QAPI: Introduce memchar_write QMP command
>   QAPI: Introduce memchar_read QMP command
>
>  monitor.c        |    2 +-
>  qapi-schema.json |   40 +++++++++++++++++
>  qemu-char.c      |  130 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  qemu-char.h      |    2 +-
>  qmp-commands.hx  |   60 ++++++++++++++++++++++++-
>  5 files changed, 212 insertions(+), 22 deletions(-)

Re: [Qemu-devel] [RFC PATCH 1/4] qemu-char: Convert MemCharDriver to circular buffer

[Lei Li]

On 08/02/2012 05:30 AM, Anthony Liguori wrote:
> Lei Li <lilei@linux.vnet.ibm.com> writes:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qemu-char.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>>   qemu-char.h |    2 +-
>>   2 files changed, 78 insertions(+), 20 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index c2aaaee..087c92d 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2517,38 +2517,96 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>>   /***********************************************************/
>>   /* Memory chardev */
>>   typedef struct {
>> -    size_t outbuf_size;
>> -    size_t outbuf_capacity;
>> -    uint8_t *outbuf;
>> +    size_t cbuf_capacity;
>> +    size_t cbuf_in;
>> +    size_t cbuf_out;
>> +    size_t cbuf_count;
>> +    uint8_t *cbuf;
>>   } MemoryDriver;
> Probably should move the buffer into a separate structure and then you
> can drop the cbuf_ prefixes.
>
>> +static int mem_chr_is_empty(CharDriverState *chr)
>> +{
>> +    MemoryDriver *d = chr->opaque;
>> +
>> +    return d->cbuf_count == 0;
>> +}
>> +
>> +static int mem_chr_is_full(CharDriverState *chr)
>> +{
>> +    MemoryDriver *d = chr->opaque;
>> +
>> +    return d->cbuf_count == d->cbuf_capacity;
>> +}
>> +
> Typically, you would use a producer and a consumer index.  To test for
> empty, you would check if (consumer == producer).
>
> To check for full, you would check if ((producer - consumer) == size).
>
> To get the actual index, you always modulus the indexes with size.  This
> only works if size is a power of 2 but that's a reasonable restriction.
>
>>   static int mem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>>   {
>>       MemoryDriver *d = chr->opaque;
>> +    int left;
>>   
>> -    /* TODO: the QString implementation has the same code, we should
>> -     * introduce a generic way to do this in cutils.c */
>> -    if (d->outbuf_capacity < d->outbuf_size + len) {
>> -        /* grow outbuf */
>> -        d->outbuf_capacity += len;
>> -        d->outbuf_capacity *= 2;
>> -        d->outbuf = g_realloc(d->outbuf, d->outbuf_capacity);
>> +    if (d->cbuf_capacity < len) {
>> +        return -1;
>>       }
>>   
>> -    memcpy(d->outbuf + d->outbuf_size, buf, len);
>> -    d->outbuf_size += len;
>> +    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;
> Doing a mempcy() like this may work, but seems inefficient to me.  I
> think reading like a ring queue works a bit nicer.

Hi Anthony,

What do you mean "reading like a ring queue"?  I am a little confused
here. Could you please give more details?

And thanks for your suggestions. :)

>> +    } else {
>> +        /* Completely overwrite */
>> +        if (mem_chr_is_full(chr)) {
>> +            d->cbuf_out = (d->cbuf_out + len) % d->cbuf_capacity;
>> +        } else {
>> +        /* Enough cbuf to write */
>> +        memcpy(d->cbuf + d->cbuf_in, buf, len);
>> +        d->cbuf_count += len;
>> +        }
> Looks like indenting is off here.
>
> Regards,
>
> Anthony Liguori
>
>> +    }
>> +
>> +    d->cbuf_in = (d->cbuf_in + len) % d->cbuf_capacity;
>>   
>>       return len;
>>   }
>>   
>> -void qemu_chr_init_mem(CharDriverState *chr)
>> +static void mem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
>> +{
>> +    MemoryDriver *d = chr->opaque;
>> +    int left;
>> +
>> +    if (mem_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;
>> +}
>> +
>> +void qemu_chr_init_mem(CharDriverState *chr, size_t size)
>>   {
>>       MemoryDriver *d;
>>   
>>       d = g_malloc(sizeof(*d));
>> -    d->outbuf_size = 0;
>> -    d->outbuf_capacity = 4096;
>> -    d->outbuf = g_malloc0(d->outbuf_capacity);
>> +    d->cbuf_capacity = 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;
>> @@ -2558,7 +2616,7 @@ void qemu_chr_init_mem(CharDriverState *chr)
>>   QString *qemu_chr_mem_to_qs(CharDriverState *chr)
>>   {
>>       MemoryDriver *d = chr->opaque;
>> -    return qstring_from_substr((char *) d->outbuf, 0, d->outbuf_size - 1);
>> +    return qstring_from_substr((char *) d->cbuf, 0, d->cbuf_count - 1);
>>   }
>>   
>>   /* NOTE: this driver can not be closed with qemu_chr_delete()! */
>> @@ -2566,7 +2624,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
>>   {
>>       MemoryDriver *d = chr->opaque;
>>   
>> -    g_free(d->outbuf);
>> +    g_free(d->cbuf);
>>       g_free(chr->opaque);
>>       chr->opaque = NULL;
>>       chr->chr_write = NULL;
>> @@ -2575,7 +2633,7 @@ void qemu_chr_close_mem(CharDriverState *chr)
>>   size_t qemu_chr_mem_osize(const CharDriverState *chr)
>>   {
>>       const MemoryDriver *d = chr->opaque;
>> -    return d->outbuf_size;
>> +    return d->cbuf_count;
>>   }
>>   
>>   QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>> diff --git a/qemu-char.h b/qemu-char.h
>> index 486644b..d8d90cc 100644
>> --- a/qemu-char.h
>> +++ b/qemu-char.h
>> @@ -243,7 +243,7 @@ CharDriverState *qemu_chr_open_eventfd(int eventfd);
>>   extern int term_escape_char;
>>   
>>   /* memory chardev */
>> -void qemu_chr_init_mem(CharDriverState *chr);
>> +void qemu_chr_init_mem(CharDriverState *chr, size_t size);
>>   void qemu_chr_close_mem(CharDriverState *chr);
>>   QString *qemu_chr_mem_to_qs(CharDriverState *chr);
>>   size_t qemu_chr_mem_osize(const CharDriverState *chr);
>> -- 
>> 1.7.7.6


-- 
Lei