[Qemu-devel] [PATCH for-1.4 v2 11/13] qemu-char: Saner naming of memchar stuff & doc fixes

[Markus Armbruster]

New device, has never been released, so we can still improve things
without worrying about compatibility.

Naming is a mess.  The code calls the device driver CirMemCharDriver,
the public API calls it "memory", "memchardev", or "memchar", and the
special commands are named like "memchar-FOO".  "memory" is a
particularly unfortunate choice, because there's another character
device driver called MemoryDriver.  Moreover, the device's distinctive
property is that it's a ring buffer, not that's in memory.  Therefore:

* Rename CirMemCharDriver to RingBufCharDriver, and call the thing a
  "ringbuf" in the API.

* Rename QMP and HMP commands from memchar-FOO to ringbuf-FOO.

* Rename device parameter from maxcapacity to size (simple words are
  good for you).

* Clearly mark the parameter as optional in documentation.

* Fix error reporting so that chardev-add reports to current monitor,
  not stderr.

* Replace cirmem in C identifiers by ringbuf.

* Rework documentation.  Document the impact of our crappy UTF-8
  handling on reading.

* QMP examples that even work.

I could split this up into multiple commits, but they'd change the
same documentation lines multiple times.  Not worth it.

Signed-off-by: Markus Armbruster <address@hidden>
---
 hmp-commands.hx  | 37 +++++++++++++++-----------------
 hmp.c            |  8 +++----
 hmp.h            |  4 ++--
 qapi-schema.json | 47 ++++++++++++++++++++++++----------------
 qemu-char.c      | 65 +++++++++++++++++++++++++++++++-------------------------
 qemu-options.hx  | 13 +++++-------
 qmp-commands.hx  | 55 ++++++++++++++++++++++++-----------------------
 7 files changed, 122 insertions(+), 107 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bdd48f3..66ec716 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -841,40 +841,37 @@ Inject an NMI on the given CPU (x86 only).
 ETEXI
 
     {
-        .name       = "memchar_write",
+        .name       = "ringbuf_write",
         .args_type  = "device:s,data:s",
         .params     = "device data",
-        .help       = "Provide writing interface for CirMemCharDriver. Write"
-                      "'data' to it.",
-        .mhandler.cmd = hmp_memchar_write,
+        .help       = "Write to a ring buffer character device",
+        .mhandler.cmd = hmp_ringbuf_write,
     },
 
 STEXI
address@hidden memchar_write @var{device} @var{data}
address@hidden memchar_write
-Provide writing interface for CirMemCharDriver. Write @var{data}
-to char device 'memory'.
address@hidden ringbuf_write @var{device} @var{data}
address@hidden ringbuf_write
+Write @var{data} to ring buffer character device @var{device}.
address@hidden must be a UTF-8 string.
 
 ETEXI
 
     {
-        .name       = "memchar_read",
+        .name       = "ringbuf_read",
         .args_type  = "device:s,size:i",
         .params     = "device size",
-        .help       = "Provide read interface for CirMemCharDriver. Read from"
-                      "it and return the data with size.",
-        .mhandler.cmd = hmp_memchar_read,
+        .help       = "Read from a ring buffer character device",
+        .mhandler.cmd = hmp_ringbuf_read,
     },
 
 STEXI
address@hidden memchar_read @var{device}
address@hidden memchar_read
-Provide read interface for CirMemCharDriver. Read from char device
-'memory' and return the data.
-
address@hidden is the size of data want to read from. 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.
address@hidden ringbuf_read @var{device}
address@hidden ringbuf_read
+Read and print up to @var{size} bytes from ring buffer character
+device @var{device}.
+Bug: can screw up when the buffer contains invalid UTF-8 sequences,
+NUL characters, after the ring buffer lost data, and when reading
+stops because the size limit is reached.
 
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index f6bb767..cbd5727 100644
--- a/hmp.c
+++ b/hmp.c
@@ -662,25 +662,25 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
-void hmp_memchar_write(Monitor *mon, const QDict *qdict)
+void hmp_ringbuf_write(Monitor *mon, const QDict *qdict)
 {
     const char *chardev = qdict_get_str(qdict, "device");
     const char *data = qdict_get_str(qdict, "data");
     Error *errp = NULL;
 
-    qmp_memchar_write(chardev, data, false, 0, &errp);
+    qmp_ringbuf_write(chardev, data, false, 0, &errp);
 
     hmp_handle_error(mon, &errp);
 }
 
-void hmp_memchar_read(Monitor *mon, const QDict *qdict)
+void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)
 {
     uint32_t size = qdict_get_int(qdict, "size");
     const char *chardev = qdict_get_str(qdict, "device");
     char *data;
     Error *errp = NULL;
 
-    data = qmp_memchar_read(chardev, size, false, 0, &errp);
+    data = qmp_ringbuf_read(chardev, size, false, 0, &errp);
     if (errp) {
         monitor_printf(mon, "%s\n", error_get_pretty(errp));
         error_free(errp);
diff --git a/hmp.h b/hmp.h
index 076d8cf..30b3c20 100644
--- a/hmp.h
+++ b/hmp.h
@@ -43,8 +43,8 @@ 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_memchar_read(Monitor *mon, const QDict *qdict);
+void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
+void hmp_ringbuf_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 6f63791..736f881 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -329,9 +329,9 @@
 #
 # An enumeration of data format.
 #
-# @utf8: The data format is 'utf8'.
+# @utf8: Data is a UTF-8 string (RFC 3629)
 #
-# @base64: The data format is 'base64'.
+# @base64: Data is Base64 encoded binary (RFC 3548)
 #
 # Since: 1.4
 ##
@@ -339,44 +339,55 @@
   'data': [ 'utf8', 'base64' ] }
 
 ##
-# @memchar-write:
+# @ringbuf-write:
 #
-# Provide writing interface for memchardev. Write data to char
-# device 'memory'.
+# Write to a ring buffer character device.
 #
-# @device: the name of the memory char device.
+# @device: the ring buffer character device name
 #
-# @data: the source data write to memchar.
+# @data: data to write
 #
-# @format: #optional the format of the data write to chardev 'memory',
-#          by default is 'utf8'.
+# @format: #optional data encoding (default 'utf8').
+#          - base64: data must be base64 encoded text.  Its binary
+#            decoding gets written.
+#            Bug: invalid base64 is currently not rejected.
+#            Whitespace *is* invalid.
+#          - utf8: data's UTF-8 encoding is written
+#          - data itself is always Unicode regardless of format, like
+#            any other string.
 #
 # Returns: Nothing on success
 #
 # Since: 1.4
 ##
-{ 'command': 'memchar-write',
+{ 'command': 'ringbuf-write',
   'data': {'device': 'str', 'data': 'str',
            '*format': 'DataFormat'} }
 
 ##
-# @memchar-read:
+# @ringbuf-read:
 #
-# Provide read interface for memchardev. Read from the char
-# device 'memory' and return the data.
+# Read from a ring buffer character device.
 #
-# @device: the name of the memory char device.
+# @device: the ring buffer character device name
 #
-# @size: the size to read in bytes.
+# @size: how many bytes to read at most
 #
-# @format: #optional the format of the data want to read from
-#          memchardev, by default is 'utf8'.
+# @format: #optional data encoding (default 'utf8').
+#          - base64: the data read is returned in base64 encoding.
+#          - utf8: the data read is interpreted as UTF-8.
+#            Bug: can screw up when the buffer contains invalid UTF-8
+#            sequences, NUL characters, after the ring buffer lost
+#            data, and when reading stops because the size limit is
+#            reached.
+#          - The return value is always Unicode regardless of format,
+#            like any other string.
 #
 # Returns: data read from the device
 #
 # Since: 1.4
 ##
-{ 'command': 'memchar-read',
+{ 'command': 'ringbuf-read',
   'data': {'device': 'str', 'size': 'int', '*format': 'DataFormat'},
   'returns': 'str' }
 
diff --git a/qemu-char.c b/qemu-char.c
index 831d564..8a35403 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2645,25 +2645,25 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr)
 }
 
 /*********************************************************/
-/* CircularMemory chardev */
+/* Ring buffer chardev */
 
 typedef struct {
     size_t size;
     size_t prod;
     size_t cons;
     uint8_t *cbuf;
-} CirMemCharDriver;
+} RingBufCharDriver;
 
-static size_t cirmem_count(const CharDriverState *chr)
+static size_t ringbuf_count(const CharDriverState *chr)
 {
-    const CirMemCharDriver *d = chr->opaque;
+    const RingBufCharDriver *d = chr->opaque;
 
     return d->prod - d->cons;
 }
 
-static int cirmem_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+static int ringbuf_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
-    CirMemCharDriver *d = chr->opaque;
+    RingBufCharDriver *d = chr->opaque;
     int i;
 
     if (!buf || (len < 0)) {
@@ -2680,9 +2680,9 @@ static int cirmem_chr_write(CharDriverState *chr, const 
uint8_t *buf, int len)
     return 0;
 }
 
-static int cirmem_chr_read(CharDriverState *chr, uint8_t *buf, int len)
+static int ringbuf_chr_read(CharDriverState *chr, uint8_t *buf, int len)
 {
-    CirMemCharDriver *d = chr->opaque;
+    RingBufCharDriver *d = chr->opaque;
     int i;
 
     for (i = 0; i < len && d->cons != d->prod; i++) {
@@ -2692,31 +2692,31 @@ static int cirmem_chr_read(CharDriverState *chr, 
uint8_t *buf, int len)
     return i;
 }
 
-static void cirmem_chr_close(struct CharDriverState *chr)
+static void ringbuf_chr_close(struct CharDriverState *chr)
 {
-    CirMemCharDriver *d = chr->opaque;
+    RingBufCharDriver *d = chr->opaque;
 
     g_free(d->cbuf);
     g_free(d);
     chr->opaque = NULL;
 }
 
-static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts *opts)
+static CharDriverState *qemu_chr_open_ringbuf(QemuOpts *opts)
 {
     CharDriverState *chr;
-    CirMemCharDriver *d;
+    RingBufCharDriver *d;
 
     chr = g_malloc0(sizeof(CharDriverState));
     d = g_malloc(sizeof(*d));
 
-    d->size = qemu_opt_get_number(opts, "maxcapacity", 0);
+    d->size = qemu_opt_get_number(opts, "size", 0);
     if (d->size == 0) {
         d->size = CBUFF_SIZE;
     }
 
     /* The size must be power of 2 */
     if (d->size & (d->size - 1)) {
-        fprintf(stderr, "chardev: size of memory device must be power of 2\n");
+        error_report("size of ringbuf device must be power of two");
         goto fail;
     }
 
@@ -2725,8 +2725,8 @@ static CharDriverState *qemu_chr_open_cirmemchr(QemuOpts 
*opts)
     d->cbuf = g_malloc0(d->size);
 
     chr->opaque = d;
-    chr->chr_write = cirmem_chr_write;
-    chr->chr_close = cirmem_chr_close;
+    chr->chr_write = ringbuf_chr_write;
+    chr->chr_close = ringbuf_chr_close;
 
     return chr;
 
@@ -2736,12 +2736,12 @@ fail:
     return NULL;
 }
 
-static bool chr_is_cirmem(const CharDriverState *chr)
+static bool chr_is_ringbuf(const CharDriverState *chr)
 {
-    return chr->chr_write == cirmem_chr_write;
+    return chr->chr_write == ringbuf_chr_write;
 }
 
-void qmp_memchar_write(const char *device, const char *data,
+void qmp_ringbuf_write(const char *device, const char *data,
                        bool has_format, enum DataFormat format,
                        Error **errp)
 {
@@ -2756,8 +2756,8 @@ void qmp_memchar_write(const char *device, const char 
*data,
         return;
     }
 
-    if (!chr_is_cirmem(chr)) {
-        error_setg(errp,"%s is not memory char device", device);
+    if (!chr_is_ringbuf(chr)) {
+        error_setg(errp,"%s is not a ringbuf device", device);
         return;
     }
 
@@ -2768,7 +2768,7 @@ void qmp_memchar_write(const char *device, const char 
*data,
         write_count = strlen(data);
     }
 
-    ret = cirmem_chr_write(chr, write_data, write_count);
+    ret = ringbuf_chr_write(chr, write_data, write_count);
 
     if (write_data != (uint8_t *)data) {
         g_free((void *)write_data);
@@ -2780,7 +2780,7 @@ void qmp_memchar_write(const char *device, const char 
*data,
     }
 }
 
-char *qmp_memchar_read(const char *device, int64_t size,
+char *qmp_ringbuf_read(const char *device, int64_t size,
                        bool has_format, enum DataFormat format,
                        Error **errp)
 {
@@ -2795,8 +2795,8 @@ char *qmp_memchar_read(const char *device, int64_t size,
         return NULL;
     }
 
-    if (!chr_is_cirmem(chr)) {
-        error_setg(errp,"%s is not memory char device", device);
+    if (!chr_is_ringbuf(chr)) {
+        error_setg(errp,"%s is not a ringbuf device", device);
         return NULL;
     }
 
@@ -2805,16 +2805,23 @@ char *qmp_memchar_read(const char *device, int64_t size,
         return NULL;
     }
 
-    count = cirmem_count(chr);
+    count = ringbuf_count(chr);
     size = size > count ? count : size;
     read_data = g_malloc(size + 1);
 
-    cirmem_chr_read(chr, read_data, size);
+    ringbuf_chr_read(chr, read_data, size);
 
     if (has_format && (format == DATA_FORMAT_BASE64)) {
         data = g_base64_encode(read_data, size);
         g_free(read_data);
     } else {
+        /*
+         * FIXME should read only complete, valid UTF-8 characters up
+         * to @size bytes.  Invalid sequences should be replaced by a
+         * suitable replacement character.  Except when (and only
+         * when) ring buffer lost characters since last read, initial
+         * continuation characters should be dropped.
+         */
         read_data[size] = 0;
         data = (char *)read_data;
     }
@@ -2975,7 +2982,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 = "memory",    .open = qemu_chr_open_cirmemchr },
+    { .name = "memory",    .open = qemu_chr_open_ringbuf },
 #ifdef _WIN32
     { .name = "file",      .open = qemu_chr_open_win_file_out },
     { .name = "pipe",      .open = qemu_chr_open_win_pipe },
@@ -3236,7 +3243,7 @@ QemuOptsList qemu_chardev_opts = {
             .name = "debug",
             .type = QEMU_OPT_NUMBER,
         },{
-            .name = "maxcapacity",
+            .name = "size",
             .type = QEMU_OPT_NUMBER,
         },
         { /* end of list */ }
diff --git a/qemu-options.hx b/qemu-options.hx
index 2d44137..046bdc0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1736,7 +1736,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 memory,id=id,maxcapacity=maxcapacity\n"
+    "-chardev ringbuf,id=id[,size=size]\n"
     "-chardev file,id=id,path=path[,mux=on|off]\n"
     "-chardev pipe,id=id,path=path[,mux=on|off]\n"
 #ifdef _WIN32
@@ -1778,7 +1778,7 @@ Backend is one of:
 @option{udp},
 @option{msmouse},
 @option{vc},
address@hidden,
address@hidden,
 @option{file},
 @option{pipe},
 @option{console},
@@ -1887,13 +1887,10 @@ the console, in pixels.
 @option{cols} and @option{rows} specify that the console be sized to fit a text
 console with the given dimensions.
 
address@hidden -chardev memory ,address@hidden ,address@hidden
address@hidden -chardev ringbuf ,address@hidden [,address@hidden
 
-Create a circular buffer with fixed size indicated by optionally 
@option{maxcapacity}
-which will be default 64K if it is not given.
-
address@hidden specifies the max capacity of the size of circular buffer
-to create. Should be power of 2.
+Create a ring buffer with fixed size @option{size}.
address@hidden must be a power of two, and defaults to @code{64K}).
 
 @item -chardev file ,address@hidden ,address@hidden
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 51ce2e6..799adea 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -466,30 +466,30 @@ Note: inject-nmi fails when the guest doesn't support 
injecting.
 EQMP
 
     {
-        .name       = "memchar-write",
+        .name       = "ringbuf-write",
         .args_type  = "device:s,data:s,format:s?",
-        .mhandler.cmd_new = qmp_marshal_input_memchar_write,
+        .mhandler.cmd_new = qmp_marshal_input_ringbuf_write,
     },
 
 SQMP
-memchar-write
+ringbuf-write
 -------------
 
-Provide writing interface for CirMemCharDriver. Write data to memory
-char device.
+Write to a ring buffer character device.
 
 Arguments:
 
-- "device": the name of the char device, must be unique (json-string)
-- "data": the source data write to memory (json-string)
-- "format": the data format write to memory, default is
-            utf8. (json-string, optional)
-          - Possible values: "utf8", "base64"
+- "device": ring buffer character device name (json-string)
+- "data": data to write (json-string)
+- "format": data format (json-string, optional)
+          - Possible values: "utf8" (default), "base64"
+            Bug: invalid base64 is currently not rejected.
+            Whitespace *is* invalid.
 
 Example:
 
--> { "execute": "memchar-write",
-                "arguments": { "device": foo,
+-> { "execute": "ringbuf-write",
+                "arguments": { "device": "foo",
                                "data": "abcdefgh",
                                "format": "utf8" } }
 <- { "return": {} }
@@ -497,32 +497,35 @@ Example:
 EQMP
 
     {
-        .name       = "memchar-read",
+        .name       = "ringbuf-read",
         .args_type  = "device:s,size:i,format:s?",
-        .mhandler.cmd_new = qmp_marshal_input_memchar_read,
+        .mhandler.cmd_new = qmp_marshal_input_ringbuf_read,
     },
 
 SQMP
-memchar-read
+ringbuf-read
 -------------
 
-Provide read interface for CirMemCharDriver. Read from the char
-device memory and return the data with size.
+Read from a ring buffer character device.
 
 Arguments:
 
-- "device": 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"
+- "device": ring buffer character device name (json-string)
+- "size": how many bytes to read at most (json-int)
+          - Number of data bytes, not number of characters in encoded data
+- "format": data format (json-string, optional)
+          - Possible values: "utf8" (default), "base64"
+          - Naturally, format "utf8" works only when the ring buffer
+            contains valid UTF-8 text.  Invalid UTF-8 sequences get
+            replaced.  Bug: replacement doesn't work.  Bug: can screw
+            up on encountering NUL characters, after the ring buffer
+            lost data, and when reading stops because the size limit
+            is reached.
 
 Example:
 
--> { "execute": "memchar-read",
-                "arguments": { "device": foo,
+-> { "execute": "ringbuf-read",
+                "arguments": { "device": "foo",
                                "size": 1000,
                                "format": "utf8" } }
 <- {"return": "abcdefgh"}
-- 
1.7.11.7