[Qemu-devel] [PATCH v2 0/3] qemu-char: ringbuf fixes

[Qemu-devel] [PATCH v2 0/3] qemu-char: ringbuf fixes

[Markus Armbruster]

Fix code to match documentation.  The patches take pains to preserve
command-line, HMP and QMP bug-compatibility.  I'm not sure that's
actually needed, because:

* All: the documented way to use the driver never worked in any
  released version, and the way that worked was undocumented.

* QMP: I doubt anyone used chardev-add / chardev-remove in anger,
  because serious use surely would've run into the bug fixed in commit
  commit 2ea3e2c "qemu-char: Fix ID reuse after chardev-remove for
  qapi-based init".

I include the compatibility gunk anyway, so we can have a more
informed discussion.

You might want to apply this after my "Our QAPI parser is a hack,
replace it" series, so the indented comment in PATCH 3/3 works by
design rather than accident.

v2: Trivially rebased

Markus Armbruster (3):
  Revert "chardev: Make the name of memory device consistent"
  qemu-char: Register ring buffer driver with correct name "ringbuf"
  qapi: Rename ChardevBackend member "memory" to "ringbuf"

 qapi-schema.json | 12 +++++++-----
 qemu-char.c      | 26 +++++++++++++++-----------
 qemu-options.hx  |  6 +++---
 3 files changed, 25 insertions(+), 19 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH v2 1/3] Revert "chardev: Make the name of memory device consistent"

[Markus Armbruster]

This reverts commit 6a85e60cb994bd95d1537aafbff65816f3de4637.

Commit 51767e7 "qemu-char: Add new char backend CirMemCharDriver"
introduced a memory ring buffer character device driver named
"memory".  Commit 3949e59 "qemu-char: Saner naming of memchar stuff &
doc fixes" changed the driver name to "ringbuf", along with a whole
bunch of other names, with the following rationale:

    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.

This is what we released in 1.4.0.

Unfortunately, the rename missed a critical instance of "memory": the
actual driver name.  Thus, the new device could be used only by an
entirely undocumented name.  The documented name did not work.
Bummer.

Commit 6a85e60 fixes this by changing the documentation to match the
code.  It also changes some, but not all related occurences of
"ringbuf" to "memory".  Left alone are identifiers in C code, HMP and
QMP commands.  The latter are external interface, so they can't be
changed.

The result is an inconsistent mess.  Moreover, "memory" is a rotten
name.  The device's distinctive property is that it's a ring buffer,
not that's in memory.  User's don't care whether it's in RAM, flash,
or carved into chocolate tablets by Oompa Loompas.

Revert the commit.  Next commit will fix just the bug.

Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi-schema.json |  6 +++---
 qemu-char.c      | 16 ++++++++--------
 qemu-options.hx  |  6 +++---
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index f82d829..a56de74 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3362,7 +3362,7 @@
                                  '*rows'   : 'int' } }
 
 ##
-# @ChardevMemory:
+# @ChardevRingbuf:
 #
 # Configuration info for memory chardevs
 #
@@ -3370,7 +3370,7 @@
 #
 # Since: 1.5
 ##
-{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
+{ 'type': 'ChardevRingbuf', 'data': { '*size'  : 'int' } }
 
 ##
 # @ChardevBackend:
@@ -3397,7 +3397,7 @@
                                        'spicevmc' : 'ChardevSpiceChannel',
                                        'spiceport' : 'ChardevSpicePort',
                                        'vc'     : 'ChardevVC',
-                                       'memory' : 'ChardevMemory' } }
+                                       'memory' : 'ChardevRingbuf' } }
 
 ##
 # @ChardevReturn:
diff --git a/qemu-char.c b/qemu-char.c
index c86ce4b..bd1aefb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2783,8 +2783,8 @@ static void ringbuf_chr_close(struct CharDriverState *chr)
     chr->opaque = NULL;
 }
 
-static CharDriverState *qemu_chr_open_memory(ChardevMemory *opts,
-                                             Error **errp)
+static CharDriverState *qemu_chr_open_ringbuf(ChardevRingbuf *opts,
+                                              Error **errp)
 {
     CharDriverState *chr;
     RingBufCharDriver *d;
@@ -2796,7 +2796,7 @@ static CharDriverState *qemu_chr_open_memory(ChardevMemory *opts,
 
     /* The size must be power of 2 */
     if (d->size & (d->size - 1)) {
-        error_setg(errp, "size of memory chardev must be power of two");
+        error_setg(errp, "size of ringbuf chardev must be power of two");
         goto fail;
     }
 
@@ -3105,12 +3105,12 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
     backend->pipe->device = g_strdup(device);
 }
 
-static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
-                                  Error **errp)
+static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
+                                   Error **errp)
 {
     int val;
 
-    backend->memory = g_new0(ChardevMemory, 1);
+    backend->memory = g_new0(ChardevRingbuf, 1);
 
     val = qemu_opt_get_size(opts, "size", 0);
     if (val != 0) {
@@ -3724,7 +3724,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         chr = vc_init(backend->vc);
         break;
     case CHARDEV_BACKEND_KIND_MEMORY:
-        chr = qemu_chr_open_memory(backend->memory, errp);
+        chr = qemu_chr_open_ringbuf(backend->memory, errp);
         break;
     default:
         error_setg(errp, "unknown chardev backend (%d)", backend->kind);
@@ -3775,7 +3775,7 @@ static void register_types(void)
     register_char_driver("socket", qemu_chr_open_socket);
     register_char_driver("udp", qemu_chr_open_udp);
     register_char_driver_qapi("memory", CHARDEV_BACKEND_KIND_MEMORY,
-                              qemu_chr_parse_memory);
+                              qemu_chr_parse_ringbuf);
     register_char_driver_qapi("file", CHARDEV_BACKEND_KIND_FILE,
                               qemu_chr_parse_file_out);
     register_char_driver_qapi("stdio", CHARDEV_BACKEND_KIND_STDIO,
diff --git a/qemu-options.hx b/qemu-options.hx
index 2dbfd42..7e30a81 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1782,7 +1782,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[,size=size]\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
@@ -1820,7 +1820,7 @@ Backend is one of:
 @option{udp},
 @option{msmouse},
 @option{vc},
-@option{memory},
+@option{ringbuf},
 @option{file},
 @option{pipe},
 @option{console},
@@ -1929,7 +1929,7 @@ 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 memory ,id=@var{id} [,size=@var{size}]
+@item -chardev ringbuf ,id=@var{id} [,size=@var{size}]
 
 Create a ring buffer with fixed size @option{size}.
 @var{size} must be a power of two, and defaults to @code{64K}).
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 2/3] qemu-char: Register ring buffer driver with correct name "ringbuf"

[Markus Armbruster]

The driver is new in 1.4, with the documented name "ringbuf".
However, it's actual name is the completely undocumented "memory".
Screwed up in commit 3949e59.  Fix code to match documentation.

Keep the undocumented name working as an alias for compatibility.

Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-char.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index bd1aefb..742b73b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3774,7 +3774,7 @@ static void register_types(void)
     register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL);
     register_char_driver("socket", qemu_chr_open_socket);
     register_char_driver("udp", qemu_chr_open_udp);
-    register_char_driver_qapi("memory", CHARDEV_BACKEND_KIND_MEMORY,
+    register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_MEMORY,
                               qemu_chr_parse_ringbuf);
     register_char_driver_qapi("file", CHARDEV_BACKEND_KIND_FILE,
                               qemu_chr_parse_file_out);
@@ -3794,6 +3794,9 @@ static void register_types(void)
                               qemu_chr_parse_pipe);
     register_char_driver_qapi("mux", CHARDEV_BACKEND_KIND_MUX,
                               qemu_chr_parse_mux);
+    /* Bug-compatibility: */
+    register_char_driver_qapi("memory", CHARDEV_BACKEND_KIND_MEMORY,
+                              qemu_chr_parse_ringbuf);
 }
 
 type_init(register_types);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 3/3] qapi: Rename ChardevBackend member "memory" to "ringbuf"

[Markus Armbruster]

Commit 1da48c6 called the new member "memory" after commit 3949e59
standardized "ringbuf".  Rename for consistency.

However, member name "memory" is visible in QMP since 1.5.  It's
undocumented just like the driver name.  Keep it working anyway.

Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi-schema.json |  6 ++++--
 qemu-char.c      | 11 ++++++-----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index a56de74..a51f7d2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3364,9 +3364,9 @@
 ##
 # @ChardevRingbuf:
 #
-# Configuration info for memory chardevs
+# Configuration info for ring buffer chardevs.
 #
-# @size: #optional Ringbuffer size, must be power of two, default is 65536
+# @size: #optional ring buffer size, must be power of two, default is 65536
 #
 # Since: 1.5
 ##
@@ -3397,6 +3397,8 @@
                                        'spicevmc' : 'ChardevSpiceChannel',
                                        'spiceport' : 'ChardevSpicePort',
                                        'vc'     : 'ChardevVC',
+                                       'ringbuf': 'ChardevRingbuf',
+                                       # next one is just for compatibility
                                        'memory' : 'ChardevRingbuf' } }
 
 ##
diff --git a/qemu-char.c b/qemu-char.c
index 742b73b..3f606c9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3110,12 +3110,12 @@ static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
 {
     int val;
 
-    backend->memory = g_new0(ChardevRingbuf, 1);
+    backend->ringbuf = g_new0(ChardevRingbuf, 1);
 
     val = qemu_opt_get_size(opts, "size", 0);
     if (val != 0) {
-        backend->memory->has_size = true;
-        backend->memory->size = val;
+        backend->ringbuf->has_size = true;
+        backend->ringbuf->size = val;
     }
 }
 
@@ -3723,8 +3723,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     case CHARDEV_BACKEND_KIND_VC:
         chr = vc_init(backend->vc);
         break;
+    case CHARDEV_BACKEND_KIND_RINGBUF:
     case CHARDEV_BACKEND_KIND_MEMORY:
-        chr = qemu_chr_open_ringbuf(backend->memory, errp);
+        chr = qemu_chr_open_ringbuf(backend->ringbuf, errp);
         break;
     default:
         error_setg(errp, "unknown chardev backend (%d)", backend->kind);
@@ -3774,7 +3775,7 @@ static void register_types(void)
     register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL);
     register_char_driver("socket", qemu_chr_open_socket);
     register_char_driver("udp", qemu_chr_open_udp);
-    register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_MEMORY,
+    register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF,
                               qemu_chr_parse_ringbuf);
     register_char_driver_qapi("file", CHARDEV_BACKEND_KIND_FILE,
                               qemu_chr_parse_file_out);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v2 0/3] qemu-char: ringbuf fixes

[Eric Blake]

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

On 07/26/2013 08:44 AM, Markus Armbruster wrote:
> Fix code to match documentation.  The patches take pains to preserve
> command-line, HMP and QMP bug-compatibility.  I'm not sure that's
> actually needed, because:
> 
> * All: the documented way to use the driver never worked in any
>   released version, and the way that worked was undocumented.
> 
> * QMP: I doubt anyone used chardev-add / chardev-remove in anger,
>   because serious use surely would've run into the bug fixed in commit
>   commit 2ea3e2c "qemu-char: Fix ID reuse after chardev-remove for
>   qapi-based init".
> 
> I include the compatibility gunk anyway, so we can have a more
> informed discussion.

Libvirt 1.1.1 has added patches that try to use chardev-add; but so far,
it only uses 'null', 'pty', 'file', 'parallel', 'serial', and 'socket'
backends.  But if libvirt adds support for a ringbuf device, it would be
nice to support it on as many qemu versions at once as possible, rather
than having to determine whether qemu is new enough to have fixed
semantics vs. using the undocumented backend of the older semantics.  So
I appreciate the attempt at back-compat.

> 
> You might want to apply this after my "Our QAPI parser is a hack,																																																	
> replace it" series, so the indented comment in PATCH 3/3 works by
> design rather than accident.

Heh :)

Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 0/3] qemu-char: ringbuf fixes

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori