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

[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 1/5] qemu-char: Add new char backend CircularMemCharDriver

[Lei Li]

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

diff --git a/qemu-char.c b/qemu-char.c
index b082bae..b174da1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
     return d->outbuf_size;
 }
 
+/*********************************************************/
+/*CircularMemoryr chardev*/
+
+typedef struct {
+    size_t size;
+    size_t producer;
+    size_t consumer;
+    uint8_t *cbuf;
+} CirMemCharDriver;
+
+static bool cirmem_chr_is_empty(const CharDriverState *chr)
+{
+    const CirMemCharDriver *d = chr->opaque;
+
+    return d->producer == d->consumer;
+}
+
+static bool cirmem_chr_is_full(const CharDriverState *chr)
+{
+    const CirMemCharDriver *d = chr->opaque;
+
+    return (d->producer - d->consumer) >= d->size;
+}
+
+static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    CirMemCharDriver *d = chr->opaque;
+    int i;
+
+    if (len < 0) {
+        return -1;
+    }
+
+    /* The size should be a power of 2. */
+    for (i = 0; i < len; i++ ) {
+        d->cbuf[d->producer % d->size] = buf[i];
+        d->producer++;
+    }
+
+    return 0;
+}
+
+static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
+{
+    CirMemCharDriver *d = chr->opaque;
+    int i;
+
+    if (cirmem_chr_is_empty(chr) || len < 0) {
+        return -1;
+    }
+
+    if (len > d->size) {
+        len = d->size;
+    }
+
+    for (i = 0; i < len; i++) {
+        buf[i] = d->cbuf[d->consumer % d->size];
+        d->consumer++;
+    }
+
+    return 0;
+}
+
+static void cirmem_chr_close(struct CharDriverState *chr)
+{
+    CirMemCharDriver *d = chr->opaque;
+
+    g_free(d);
+    g_free(chr->opaque);
+    chr->opaque = NULL;
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
-- 
1.7.7.6

[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     |   36 ++++++++++++++++++++++++++++++++++++
 qemu-config.c   |    3 +++
 qemu-options.hx |   10 ++++++++++
 3 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index b174da1..381bf60 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -99,6 +99,7 @@
 #include "ui/qemu-spice.h"
 
 #define READ_BUF_LEN 4096
+#define CBUFF_SIZE 65536
 
 /***********************************************************/
 /* character device */
@@ -2660,6 +2661,35 @@ static void cirmem_chr_close(struct CharDriverState *chr)
     chr->opaque = NULL;
 }
 
+static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
+{
+    CharDriverState *chr;
+    CirMemCharDriver *d;
+
+    chr = g_malloc0(sizeof(CharDriverState));
+    d = g_malloc(sizeof(*d));
+
+    d->size = qemu_opt_get_number(opts, "maxcapacity", 0);
+    if (d->size == 0) {
+        d->size = CBUFF_SIZE;
+    }
+
+    if (d->size & (d->size -1)) {
+        return NULL;
+    }
+
+    d->producer = 0;
+    d->consumer = 0;
+    d->cbuf = g_malloc0(d->size);
+
+    memset(chr, 0, sizeof(*chr));
+    chr->opaque = d;
+    chr->chr_write = cirmem_chr_write;
+    chr->chr_close = cirmem_chr_close;
+
+    return chr;
+}
+
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 {
     char host[65], port[33], width[8], height[8];
@@ -2724,6 +2754,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) {
@@ -2797,6 +2832,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 cd1ec21..5553bb6 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 7d97f96..4f90f20 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1684,6 +1684,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
@@ -1722,6 +1723,7 @@ Backend is one of:
 @option{udp},
 @option{msmouse},
 @option{vc},
+@option{memchr},
 @option{file},
 @option{pipe},
 @option{console},
@@ -1828,6 +1830,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. Should be power of 2.
+
 @item -chardev file ,id=@var{id} ,path=@var{path}
 
 Log all traffic received from the guest to a file.
-- 
1.7.7.6

[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  |   22 +++++++++++++++++
 hmp.c            |   19 +++++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-char.c      |   46 ++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   40 +++++++++++++++++++++++++++++++
 6 files changed, 197 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e0b537d..753aab3 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -825,6 +825,28 @@ Inject an NMI on the given CPU (x86 only).
 ETEXI
 
     {
+        .name       = "memchar_write",
+        .args_type  = "chardev:s,control:-b,data:s",
+        .params     = "chardev [-b] data",
+        .help       = "Provide writing interface for CirMemCharDriver. Write"
+                      "'data' to it. (Use -b for 'block' option)",
+        .mhandler.cmd = hmp_memchar_write,
+    },
+
+STEXI
+@item memchar_write @var{chardev} [-b] @var{data}
+@findex memchar_write
+Provide writing interface for CirMemCharDriver. Write @var{data}
+to cirmemchr char device. The size of the data write to the driver
+should better be power of 2.
+
+@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 70bdec2..18ca61b 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;
+    const char *chardev = qdict_get_str(qdict, "chardev");
+    const char *data = qdict_get_str(qdict, "data");
+    enum DataFormat format;
+    int con = qdict_get_try_bool(qdict, "block", 0);
+    enum CongestionControl control;
+    Error *errp = NULL;
+
+    size = strlen(data);
+    format = 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 71ea384..406ebb1 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 f9dbdae..a908aa6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -325,6 +325,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 drop.
+#
+# @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 memchardev. Write data to memchar
+# char device.
+#
+# @chardev: the name of the memchar char device.
+#
+# @size: the size to write in bytes. Should be power of 2.
+#
+# @data: the source data write to memchar.
+#
+# @format: #optional the format of the data write to memchardev, 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 memchr 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 381bf60..133d282 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2690,6 +2690,52 @@ 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)) {
+            error_setg(errp, "Failed to write to memchr %s", chardev);
+            return;
+        } else {
+            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);
+        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 2f8477e..502ed57 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -466,6 +466,46 @@ 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?",
+        .help       = "write size of data to memchar chardev",
+        .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, should be power of 2 (json-int)
+- "data": the source data writed to memchar (json-string)
+- "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-write",
+                "arguments": { "chardev": foo,
+                               "size": 8,
+                               "data": "abcdefgh",
+                               "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

[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

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

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 hmp-commands.hx |   23 +++++++++++++++++++++++
 hmp.c           |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp.h           |    1 +
 monitor.c       |   15 +++++++++++++++
 monitor.h       |    3 +++
 5 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 5f91428..f862a53 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -868,7 +868,30 @@ char device and return @var{size} of the data.
 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       = "console",
+        .args_type  = "chardev:s",
+        .params     = "chardev",
+        .help       = "Connect to the serial console from within the"
+                      "monitor, allow to write data to memchardev"
+                      "'chardev'. Exit from the console and return back"
+                      "to monitor by typing 'ctrl-]'",
+        .mhandler.cmd = hmp_console,
+    },
 
+STEXI
+@item console @var{device}
+@findex console
+
+Connect to the serial console from within the monitor, allow to write data
+to memchardev @var{chardev}. Exit from the console and return back to
+monitor by typing 'ctrl-]'.
+@example
+(qemu) console foo
+foo: data string...
+@end example
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index fa858c4..bc245f4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1245,3 +1245,56 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
     qmp_screendump(filename, &err);
     hmp_handle_error(mon, &err);
 }
+
+enum escape_char
+{
+    ESCAPE_CHAR_CTRL_GS = 0x1d  /* ctrl-] 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;
+    enum escape_char console_escape = ESCAPE_CHAR_CTRL_GS;
+
+    Error *err = NULL;
+
+    if (*data == console_escape) {
+        monitor_resume(mon);
+        return;
+    }
+
+    qmp_memchar_write(chr->label, size, data, 0, format,
+                      0, control, &err);
+    if (err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(err));
+        error_free(err);
+        return;
+    }
+
+    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);
+        goto out;
+    }
+
+    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
+        monitor_printf(mon, "Connect to console %s failed\n", device);
+    }
+
+out:
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index a5a0cfe..5b54a79 100644
--- a/hmp.h
+++ b/hmp.h
@@ -77,5 +77,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
 void hmp_send_key(Monitor *mon, const QDict *qdict);
 void hmp_screen_dump(Monitor *mon, const QDict *qdict);
+void hmp_console(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 131b325..453c084 100644
--- a/monitor.c
+++ b/monitor.c
@@ -256,6 +256,21 @@ 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;
+    }
+
+    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 b6e7d95..735bd1b 100644
--- a/monitor.h
+++ b/monitor.h
@@ -86,6 +86,9 @@ 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);
 
 int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/5] qemu-char: Add new char backend CircularMemCharDriver

[Eric Blake]

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

On 10/21/2012 10:47 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qemu-char.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 72 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..b174da1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
>      return d->outbuf_size;
>  }
>  
> +/*********************************************************/
> +/*CircularMemoryr chardev*/

s/CircularMemoryr/CircularMemory/


> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    CirMemCharDriver *d = chr->opaque;
> +    int i;
> +
> +    if (len < 0) {
> +        return -1;
> +    }
> +
> +    /* The size should be a power of 2. */

Shouldn't you enforce that, then?

-- 
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 2/5] Expose CirMemCharDriver via command line

[Luiz Capitulino]

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

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  qemu-char.c     |   36 ++++++++++++++++++++++++++++++++++++
>  qemu-config.c   |    3 +++
>  qemu-options.hx |   10 ++++++++++
>  3 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index b174da1..381bf60 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -99,6 +99,7 @@
>  #include "ui/qemu-spice.h"
>  
>  #define READ_BUF_LEN 4096
> +#define CBUFF_SIZE 65536
>  
>  /***********************************************************/
>  /* character device */
> @@ -2660,6 +2661,35 @@ static void cirmem_chr_close(struct CharDriverState *chr)
>      chr->opaque = NULL;
>  }
>  
> +static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
> +{
> +    CharDriverState *chr;
> +    CirMemCharDriver *d;
> +
> +    chr = g_malloc0(sizeof(CharDriverState));
> +    d = g_malloc(sizeof(*d));
> +
> +    d->size = qemu_opt_get_number(opts, "maxcapacity", 0);
> +    if (d->size == 0) {
> +        d->size = CBUFF_SIZE;
> +    }
> +
> +    if (d->size & (d->size -1)) {
> +        return NULL;
> +    }
> +
> +    d->producer = 0;
> +    d->consumer = 0;
> +    d->cbuf = g_malloc0(d->size);
> +
> +    memset(chr, 0, sizeof(*chr));

This has already been initialized by g_malloc0().

> +    chr->opaque = d;
> +    chr->chr_write = cirmem_chr_write;
> +    chr->chr_close = cirmem_chr_close;
> +
> +    return chr;
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];
> @@ -2724,6 +2754,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) {
> @@ -2797,6 +2832,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 },

Why not call it "memory"?

>  #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 cd1ec21..5553bb6 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 7d97f96..4f90f20 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1684,6 +1684,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
> @@ -1722,6 +1723,7 @@ Backend is one of:
>  @option{udp},
>  @option{msmouse},
>  @option{vc},
> +@option{memchr},
>  @option{file},
>  @option{pipe},
>  @option{console},
> @@ -1828,6 +1830,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. Should be power of 2.
> +
>  @item -chardev file ,id=@var{id} ,path=@var{path}
>  
>  Log all traffic received from the guest to a file.

Re: [Qemu-devel] [PATCH 1/5] qemu-char: Add new char backend CircularMemCharDriver

[Luiz Capitulino]

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

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

This patch should be squashed in the next one. More comments below.

> ---
>  qemu-char.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 72 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..b174da1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
>      return d->outbuf_size;
>  }
>  
> +/*********************************************************/
> +/*CircularMemoryr chardev*/

s/Memoryr/Memory

> +
> +typedef struct {
> +    size_t size;
> +    size_t producer;
> +    size_t consumer;
> +    uint8_t *cbuf;
> +} CirMemCharDriver;
> +
> +static bool cirmem_chr_is_empty(const CharDriverState *chr)
> +{
> +    const CirMemCharDriver *d = chr->opaque;
> +
> +    return d->producer == d->consumer;
> +}
> +
> +static bool cirmem_chr_is_full(const CharDriverState *chr)
> +{
> +    const CirMemCharDriver *d = chr->opaque;
> +
> +    return (d->producer - d->consumer) >= d->size;
> +}
> +
> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    CirMemCharDriver *d = chr->opaque;
> +    int i;
> +
> +    if (len < 0) {
> +        return -1;
> +    }
> +
> +    /* The size should be a power of 2. */
> +    for (i = 0; i < len; i++ ) {
> +        d->cbuf[d->producer % d->size] = buf[i];
> +        d->producer++;
> +    }
> +
> +    return 0;
> +}
> +
> +static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
> +{
> +    CirMemCharDriver *d = chr->opaque;
> +    int i;
> +
> +    if (cirmem_chr_is_empty(chr) || len < 0) {
> +        return -1;
> +    }
> +
> +    if (len > d->size) {
> +        len = d->size;
> +    }
> +
> +    for (i = 0; i < len; i++) {
> +        buf[i] = d->cbuf[d->consumer % d->size];
> +        d->consumer++;
> +    }
> +
> +    return 0;
> +}

You don't seem to reset producer/consumer anywhere, I wonder if it's possible
for a long running VM to trigger the limit here.

> +
> +static void cirmem_chr_close(struct CharDriverState *chr)
> +{
> +    CirMemCharDriver *d = chr->opaque;
> +
> +    g_free(d);
> +    g_free(chr->opaque);

Double free. I think you want to free cbuf, like:

g_free(d->cbuf);
g_free(d);

> +    chr->opaque = NULL;
> +}
> +
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
>  {
>      char host[65], port[33], width[8], height[8];

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

[Luiz Capitulino]

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

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |   22 +++++++++++++++++
>  hmp.c            |   19 +++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-char.c      |   46 ++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   40 +++++++++++++++++++++++++++++++
>  6 files changed, 197 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e0b537d..753aab3 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -825,6 +825,28 @@ Inject an NMI on the given CPU (x86 only).
>  ETEXI
>  
>      {
> +        .name       = "memchar_write",
> +        .args_type  = "chardev:s,control:-b,data:s",
> +        .params     = "chardev [-b] data",
> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
> +                      "'data' to it. (Use -b for 'block' option)",
> +        .mhandler.cmd = hmp_memchar_write,

Honest question: is this (and memchr_read) really useful? Isn't
the console command alone good enough?

> +    },
> +
> +STEXI
> +@item memchar_write @var{chardev} [-b] @var{data}
> +@findex memchar_write
> +Provide writing interface for CirMemCharDriver. Write @var{data}
> +to cirmemchr char device. The size of the data write to the driver
> +should better be power of 2.

As this is a human interface, it makes sense to round-up automatically.
Actually, you don't even accept a size parameter :)

> +
> +@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 70bdec2..18ca61b 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;
> +    const char *chardev = qdict_get_str(qdict, "chardev");
> +    const char *data = qdict_get_str(qdict, "data");
> +    enum DataFormat format;
> +    int con = qdict_get_try_bool(qdict, "block", 0);
> +    enum CongestionControl control;
> +    Error *errp = NULL;
> +
> +    size = strlen(data);
> +    format = 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 71ea384..406ebb1 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 f9dbdae..a908aa6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -325,6 +325,75 @@
>  { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>  
>  ##
> +# @DataFormat:

DataEncoding?

> +#
> +# An enumeration of data format write to or read from
> +# memchardev. The default value would be utf8.

This is generic enough, don't need to mention memchardev.

> +#
> +# @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 drop.
> +#
> +# @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.

This is not good, as an app would have to try and error 'block' to see
if it's supported.

My suggestion is to drop all CongestionControl bits and assume a dropping
behavior for this command. Later, when we add async support to QMP we can
add an async version of this command.

> +#
> +# Since: 1.3
> +##
> +{'enum': 'CongestionControl'
> + 'data': [ 'drop', 'block' ] }
> +
> +##
> +# @memchar-write:
> +#
> +# Provide writing interface for memchardev. Write data to memchar
> +# char device.
> +#
> +# @chardev: the name of the memchar char device.

I wonder if "device" is better than "chardev".

> +#
> +# @size: the size to write in bytes. Should be power of 2.
> +#
> +# @data: the source data write to memchar.
> +#
> +# @format: #optional the format of the data write to memchardev, 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 memchr device, DeviceNotFound
> +#          If an I/O error occurs while writing, IOError

Please, drop the IOError line as this error doesn't exist anymore. The
DeviceNotFound one is fine.

> +#
> +# 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 381bf60..133d282 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2690,6 +2690,52 @@ 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)) {
> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> +            return;
> +        } else {
> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> +            return;
> +        }
> +    }

As I said above, I think you should drop all this stuff.

> +
> +    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);

What if chr is not a memory device?

> +
> +    if (ret < 0) {
> +        error_setg(errp, "Failed to write to memchr %s", chardev);
> +        return;
> +    }

Would be nice to return -errno codes from cirmem_chr_write() and use
error_setg_errno() (not in tree yet). Considering we're allowed to return
-errno, of course.

> +}
> +
>  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 2f8477e..502ed57 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -466,6 +466,46 @@ 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?",
> +        .help       = "write size of data to memchar chardev",
> +        .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, should be power of 2 (json-int)
> +- "data": the source data writed to memchar (json-string)
> +- "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-write",
> +                "arguments": { "chardev": foo,
> +                               "size": 8,
> +                               "data": "abcdefgh",
> +                               "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 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,

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

[Luiz Capitulino]

On Mon, 22 Oct 2012 00:48:01 +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           |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h           |    1 +
>  monitor.c       |   15 +++++++++++++++
>  monitor.h       |    3 +++
>  5 files changed, 95 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 5f91428..f862a53 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -868,7 +868,30 @@ char device and return @var{size} of the data.
>  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       = "console",
> +        .args_type  = "chardev:s",
> +        .params     = "chardev",
> +        .help       = "Connect to the serial console from within the"
> +                      "monitor, allow to write data to memchardev"
> +                      "'chardev'. Exit from the console and return back"
> +                      "to monitor by typing 'ctrl-]'",
> +        .mhandler.cmd = hmp_console,
> +    },
>  
> +STEXI
> +@item console @var{device}
> +@findex console
> +
> +Connect to the serial console from within the monitor, allow to write data
> +to memchardev @var{chardev}. Exit from the console and return back to
> +monitor by typing 'ctrl-]'.
> +@example
> +(qemu) console foo
> +foo: data string...
> +@end example
>  ETEXI
>  
>      {
> diff --git a/hmp.c b/hmp.c
> index fa858c4..bc245f4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1245,3 +1245,56 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>      qmp_screendump(filename, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +enum escape_char
> +{
> +    ESCAPE_CHAR_CTRL_GS = 0x1d  /* ctrl-] 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;
> +    enum escape_char console_escape = ESCAPE_CHAR_CTRL_GS;
> +
> +    Error *err = NULL;
> +
> +    if (*data == console_escape) {
> +        monitor_resume(mon);
> +        return;
> +    }
> +
> +    qmp_memchar_write(chr->label, size, data, 0, format,
> +                      0, control, &err);
> +    if (err) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> +        error_free(err);
> +        return;
> +    }

Shouldn't you also read from the device?

> +
> +    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);
> +        goto out;
> +    }

No need to do this here as the QMP command will do it too.

> +
> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
> +        monitor_printf(mon, "Connect to console %s failed\n", device);
> +    }
> +
> +out:
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index a5a0cfe..5b54a79 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -77,5 +77,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
>  void hmp_closefd(Monitor *mon, const QDict *qdict);
>  void hmp_send_key(Monitor *mon, const QDict *qdict);
>  void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> +void hmp_console(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index 131b325..453c084 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -256,6 +256,21 @@ 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;
> +    }
> +
> +    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 b6e7d95..735bd1b 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -86,6 +86,9 @@ 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);
>  
>  int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);

Re: [Qemu-devel] [PATCH 1/5] qemu-char: Add new char backend CircularMemCharDriver

[Lei Li]

On 10/22/2012 10:08 PM, Eric Blake wrote:
> On 10/21/2012 10:47 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   qemu-char.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 72 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index b082bae..b174da1 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
>>       return d->outbuf_size;
>>   }
>>   
>> +/*********************************************************/
>> +/*CircularMemoryr chardev*/
> s/CircularMemoryr/CircularMemory/
>
Yeah, I just found it...
Thanks!

>> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>> +{
>> +    CirMemCharDriver *d = chr->opaque;
>> +    int i;
>> +
>> +    if (len < 0) {
>> +        return -1;
>> +    }
>> +
>> +    /* The size should be a power of 2. */
> Shouldn't you enforce that, then?

Yes, it has been checked when open the CirMemChar backend in patch 2/5,
as code below:

if (d->size & (d->size -1)) {
     return NULL;
}



-- 
Lei

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

[Lei Li]

On 10/23/2012 02:37 AM, Luiz Capitulino wrote:
> On Mon, 22 Oct 2012 00:47:59 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx  |   22 +++++++++++++++++
>>   hmp.c            |   19 +++++++++++++++
>>   hmp.h            |    1 +
>>   qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qemu-char.c      |   46 ++++++++++++++++++++++++++++++++++++
>>   qmp-commands.hx  |   40 +++++++++++++++++++++++++++++++
>>   6 files changed, 197 insertions(+), 0 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index e0b537d..753aab3 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -825,6 +825,28 @@ Inject an NMI on the given CPU (x86 only).
>>   ETEXI
>>   
>>       {
>> +        .name       = "memchar_write",
>> +        .args_type  = "chardev:s,control:-b,data:s",
>> +        .params     = "chardev [-b] data",
>> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
>> +                      "'data' to it. (Use -b for 'block' option)",
>> +        .mhandler.cmd = hmp_memchar_write,
> Honest question: is this (and memchr_read) really useful? Isn't
> the console command alone good enough?
>
>> +    },
>> +
>> +STEXI
>> +@item memchar_write @var{chardev} [-b] @var{data}
>> +@findex memchar_write
>> +Provide writing interface for CirMemCharDriver. Write @var{data}
>> +to cirmemchr char device. The size of the data write to the driver
>> +should better be power of 2.
> As this is a human interface, it makes sense to round-up automatically.
> Actually, you don't even accept a size parameter :)

Yeah, I have take your suggestion drop the size parameters by calculating,
but I forgot to get rid of the comment here...  :-[

>> +
>> +@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 70bdec2..18ca61b 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;
>> +    const char *chardev = qdict_get_str(qdict, "chardev");
>> +    const char *data = qdict_get_str(qdict, "data");
>> +    enum DataFormat format;
>> +    int con = qdict_get_try_bool(qdict, "block", 0);
>> +    enum CongestionControl control;
>> +    Error *errp = NULL;
>> +
>> +    size = strlen(data);
>> +    format = 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 71ea384..406ebb1 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 f9dbdae..a908aa6 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -325,6 +325,75 @@
>>   { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
>>   
>>   ##
>> +# @DataFormat:
> DataEncoding?
>
>> +#
>> +# An enumeration of data format write to or read from
>> +# memchardev. The default value would be utf8.
> This is generic enough, don't need to mention memchardev.
>
>> +#
>> +# @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 drop.
>> +#
>> +# @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.
> This is not good, as an app would have to try and error 'block' to see
> if it's supported.
>
> My suggestion is to drop all CongestionControl bits and assume a dropping
> behavior for this command. Later, when we add async support to QMP we can
> add an async version of this command.

How about keep the CongestionControl, support both options but let it all behaver
like 'drop'? Then it's easy to add async support by just modifying operation for
'CONGESTION_CONTROL_BLOCK' in qmp_memchar_write/read command.
But if you insist, I will drop the CongestionControl bits for now.

>
>> +#
>> +# Since: 1.3
>> +##
>> +{'enum': 'CongestionControl'
>> + 'data': [ 'drop', 'block' ] }
>> +
>> +##
>> +# @memchar-write:
>> +#
>> +# Provide writing interface for memchardev. Write data to memchar
>> +# char device.
>> +#
>> +# @chardev: the name of the memchar char device.
> I wonder if "device" is better than "chardev".

OK.

>> +#
>> +# @size: the size to write in bytes. Should be power of 2.
>> +#
>> +# @data: the source data write to memchar.
>> +#
>> +# @format: #optional the format of the data write to memchardev, 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 memchr device, DeviceNotFound
>> +#          If an I/O error occurs while writing, IOError
> Please, drop the IOError line as this error doesn't exist anymore. The
> DeviceNotFound one is fine.

OK.

>> +#
>> +# 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 381bf60..133d282 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2690,6 +2690,52 @@ 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)) {
>> +            error_setg(errp, "Failed to write to memchr %s", chardev);
>> +            return;
>> +        } else {
>> +            error_setg(errp, "Failed to write to memchr %s", chardev);
>> +            return;
>> +        }
>> +    }
> As I said above, I think you should drop all this stuff.
>
>> +
>> +    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);
> What if chr is not a memory device?

Good question! I have not considered this situation...

>
>> +
>> +    if (ret < 0) {
>> +        error_setg(errp, "Failed to write to memchr %s", chardev);
>> +        return;
>> +    }
> Would be nice to return -errno codes from cirmem_chr_write() and use
> error_setg_errno() (not in tree yet). Considering we're allowed to return
> -errno, of course.

Sure.

>> +}
>> +
>>   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 2f8477e..502ed57 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -466,6 +466,46 @@ 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?",
>> +        .help       = "write size of data to memchar chardev",
>> +        .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, should be power of 2 (json-int)
>> +- "data": the source data writed to memchar (json-string)
>> +- "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-write",
>> +                "arguments": { "chardev": foo,
>> +                               "size": 8,
>> +                               "data": "abcdefgh",
>> +                               "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 1/5] qemu-char: Add new char backend CircularMemCharDriver

[Lei Li]

On 10/23/2012 02:14 AM, Luiz Capitulino wrote:
> On Mon, 22 Oct 2012 00:47:57 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> This patch should be squashed in the next one. More comments below.
>
>> ---
>>   qemu-char.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 72 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index b082bae..b174da1 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2588,6 +2588,78 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
>>       return d->outbuf_size;
>>   }
>>   
>> +/*********************************************************/
>> +/*CircularMemoryr chardev*/
> s/Memoryr/Memory
>
>> +
>> +typedef struct {
>> +    size_t size;
>> +    size_t producer;
>> +    size_t consumer;
>> +    uint8_t *cbuf;
>> +} CirMemCharDriver;
>> +
>> +static bool cirmem_chr_is_empty(const CharDriverState *chr)
>> +{
>> +    const CirMemCharDriver *d = chr->opaque;
>> +
>> +    return d->producer == d->consumer;
>> +}
>> +
>> +static bool cirmem_chr_is_full(const CharDriverState *chr)
>> +{
>> +    const CirMemCharDriver *d = chr->opaque;
>> +
>> +    return (d->producer - d->consumer) >= d->size;
>> +}
>> +
>> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>> +{
>> +    CirMemCharDriver *d = chr->opaque;
>> +    int i;
>> +
>> +    if (len < 0) {
>> +        return -1;
>> +    }
>> +
>> +    /* The size should be a power of 2. */
>> +    for (i = 0; i < len; i++ ) {
>> +        d->cbuf[d->producer % d->size] = buf[i];
>> +        d->producer++;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
>> +{
>> +    CirMemCharDriver *d = chr->opaque;
>> +    int i;
>> +
>> +    if (cirmem_chr_is_empty(chr) || len < 0) {
>> +        return -1;
>> +    }
>> +
>> +    if (len > d->size) {
>> +        len = d->size;
>> +    }
>> +
>> +    for (i = 0; i < len; i++) {
>> +        buf[i] = d->cbuf[d->consumer % d->size];
>> +        d->consumer++;
>> +    }
>> +
>> +    return 0;
>> +}
> You don't seem to reset producer/consumer anywhere, I wonder if it's possible
> for a long running VM to trigger the limit here.

Yes, it make sense.


-- 
Lei

Re: [Qemu-devel] [PATCH 1/5] qemu-char: Add new char backend CircularMemCharDriver

[Luiz Capitulino]

On Tue, 23 Oct 2012 13:40:13 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> >> +static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> >> +{
> >> +    CirMemCharDriver *d = chr->opaque;
> >> +    int i;
> >> +
> >> +    if (len < 0) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    /* The size should be a power of 2. */
> > Shouldn't you enforce that, then?
> 
> Yes, it has been checked when open the CirMemChar backend in patch 2/5,
> as code below:
> 
> if (d->size & (d->size -1)) {
>      return NULL;
> }

You could add an assert() in cirmem_chr_write() then.

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

[Luiz Capitulino]

On Tue, 23 Oct 2012 14:36:17 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> On 10/23/2012 02:37 AM, Luiz Capitulino wrote:
> > On Mon, 22 Oct 2012 00:47:59 +0800
> > Lei Li <lilei@linux.vnet.ibm.com> wrote:
> >
> >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> >> ---
> >>   hmp-commands.hx  |   22 +++++++++++++++++
> >>   hmp.c            |   19 +++++++++++++++
> >>   hmp.h            |    1 +
> >>   qapi-schema.json |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   qemu-char.c      |   46 ++++++++++++++++++++++++++++++++++++
> >>   qmp-commands.hx  |   40 +++++++++++++++++++++++++++++++
> >>   6 files changed, 197 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index e0b537d..753aab3 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -825,6 +825,28 @@ Inject an NMI on the given CPU (x86 only).
> >>   ETEXI
> >>   
> >>       {
> >> +        .name       = "memchar_write",
> >> +        .args_type  = "chardev:s,control:-b,data:s",
> >> +        .params     = "chardev [-b] data",
> >> +        .help       = "Provide writing interface for CirMemCharDriver. Write"
> >> +                      "'data' to it. (Use -b for 'block' option)",
> >> +        .mhandler.cmd = hmp_memchar_write,
> > Honest question: is this (and memchr_read) really useful? Isn't
> > the console command alone good enough?
> >
> >> +    },
> >> +
> >> +STEXI
> >> +@item memchar_write @var{chardev} [-b] @var{data}
> >> +@findex memchar_write
> >> +Provide writing interface for CirMemCharDriver. Write @var{data}
> >> +to cirmemchr char device. The size of the data write to the driver
> >> +should better be power of 2.
> > As this is a human interface, it makes sense to round-up automatically.
> > Actually, you don't even accept a size parameter :)
> 
> Yeah, I have take your suggestion drop the size parameters by calculating,
> but I forgot to get rid of the comment here...  :-[
> 
> >> +
> >> +@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 70bdec2..18ca61b 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;
> >> +    const char *chardev = qdict_get_str(qdict, "chardev");
> >> +    const char *data = qdict_get_str(qdict, "data");
> >> +    enum DataFormat format;
> >> +    int con = qdict_get_try_bool(qdict, "block", 0);
> >> +    enum CongestionControl control;
> >> +    Error *errp = NULL;
> >> +
> >> +    size = strlen(data);
> >> +    format = 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 71ea384..406ebb1 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 f9dbdae..a908aa6 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -325,6 +325,75 @@
> >>   { 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
> >>   
> >>   ##
> >> +# @DataFormat:
> > DataEncoding?
> >
> >> +#
> >> +# An enumeration of data format write to or read from
> >> +# memchardev. The default value would be utf8.
> > This is generic enough, don't need to mention memchardev.
> >
> >> +#
> >> +# @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 drop.
> >> +#
> >> +# @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.
> > This is not good, as an app would have to try and error 'block' to see
> > if it's supported.
> >
> > My suggestion is to drop all CongestionControl bits and assume a dropping
> > behavior for this command. Later, when we add async support to QMP we can
> > add an async version of this command.
> 
> How about keep the CongestionControl, support both options but let it all behaver
> like 'drop'? Then it's easy to add async support by just modifying operation for
> 'CONGESTION_CONTROL_BLOCK' in qmp_memchar_write/read command.

That's worse, because you'd change the option's semantics.

> But if you insist, I will drop the CongestionControl bits for now.
> 
> >
> >> +#
> >> +# Since: 1.3
> >> +##
> >> +{'enum': 'CongestionControl'
> >> + 'data': [ 'drop', 'block' ] }
> >> +
> >> +##
> >> +# @memchar-write:
> >> +#
> >> +# Provide writing interface for memchardev. Write data to memchar
> >> +# char device.
> >> +#
> >> +# @chardev: the name of the memchar char device.
> > I wonder if "device" is better than "chardev".
> 
> OK.
> 
> >> +#
> >> +# @size: the size to write in bytes. Should be power of 2.
> >> +#
> >> +# @data: the source data write to memchar.
> >> +#
> >> +# @format: #optional the format of the data write to memchardev, 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 memchr device, DeviceNotFound
> >> +#          If an I/O error occurs while writing, IOError
> > Please, drop the IOError line as this error doesn't exist anymore. The
> > DeviceNotFound one is fine.
> 
> OK.
> 
> >> +#
> >> +# 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 381bf60..133d282 100644
> >> --- a/qemu-char.c
> >> +++ b/qemu-char.c
> >> @@ -2690,6 +2690,52 @@ 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)) {
> >> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> >> +            return;
> >> +        } else {
> >> +            error_setg(errp, "Failed to write to memchr %s", chardev);
> >> +            return;
> >> +        }
> >> +    }
> > As I said above, I think you should drop all this stuff.
> >
> >> +
> >> +    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);
> > What if chr is not a memory device?
> 
> Good question! I have not considered this situation...
> 
> >
> >> +
> >> +    if (ret < 0) {
> >> +        error_setg(errp, "Failed to write to memchr %s", chardev);
> >> +        return;
> >> +    }
> > Would be nice to return -errno codes from cirmem_chr_write() and use
> > error_setg_errno() (not in tree yet). Considering we're allowed to return
> > -errno, of course.
> 
> Sure.
> 
> >> +}
> >> +
> >>   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 2f8477e..502ed57 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -466,6 +466,46 @@ 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?",
> >> +        .help       = "write size of data to memchar chardev",
> >> +        .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, should be power of 2 (json-int)
> >> +- "data": the source data writed to memchar (json-string)
> >> +- "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-write",
> >> +                "arguments": { "chardev": foo,
> >> +                               "size": 8,
> >> +                               "data": "abcdefgh",
> >> +                               "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 5/5] HMP: Introduce console command

[Lei Li]

On 10/23/2012 02:59 AM, Luiz Capitulino wrote:
> On Mon, 22 Oct 2012 00:48:01 +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           |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hmp.h           |    1 +
>>   monitor.c       |   15 +++++++++++++++
>>   monitor.h       |    3 +++
>>   5 files changed, 95 insertions(+), 0 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 5f91428..f862a53 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -868,7 +868,30 @@ char device and return @var{size} of the data.
>>   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       = "console",
>> +        .args_type  = "chardev:s",
>> +        .params     = "chardev",
>> +        .help       = "Connect to the serial console from within the"
>> +                      "monitor, allow to write data to memchardev"
>> +                      "'chardev'. Exit from the console and return back"
>> +                      "to monitor by typing 'ctrl-]'",
>> +        .mhandler.cmd = hmp_console,
>> +    },
>>   
>> +STEXI
>> +@item console @var{device}
>> +@findex console
>> +
>> +Connect to the serial console from within the monitor, allow to write data
>> +to memchardev @var{chardev}. Exit from the console and return back to
>> +monitor by typing 'ctrl-]'.
>> +@example
>> +(qemu) console foo
>> +foo: data string...
>> +@end example
>>   ETEXI
>>   
>>       {
>> diff --git a/hmp.c b/hmp.c
>> index fa858c4..bc245f4 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1245,3 +1245,56 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>>       qmp_screendump(filename, &err);
>>       hmp_handle_error(mon, &err);
>>   }
>> +
>> +enum escape_char
>> +{
>> +    ESCAPE_CHAR_CTRL_GS = 0x1d  /* ctrl-] 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;
>> +    enum escape_char console_escape = ESCAPE_CHAR_CTRL_GS;
>> +
>> +    Error *err = NULL;
>> +
>> +    if (*data == console_escape) {
>> +        monitor_resume(mon);
>> +        return;
>> +    }
>> +
>> +    qmp_memchar_write(chr->label, size, data, 0, format,
>> +                      0, control, &err);
>> +    if (err) {
>> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
>> +        error_free(err);
>> +        return;
>> +    }
> Shouldn't you also read from the device?

The use-case for this console command is just allow the user to
write data to each memchar device as in a signal terminal. Then
type escape sequences to take you back to the monitor. So I don't
think it is also need to read from the device in this command.

BTW, we can read from the device by hmp_memchar_read. :)

>> +
>> +    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);
>> +        goto out;
>> +    }
> No need to do this here as the QMP command will do it too.
>
I think we should do this check here, otherwise would
cause core dump when 'console' to a chardev that does not
exist. Wepass parameterchr->label by qmp_memchar_write()
in the handler hmp_read_console.

>> +
>> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
>> +        monitor_printf(mon, "Connect to console %s failed\n", device);
>> +    }
>> +
>> +out:
>> +    hmp_handle_error(mon, &err);
>> +}
>> diff --git a/hmp.h b/hmp.h
>> index a5a0cfe..5b54a79 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -77,5 +77,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
>>   void hmp_closefd(Monitor *mon, const QDict *qdict);
>>   void hmp_send_key(Monitor *mon, const QDict *qdict);
>>   void hmp_screen_dump(Monitor *mon, const QDict *qdict);
>> +void hmp_console(Monitor *mon, const QDict *qdict);
>>   
>>   #endif
>> diff --git a/monitor.c b/monitor.c
>> index 131b325..453c084 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -256,6 +256,21 @@ 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;
>> +    }
>> +
>> +    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 b6e7d95..735bd1b 100644
>> --- a/monitor.h
>> +++ b/monitor.h
>> @@ -86,6 +86,9 @@ 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);
>>   
>>   int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
>


-- 
Lei

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

[Luiz Capitulino]

On Wed, 24 Oct 2012 15:17:21 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> On 10/23/2012 02:59 AM, Luiz Capitulino wrote:
> > On Mon, 22 Oct 2012 00:48:01 +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           |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   hmp.h           |    1 +
> >>   monitor.c       |   15 +++++++++++++++
> >>   monitor.h       |    3 +++
> >>   5 files changed, 95 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index 5f91428..f862a53 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -868,7 +868,30 @@ char device and return @var{size} of the data.
> >>   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       = "console",
> >> +        .args_type  = "chardev:s",
> >> +        .params     = "chardev",
> >> +        .help       = "Connect to the serial console from within the"
> >> +                      "monitor, allow to write data to memchardev"
> >> +                      "'chardev'. Exit from the console and return back"
> >> +                      "to monitor by typing 'ctrl-]'",
> >> +        .mhandler.cmd = hmp_console,
> >> +    },
> >>   
> >> +STEXI
> >> +@item console @var{device}
> >> +@findex console
> >> +
> >> +Connect to the serial console from within the monitor, allow to write data
> >> +to memchardev @var{chardev}. Exit from the console and return back to
> >> +monitor by typing 'ctrl-]'.
> >> +@example
> >> +(qemu) console foo
> >> +foo: data string...
> >> +@end example
> >>   ETEXI
> >>   
> >>       {
> >> diff --git a/hmp.c b/hmp.c
> >> index fa858c4..bc245f4 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -1245,3 +1245,56 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
> >>       qmp_screendump(filename, &err);
> >>       hmp_handle_error(mon, &err);
> >>   }
> >> +
> >> +enum escape_char
> >> +{
> >> +    ESCAPE_CHAR_CTRL_GS = 0x1d  /* ctrl-] 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;
> >> +    enum escape_char console_escape = ESCAPE_CHAR_CTRL_GS;
> >> +
> >> +    Error *err = NULL;
> >> +
> >> +    if (*data == console_escape) {
> >> +        monitor_resume(mon);
> >> +        return;
> >> +    }
> >> +
> >> +    qmp_memchar_write(chr->label, size, data, 0, format,
> >> +                      0, control, &err);
> >> +    if (err) {
> >> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> >> +        error_free(err);
> >> +        return;
> >> +    }
> > Shouldn't you also read from the device?
> 
> The use-case for this console command is just allow the user to
> write data to each memchar device as in a signal terminal. Then
> type escape sequences to take you back to the monitor. So I don't
> think it is also need to read from the device in this command.
> 
> BTW, we can read from the device by hmp_memchar_read. :)

And how is the console command better than the hmp_memchar_write one?

Could you please give examples?

> >> +
> >> +    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);
> >> +        goto out;
> >> +    }
> > No need to do this here as the QMP command will do it too.
> >
> I think we should do this check here, otherwise would
> cause core dump when 'console' to a chardev that does not
> exist. Wepass parameterchr->label by qmp_memchar_write()
> in the handler hmp_read_console.

Do you need the chr object? Why don't you just pass 'device' to
monitor_read_console()?

> 
> >> +
> >> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
> >> +        monitor_printf(mon, "Connect to console %s failed\n", device);
> >> +    }
> >> +
> >> +out:
> >> +    hmp_handle_error(mon, &err);
> >> +}
> >> diff --git a/hmp.h b/hmp.h
> >> index a5a0cfe..5b54a79 100644
> >> --- a/hmp.h
> >> +++ b/hmp.h
> >> @@ -77,5 +77,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
> >>   void hmp_closefd(Monitor *mon, const QDict *qdict);
> >>   void hmp_send_key(Monitor *mon, const QDict *qdict);
> >>   void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> >> +void hmp_console(Monitor *mon, const QDict *qdict);
> >>   
> >>   #endif
> >> diff --git a/monitor.c b/monitor.c
> >> index 131b325..453c084 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -256,6 +256,21 @@ 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;
> >> +    }
> >> +
> >> +    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 b6e7d95..735bd1b 100644
> >> --- a/monitor.h
> >> +++ b/monitor.h
> >> @@ -86,6 +86,9 @@ 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);
> >>   
> >>   int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
> >
> 
> 

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

[Lei Li]

On 10/24/2012 08:55 PM, Luiz Capitulino wrote:
> On Wed, 24 Oct 2012 15:17:21 +0800
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>
>> On 10/23/2012 02:59 AM, Luiz Capitulino wrote:
>>> On Mon, 22 Oct 2012 00:48:01 +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           |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    hmp.h           |    1 +
>>>>    monitor.c       |   15 +++++++++++++++
>>>>    monitor.h       |    3 +++
>>>>    5 files changed, 95 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>>> index 5f91428..f862a53 100644
>>>> --- a/hmp-commands.hx
>>>> +++ b/hmp-commands.hx
>>>> @@ -868,7 +868,30 @@ char device and return @var{size} of the data.
>>>>    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       = "console",
>>>> +        .args_type  = "chardev:s",
>>>> +        .params     = "chardev",
>>>> +        .help       = "Connect to the serial console from within the"
>>>> +                      "monitor, allow to write data to memchardev"
>>>> +                      "'chardev'. Exit from the console and return back"
>>>> +                      "to monitor by typing 'ctrl-]'",
>>>> +        .mhandler.cmd = hmp_console,
>>>> +    },
>>>>    
>>>> +STEXI
>>>> +@item console @var{device}
>>>> +@findex console
>>>> +
>>>> +Connect to the serial console from within the monitor, allow to write data
>>>> +to memchardev @var{chardev}. Exit from the console and return back to
>>>> +monitor by typing 'ctrl-]'.
>>>> +@example
>>>> +(qemu) console foo
>>>> +foo: data string...
>>>> +@end example
>>>>    ETEXI
>>>>    
>>>>        {
>>>> diff --git a/hmp.c b/hmp.c
>>>> index fa858c4..bc245f4 100644
>>>> --- a/hmp.c
>>>> +++ b/hmp.c
>>>> @@ -1245,3 +1245,56 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>>>>        qmp_screendump(filename, &err);
>>>>        hmp_handle_error(mon, &err);
>>>>    }
>>>> +
>>>> +enum escape_char
>>>> +{
>>>> +    ESCAPE_CHAR_CTRL_GS = 0x1d  /* ctrl-] 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;
>>>> +    enum escape_char console_escape = ESCAPE_CHAR_CTRL_GS;
>>>> +
>>>> +    Error *err = NULL;
>>>> +
>>>> +    if (*data == console_escape) {
>>>> +        monitor_resume(mon);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qmp_memchar_write(chr->label, size, data, 0, format,
>>>> +                      0, control, &err);
>>>> +    if (err) {
>>>> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
>>>> +        error_free(err);
>>>> +        return;
>>>> +    }
>>> Shouldn't you also read from the device?
>> The use-case for this console command is just allow the user to
>> write data to each memchar device as in a signal terminal. Then
>> type escape sequences to take you back to the monitor. So I don't
>> think it is also need to read from the device in this command.
>>
>> BTW, we can read from the device by hmp_memchar_read. :)
> And how is the console command better than the hmp_memchar_write one?
>
> Could you please give examples?

Sure. The "console" command behaves like:

(qemu) console foo
foo: hello world
(qemu)

you can input data, then put enter or ctrl-] to return back to
the monitor. The data input from console would be written to the
CirMemCharDriver backend, which provide a more friendly and nice
HMP command as Anthony suggested.


>
>>>> +
>>>> +    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);
>>>> +        goto out;
>>>> +    }
>>> No need to do this here as the QMP command will do it too.
>>>
>> I think we should do this check here, otherwise would
>> cause core dump when 'console' to a chardev that does not
>> exist. Wepass parameterchr->label by qmp_memchar_write()
>> in the handler hmp_read_console.
> Do you need the chr object? Why don't you just pass 'device' to
> monitor_read_console()?

Yes, we need 'device' and 'chr' both here. 'device' pass to monitor_read_console
for the friendly prompt. 'chr' pass to handler hmp_read_console, the reason did not
pass 'device' directly to it is the limitation of argument for the handler
hmp_read_console. Also it's better to give the prompt before the user enter the input
console if the device not found, the use-case as I post above.

>>>> +
>>>> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
>>>> +        monitor_printf(mon, "Connect to console %s failed\n", device);
>>>> +    }
>>>> +
>>>> +out:
>>>> +    hmp_handle_error(mon, &err);
>>>> +}
>>>> diff --git a/hmp.h b/hmp.h
>>>> index a5a0cfe..5b54a79 100644
>>>> --- a/hmp.h
>>>> +++ b/hmp.h
>>>> @@ -77,5 +77,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
>>>>    void hmp_closefd(Monitor *mon, const QDict *qdict);
>>>>    void hmp_send_key(Monitor *mon, const QDict *qdict);
>>>>    void hmp_screen_dump(Monitor *mon, const QDict *qdict);
>>>> +void hmp_console(Monitor *mon, const QDict *qdict);
>>>>    
>>>>    #endif
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 131b325..453c084 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -256,6 +256,21 @@ 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;
>>>> +    }
>>>> +
>>>> +    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 b6e7d95..735bd1b 100644
>>>> --- a/monitor.h
>>>> +++ b/monitor.h
>>>> @@ -86,6 +86,9 @@ 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);
>>>>    
>>>>    int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
>>
>


-- 
Lei

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

[Luiz Capitulino]

On Fri, 26 Oct 2012 03:43:10 +0800
Lei Li <lilei@linux.vnet.ibm.com> wrote:

> On 10/24/2012 08:55 PM, Luiz Capitulino wrote:
> > On Wed, 24 Oct 2012 15:17:21 +0800
> > Lei Li <lilei@linux.vnet.ibm.com> wrote:
> >
> >> On 10/23/2012 02:59 AM, Luiz Capitulino wrote:
> >>> On Mon, 22 Oct 2012 00:48:01 +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           |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>    hmp.h           |    1 +
> >>>>    monitor.c       |   15 +++++++++++++++
> >>>>    monitor.h       |    3 +++
> >>>>    5 files changed, 95 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >>>> index 5f91428..f862a53 100644
> >>>> --- a/hmp-commands.hx
> >>>> +++ b/hmp-commands.hx
> >>>> @@ -868,7 +868,30 @@ char device and return @var{size} of the data.
> >>>>    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       = "console",
> >>>> +        .args_type  = "chardev:s",
> >>>> +        .params     = "chardev",
> >>>> +        .help       = "Connect to the serial console from within the"
> >>>> +                      "monitor, allow to write data to memchardev"
> >>>> +                      "'chardev'. Exit from the console and return back"
> >>>> +                      "to monitor by typing 'ctrl-]'",
> >>>> +        .mhandler.cmd = hmp_console,
> >>>> +    },
> >>>>    
> >>>> +STEXI
> >>>> +@item console @var{device}
> >>>> +@findex console
> >>>> +
> >>>> +Connect to the serial console from within the monitor, allow to write data
> >>>> +to memchardev @var{chardev}. Exit from the console and return back to
> >>>> +monitor by typing 'ctrl-]'.
> >>>> +@example
> >>>> +(qemu) console foo
> >>>> +foo: data string...
> >>>> +@end example
> >>>>    ETEXI
> >>>>    
> >>>>        {
> >>>> diff --git a/hmp.c b/hmp.c
> >>>> index fa858c4..bc245f4 100644
> >>>> --- a/hmp.c
> >>>> +++ b/hmp.c
> >>>> @@ -1245,3 +1245,56 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
> >>>>        qmp_screendump(filename, &err);
> >>>>        hmp_handle_error(mon, &err);
> >>>>    }
> >>>> +
> >>>> +enum escape_char
> >>>> +{
> >>>> +    ESCAPE_CHAR_CTRL_GS = 0x1d  /* ctrl-] 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;
> >>>> +    enum escape_char console_escape = ESCAPE_CHAR_CTRL_GS;
> >>>> +
> >>>> +    Error *err = NULL;
> >>>> +
> >>>> +    if (*data == console_escape) {
> >>>> +        monitor_resume(mon);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    qmp_memchar_write(chr->label, size, data, 0, format,
> >>>> +                      0, control, &err);
> >>>> +    if (err) {
> >>>> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> >>>> +        error_free(err);
> >>>> +        return;
> >>>> +    }
> >>> Shouldn't you also read from the device?
> >> The use-case for this console command is just allow the user to
> >> write data to each memchar device as in a signal terminal. Then
> >> type escape sequences to take you back to the monitor. So I don't
> >> think it is also need to read from the device in this command.
> >>
> >> BTW, we can read from the device by hmp_memchar_read. :)
> > And how is the console command better than the hmp_memchar_write one?
> >
> > Could you please give examples?
> 
> Sure. The "console" command behaves like:
> 
> (qemu) console foo
> foo: hello world
> (qemu)
> 
> you can input data, then put enter or ctrl-] to return back to
> the monitor. The data input from console would be written to the
> CirMemCharDriver backend, which provide a more friendly and nice
> HMP command as Anthony suggested.

Maybe I'm missing something, but I don't understand how this is
better than the hmp_memchar_write command.

> >>>> +
> >>>> +    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);
> >>>> +        goto out;
> >>>> +    }
> >>> No need to do this here as the QMP command will do it too.
> >>>
> >> I think we should do this check here, otherwise would
> >> cause core dump when 'console' to a chardev that does not
> >> exist. Wepass parameterchr->label by qmp_memchar_write()
> >> in the handler hmp_read_console.
> > Do you need the chr object? Why don't you just pass 'device' to
> > monitor_read_console()?
> 
> Yes, we need 'device' and 'chr' both here. 'device' pass to monitor_read_console
> for the friendly prompt. 'chr' pass to handler hmp_read_console, the reason did not
> pass 'device' directly to it is the limitation of argument for the handler
> hmp_read_console.

What limitation? It takes 'data' and 'opaque' (which is a bit weird).

> Also it's better to give the prompt before the user enter the input
> console if the device not found, the use-case as I post above.

Why?

> 
> >>>> +
> >>>> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
> >>>> +        monitor_printf(mon, "Connect to console %s failed\n", device);
> >>>> +    }
> >>>> +
> >>>> +out:
> >>>> +    hmp_handle_error(mon, &err);
> >>>> +}
> >>>> diff --git a/hmp.h b/hmp.h
> >>>> index a5a0cfe..5b54a79 100644
> >>>> --- a/hmp.h
> >>>> +++ b/hmp.h
> >>>> @@ -77,5 +77,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_closefd(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_send_key(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> >>>> +void hmp_console(Monitor *mon, const QDict *qdict);
> >>>>    
> >>>>    #endif
> >>>> diff --git a/monitor.c b/monitor.c
> >>>> index 131b325..453c084 100644
> >>>> --- a/monitor.c
> >>>> +++ b/monitor.c
> >>>> @@ -256,6 +256,21 @@ 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;
> >>>> +    }
> >>>> +
> >>>> +    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 b6e7d95..735bd1b 100644
> >>>> --- a/monitor.h
> >>>> +++ b/monitor.h
> >>>> @@ -86,6 +86,9 @@ 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);
> >>>>    
> >>>>    int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
> >>
> >
> 
>