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

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

[Markus Armbruster]

The first patch is a straightforward bug fix.

The rest 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 "qemu-char: Fix ID
  reuse after chardev-remove for qapi-based init" bug.

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

Markus Armbruster (4):
  qemu-char: Fix ringbuf option size
  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      | 28 ++++++++++++++++------------
 qemu-options.hx  |  6 +++---
 3 files changed, 26 insertions(+), 20 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/4] qemu-char: Fix ringbuf option size

[Markus Armbruster]

Any attempt to use it trips an "opt->desc->type == QEMU_OPT_NUMBER"
assertion.  Broken in commit 1da48c65.

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

diff --git a/qemu-char.c b/qemu-char.c
index 63972ae..63a9221 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3108,7 +3108,7 @@ static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
 
     backend->memory = g_new0(ChardevMemory, 1);
 
-    val = qemu_opt_get_number(opts, "size", 0);
+    val = qemu_opt_get_size(opts, "size", 0);
     if (val != 0) {
         backend->memory->has_size = true;
         backend->memory->size = val;
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/4] 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 6cc07c2..6445da6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3275,7 +3275,7 @@
                                  '*rows'   : 'int' } }
 
 ##
-# @ChardevMemory:
+# @ChardevRingbuf:
 #
 # Configuration info for memory chardevs
 #
@@ -3283,7 +3283,7 @@
 #
 # Since: 1.5
 ##
-{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
+{ 'type': 'ChardevRingbuf', 'data': { '*size'  : 'int' } }
 
 ##
 # @ChardevBackend:
@@ -3310,7 +3310,7 @@
                                        'spicevmc' : 'ChardevSpiceChannel',
                                        'spiceport' : 'ChardevSpicePort',
                                        'vc'     : 'ChardevVC',
-                                       'memory' : 'ChardevMemory' } }
+                                       'memory' : 'ChardevRingbuf' } }
 
 ##
 # @ChardevReturn:
diff --git a/qemu-char.c b/qemu-char.c
index 63a9221..a8fad65 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2786,8 +2786,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;
@@ -2799,7 +2799,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;
     }
 
@@ -3101,12 +3101,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) {
@@ -3705,7 +3705,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);
@@ -3756,7 +3756,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 ca6fdf6..3b71e39 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1781,7 +1781,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
@@ -1819,7 +1819,7 @@ Backend is one of:
 @option{udp},
 @option{msmouse},
 @option{vc},
-@option{memory},
+@option{ringbuf},
 @option{file},
 @option{pipe},
 @option{console},
@@ -1928,7 +1928,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 3/4] 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 a8fad65..968f572 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3755,7 +3755,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);
@@ -3773,6 +3773,9 @@ static void register_types(void)
     register_char_driver_qapi("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL);
     register_char_driver_qapi("pipe", CHARDEV_BACKEND_KIND_PIPE,
                               qemu_chr_parse_pipe);
+    /* 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 4/4] 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 6445da6..b3df8a5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3277,9 +3277,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
 ##
@@ -3310,6 +3310,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 968f572..5b1b3c4 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3106,12 +3106,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;
     }
 }
 
@@ -3704,8 +3704,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);
@@ -3755,7 +3756,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 1/4] qemu-char: Fix ringbuf option size

[Eric Blake]

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

On 06/27/2013 08:22 AM, Markus Armbruster wrote:
> Any attempt to use it trips an "opt->desc->type == QEMU_OPT_NUMBER"
> assertion.  Broken in commit 1da48c65.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 63972ae..63a9221 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3108,7 +3108,7 @@ static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
>  
>      backend->memory = g_new0(ChardevMemory, 1);
>  
> -    val = qemu_opt_get_number(opts, "size", 0);
> +    val = qemu_opt_get_size(opts, "size", 0);
>      if (val != 0) {
>          backend->memory->has_size = true;
>          backend->memory->size = val;
> 

-- 
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 2/4] Revert "chardev: Make the name of memory device consistent"

[Lei Li]

On 06/27/2013 10:22 PM, Markus Armbruster wrote:
> 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.

Hi Markus.

Actually I fixed this issue as the your coming patch set 3&4 (make them
consistent with 'ringbuf') in the beginning, but according to Paolo's
comments, since '-chardev memory' exists in 1.4 and cannot be renamed,
so made such change as Commit 6a85e60cb994bd95d1537aafbff65816f3de4637.

The original patch and discussion as link below:

http://patchwork.ozlabs.org/patch/244848/

>
> 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 6cc07c2..6445da6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3275,7 +3275,7 @@
>                                    '*rows'   : 'int' } }
>
>   ##
> -# @ChardevMemory:
> +# @ChardevRingbuf:
>   #
>   # Configuration info for memory chardevs
>   #
> @@ -3283,7 +3283,7 @@
>   #
>   # Since: 1.5
>   ##
> -{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
> +{ 'type': 'ChardevRingbuf', 'data': { '*size'  : 'int' } }
>
>   ##
>   # @ChardevBackend:
> @@ -3310,7 +3310,7 @@
>                                          'spicevmc' : 'ChardevSpiceChannel',
>                                          'spiceport' : 'ChardevSpicePort',
>                                          'vc'     : 'ChardevVC',
> -                                       'memory' : 'ChardevMemory' } }
> +                                       'memory' : 'ChardevRingbuf' } }
>
>   ##
>   # @ChardevReturn:
> diff --git a/qemu-char.c b/qemu-char.c
> index 63a9221..a8fad65 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2786,8 +2786,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;
> @@ -2799,7 +2799,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;
>       }
>
> @@ -3101,12 +3101,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) {
> @@ -3705,7 +3705,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);
> @@ -3756,7 +3756,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 ca6fdf6..3b71e39 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1781,7 +1781,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
> @@ -1819,7 +1819,7 @@ Backend is one of:
>   @option{udp},
>   @option{msmouse},
>   @option{vc},
> -@option{memory},
> +@option{ringbuf},
>   @option{file},
>   @option{pipe},
>   @option{console},
> @@ -1928,7 +1928,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}).


-- 
Lei

Re: [Qemu-devel] [PATCH 2/4] Revert "chardev: Make the name of memory device consistent"

[Markus Armbruster]

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

> On 06/27/2013 10:22 PM, Markus Armbruster wrote:
>> 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.
>
> Hi Markus.
>
> Actually I fixed this issue as the your coming patch set 3&4 (make them
> consistent with 'ringbuf') in the beginning, but according to Paolo's
> comments, since '-chardev memory' exists in 1.4 and cannot be renamed,
> so made such change as Commit 6a85e60cb994bd95d1537aafbff65816f3de4637.
>
> The original patch and discussion as link below:
>
> http://patchwork.ozlabs.org/patch/244848/

Yes, your patch fixes a real bug.  However, it changes more than that,
and thus results in more "memory" vs. "ringbuf" inconsistency than
necessary, as my commit message explains in the part quoted at the end
of this message.

Some inconsistency is unavoidable unless we forgo backward compatibility
to (undocumented!) -chardev memory.

We can't get rid of "memory", because -chardev memory "cannot be
renamed" (Paolo).  I'm not so sure, as it's undocumented, but let's
assume we can't.  This is one of only two occurences of "memory" before
your patch.  The other one is ChardevBackend member "memory", which is
similar: in 1.4, but undocumented.

We can't get rid of "ringbuf", because we can't rename QMP commands
ringbuf-read and ringbuf-write.

Your patch renames some instances of ringbuf to memory, and leaves
others alone.  That's why I called the result "an inconsistent mess" in
my commit message.

My patches rename nothing.  They *add* "ringbuf" aliases to the two
unfortunate instances of "memory".

Both yours and mine fix the "code doesn't match documentation" bug.

My patches maximize consistency within the compatibility constraints.

I reverted your patch simply because I want to start over for easier
review.

I should have raised my concerns while your patch was under review.  I
didn't, because I missed it entirely.  My fault, not yours; you cc'ed
me.

>> 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.
[...]

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

[Eric Blake]

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

On 06/27/2013 08:22 AM, Markus Armbruster wrote:
> 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 6445da6..b3df8a5 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3277,9 +3277,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
>  ##
> @@ -3310,6 +3310,8 @@
>                                         'spicevmc' : 'ChardevSpiceChannel',
>                                         'spiceport' : 'ChardevSpicePort',
>                                         'vc'     : 'ChardevVC',
> +                                       'ringbuf': 'ChardevRingbuf',
> +                                       # next one is just for compatibility
>                                         'memory' : 'ChardevRingbuf' } }

Does JSON allow comments in the middle of content?  Is this going to
screw up Amos' work on introspection?  You may need to instead have a
comment before the open '{' stating that 'memory' is an alias within the
union for back-compat reasons.

-- 
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 4/4] qapi: Rename ChardevBackend member "memory" to "ringbuf"

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/27/2013 08:22 AM, Markus Armbruster wrote:
>> 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 6445da6..b3df8a5 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3277,9 +3277,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
>>  ##
>> @@ -3310,6 +3310,8 @@
>>                                         'spicevmc' : 'ChardevSpiceChannel',
>>                                         'spiceport' : 'ChardevSpicePort',
>>                                         'vc'     : 'ChardevVC',
>> +                                       'ringbuf': 'ChardevRingbuf',
>> +                                       # next one is just for compatibility
>>                                         'memory' : 'ChardevRingbuf' } }
>
> Does JSON allow comments in the middle of content?  Is this going to
> screw up Amos' work on introspection?  You may need to instead have a
> comment before the open '{' stating that 'memory' is an alias within the
> union for back-compat reasons.

RFC 4627 doesn't do comments at all.

This file is parsed by scripts/qapi.py, which as far as I can tell
ignores lines starting with '#' anywhere in the input.

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

[Amos Kong]

On Fri, Jun 28, 2013 at 07:19:39PM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 06/27/2013 08:22 AM, Markus Armbruster wrote:
> >> 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 6445da6..b3df8a5 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -3277,9 +3277,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
> >>  ##
> >> @@ -3310,6 +3310,8 @@
> >>                                         'spicevmc' : 'ChardevSpiceChannel',
> >>                                         'spiceport' : 'ChardevSpicePort',
> >>                                         'vc'     : 'ChardevVC',
> >> +                                       'ringbuf': 'ChardevRingbuf',
> >> +                                       # next one is just for compatibility
> >>                                         'memory' : 'ChardevRingbuf' } }
> >
> > Does JSON allow comments in the middle of content?  Is this going to
> > screw up Amos' work on introspection?  You may need to instead have a
> > comment before the open '{' stating that 'memory' is an alias within the
> > union for back-compat reasons.

I didn't parse the json file by myself. I just used the parsed
dictionary. So it only needs to make qapi.py happy.

> RFC 4627 doesn't do comments at all.
> 
> This file is parsed by scripts/qapi.py, which as far as I can tell
> ignores lines starting with '#' anywhere in the input.

Not anywhere, only start with '#'

| def parse_schema(fp):
|     exprs = []
|     raw_exprs = []
|     expr = ''
|     expr_eval = None
| 
|     for line in fp:
|         if line.startswith('#') or line == '\n':

      # ignores lines starting with '#' anywhere
          if line.strip().startswith('#')   

|             continue
| 


So we should not add this kind of comment for back-compat.

-- 
			Amos.

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> On Fri, Jun 28, 2013 at 07:19:39PM +0200, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 06/27/2013 08:22 AM, Markus Armbruster wrote:
>> >> 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 6445da6..b3df8a5 100644
>> >> --- a/qapi-schema.json
>> >> +++ b/qapi-schema.json
>> >> @@ -3277,9 +3277,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
>> >>  ##
>> >> @@ -3310,6 +3310,8 @@
>> >>                                         'spicevmc' : 'ChardevSpiceChannel',
>> >>                                         'spiceport' : 'ChardevSpicePort',
>> >>                                         'vc'     : 'ChardevVC',
>> >> +                                       'ringbuf': 'ChardevRingbuf',
>> >> +                                       # next one is just for compatibility
>> >>                                         'memory' : 'ChardevRingbuf' } }
>> >
>> > Does JSON allow comments in the middle of content?  Is this going to
>> > screw up Amos' work on introspection?  You may need to instead have a
>> > comment before the open '{' stating that 'memory' is an alias within the
>> > union for back-compat reasons.
>
> I didn't parse the json file by myself. I just used the parsed
> dictionary. So it only needs to make qapi.py happy.
>
>> RFC 4627 doesn't do comments at all.
>> 
>> This file is parsed by scripts/qapi.py, which as far as I can tell
>> ignores lines starting with '#' anywhere in the input.
>
> Not anywhere, only start with '#'

Isn't that what I said?

> | def parse_schema(fp):
> |     exprs = []
> |     raw_exprs = []
> |     expr = ''
> |     expr_eval = None
> | 
> |     for line in fp:
> |         if line.startswith('#') or line == '\n':
>
>       # ignores lines starting with '#' anywhere
>           if line.strip().startswith('#')   
>
> |             continue
> | 
>
>
> So we should not add this kind of comment for back-compat.

Now I'm confused.  My patch adds a line that starts with '#'.
parse_schema() ignores it.  Works as designed.  Why do you think we
shouldn't do that?

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

[Amos Kong]

On Mon, Jul 01, 2013 at 10:10:40AM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
> > On Fri, Jun 28, 2013 at 07:19:39PM +0200, Markus Armbruster wrote:
> >> Eric Blake <eblake@redhat.com> writes:
> >> 
> >> > On 06/27/2013 08:22 AM, Markus Armbruster wrote:
> >> >> 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 6445da6..b3df8a5 100644
> >> >> --- a/qapi-schema.json
> >> >> +++ b/qapi-schema.json
> >> >> @@ -3277,9 +3277,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
> >> >>  ##
> >> >> @@ -3310,6 +3310,8 @@
> >> >>                                         'spicevmc' : 'ChardevSpiceChannel',
> >> >>                                         'spiceport' : 'ChardevSpicePort',
> >> >>                                         'vc'     : 'ChardevVC',
> >> >> +                                       'ringbuf': 'ChardevRingbuf',
> >> >> +                                       # next one is just for compatibility
> >> >>                                         'memory' : 'ChardevRingbuf' } }
> >> >
> >> > Does JSON allow comments in the middle of content?  Is this going to
> >> > screw up Amos' work on introspection?  You may need to instead have a
> >> > comment before the open '{' stating that 'memory' is an alias within the
> >> > union for back-compat reasons.
> >
> > I didn't parse the json file by myself. I just used the parsed
> > dictionary. So it only needs to make qapi.py happy.
> >
> >> RFC 4627 doesn't do comments at all.
> >> 
> >> This file is parsed by scripts/qapi.py, which as far as I can tell
> >> ignores lines starting with '#' anywhere in the input.
> >
> > Not anywhere, only start with '#'
> 
> Isn't that what I said?
> 
> > | def parse_schema(fp):
> > |     exprs = []
> > |     raw_exprs = []
> > |     expr = ''
> > |     expr_eval = None
> > | 
> > |     for line in fp:
> > |         if line.startswith('#') or line == '\n':
> >
> >       # ignores lines starting with '#' anywhere
> >           if line.strip().startswith('#')   
> >
> > |             continue
> > | 
> >
> >
> > So we should not add this kind of comment for back-compat.
> 
> Now I'm confused.  My patch adds a line that starts with '#'.
> parse_schema() ignores it.  Works as designed.  Why do you think we
> shouldn't do that?

The comment line in your patch doen't start with '#', it starts with
blank-space. If we want qapi.py to process it, we need to do strip()
first.

-- 
			Amos.

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> On Mon, Jul 01, 2013 at 10:10:40AM +0200, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
>> 
>> > On Fri, Jun 28, 2013 at 07:19:39PM +0200, Markus Armbruster wrote:
>> >> Eric Blake <eblake@redhat.com> writes:
>> >> 
>> >> > On 06/27/2013 08:22 AM, Markus Armbruster wrote:
>> >> >> 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 6445da6..b3df8a5 100644
>> >> >> --- a/qapi-schema.json
>> >> >> +++ b/qapi-schema.json
>> >> >> @@ -3277,9 +3277,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
>> >> >>  ##
>> >> >> @@ -3310,6 +3310,8 @@
>> >> >>                                         'spicevmc' : 'ChardevSpiceChannel',
>> >> >>                                         'spiceport' : 'ChardevSpicePort',
>> >> >>                                         'vc'     : 'ChardevVC',
>> >> >> +                                       'ringbuf': 'ChardevRingbuf',
>> >> >> +                                       # next one is just for compatibility
>> >> >>                                         'memory' : 'ChardevRingbuf' } }
>> >> >
>> >> > Does JSON allow comments in the middle of content?  Is this going to
>> >> > screw up Amos' work on introspection?  You may need to instead have a
>> >> > comment before the open '{' stating that 'memory' is an alias within the
>> >> > union for back-compat reasons.
>> >
>> > I didn't parse the json file by myself. I just used the parsed
>> > dictionary. So it only needs to make qapi.py happy.
>> >
>> >> RFC 4627 doesn't do comments at all.
>> >> 
>> >> This file is parsed by scripts/qapi.py, which as far as I can tell
>> >> ignores lines starting with '#' anywhere in the input.
>> >
>> > Not anywhere, only start with '#'
>> 
>> Isn't that what I said?
>> 
>> > | def parse_schema(fp):
>> > |     exprs = []
>> > |     raw_exprs = []
>> > |     expr = ''
>> > |     expr_eval = None
>> > | 
>> > |     for line in fp:
>> > |         if line.startswith('#') or line == '\n':
>> >
>> >       # ignores lines starting with '#' anywhere
>> >           if line.strip().startswith('#')   
>> >
>> > |             continue
>> > | 
>> >
>> >
>> > So we should not add this kind of comment for back-compat.
>> 
>> Now I'm confused.  My patch adds a line that starts with '#'.
>> parse_schema() ignores it.  Works as designed.  Why do you think we
>> shouldn't do that?
>
> The comment line in your patch doen't start with '#', it starts with
> blank-space. If we want qapi.py to process it, we need to do strip()
> first.

Aha.  I agree with your reading of parse_schema().  However, I get
exactly identical generated files with and without this comment.

Michael, Anthony, can you explain why?

Any particular reason for requiring comments to start in column 0?

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

[Amos Kong]

On Mon, Jul 01, 2013 at 11:28:58AM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
> > On Mon, Jul 01, 2013 at 10:10:40AM +0200, Markus Armbruster wrote:
> >> Amos Kong <akong@redhat.com> writes:
> >> 
> >> > On Fri, Jun 28, 2013 at 07:19:39PM +0200, Markus Armbruster wrote:
> >> >> Eric Blake <eblake@redhat.com> writes:
> >> >> 
> >> >> > On 06/27/2013 08:22 AM, Markus Armbruster wrote:
> >> >> >> 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 6445da6..b3df8a5 100644
> >> >> >> --- a/qapi-schema.json
> >> >> >> +++ b/qapi-schema.json
> >> >> >> @@ -3277,9 +3277,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
> >> >> >>  ##
> >> >> >> @@ -3310,6 +3310,8 @@
> >> >> >>                                         'spicevmc' : 'ChardevSpiceChannel',
> >> >> >>                                         'spiceport' : 'ChardevSpicePort',
> >> >> >>                                         'vc'     : 'ChardevVC',
> >> >> >> +                                       'ringbuf': 'ChardevRingbuf',
> >> >> >> +                                       # next one is just for compatibility
> >> >> >>                                         'memory' : 'ChardevRingbuf' } }
> >> >> >
> >> >> > Does JSON allow comments in the middle of content?  Is this going to
> >> >> > screw up Amos' work on introspection?  You may need to instead have a
> >> >> > comment before the open '{' stating that 'memory' is an alias within the
> >> >> > union for back-compat reasons.
> >> >
> >> > I didn't parse the json file by myself. I just used the parsed
> >> > dictionary. So it only needs to make qapi.py happy.
> >> >
> >> >> RFC 4627 doesn't do comments at all.
> >> >> 
> >> >> This file is parsed by scripts/qapi.py, which as far as I can tell
> >> >> ignores lines starting with '#' anywhere in the input.
> >> >
> >> > Not anywhere, only start with '#'
> >> 
> >> Isn't that what I said?
> >> 
> >> > | def parse_schema(fp):
> >> > |     exprs = []
> >> > |     raw_exprs = []
> >> > |     expr = ''
> >> > |     expr_eval = None
> >> > | 
> >> > |     for line in fp:
> >> > |         if line.startswith('#') or line == '\n':
> >> >
> >> >       # ignores lines starting with '#' anywhere
> >> >           if line.strip().startswith('#')   
> >> >
> >> > |             continue
> >> > | 
> >> >
> >> >
> >> > So we should not add this kind of comment for back-compat.
> >> 
> >> Now I'm confused.  My patch adds a line that starts with '#'.
> >> parse_schema() ignores it.  Works as designed.  Why do you think we
> >> shouldn't do that?
> >
> > The comment line in your patch doen't start with '#', it starts with
> > blank-space. If we want qapi.py to process it, we need to do strip()
> > first.
> 
> Aha.  I agree with your reading of parse_schema().  However, I get
> exactly identical generated files with and without this comment.

I tried to add a '#hello' prefix for 'serial' item, but 'serial' can still
be parsed as before. The '#hello' prefix will be ignored in qapi.py:tokenize()

  { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
                                         #hello 'serial' : 'ChardevHostdev',
                                         'parallel': 'ChardevHostdev',
   

> Michael, Anthony, can you explain why?
> 
> Any particular reason for requiring comments to start in column 0?

-- 
			Amos.

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

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Amos Kong <akong@redhat.com> writes:
>
>> On Mon, Jul 01, 2013 at 10:10:40AM +0200, Markus Armbruster wrote:
>>> Amos Kong <akong@redhat.com> writes:
>>> 
>> The comment line in your patch doen't start with '#', it starts with
>> blank-space. If we want qapi.py to process it, we need to do strip()
>> first.
>
> Aha.  I agree with your reading of parse_schema().  However, I get
> exactly identical generated files with and without this comment.
>
> Michael, Anthony, can you explain why?
>
> Any particular reason for requiring comments to start in column 0?

I don't remember a particular reason.  Most likely an evolutionary
artifact.

Regards,

Anthony Liguori

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> On Mon, Jul 01, 2013 at 11:28:58AM +0200, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
>> 
>> > On Mon, Jul 01, 2013 at 10:10:40AM +0200, Markus Armbruster wrote:
>> >> Amos Kong <akong@redhat.com> writes:
>> >> 
>> >> > On Fri, Jun 28, 2013 at 07:19:39PM +0200, Markus Armbruster wrote:
>> >> >> Eric Blake <eblake@redhat.com> writes:
>> >> >> 
>> >> >> > On 06/27/2013 08:22 AM, Markus Armbruster wrote:
>> >> >> >> 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 6445da6..b3df8a5 100644
>> >> >> >> --- a/qapi-schema.json
>> >> >> >> +++ b/qapi-schema.json
>> >> >> >> @@ -3277,9 +3277,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
>> >> >> >>  ##
>> >> >> >> @@ -3310,6 +3310,8 @@
>> >> >> >>                                         'spicevmc' : 'ChardevSpiceChannel',
>> >> >> >>                                         'spiceport' : 'ChardevSpicePort',
>> >> >> >>                                         'vc'     : 'ChardevVC',
>> >> >> >> +                                       'ringbuf': 'ChardevRingbuf',
>> >> >> >> +                                       # next one is just for compatibility
>> >> >> >>                                         'memory' : 'ChardevRingbuf' } }
>> >> >> >
>> >> >> > Does JSON allow comments in the middle of content?  Is this going to
>> >> >> > screw up Amos' work on introspection?  You may need to instead have a
>> >> >> > comment before the open '{' stating that 'memory' is an
>> >> >> > alias within the
>> >> >> > union for back-compat reasons.
>> >> >
>> >> > I didn't parse the json file by myself. I just used the parsed
>> >> > dictionary. So it only needs to make qapi.py happy.
>> >> >
>> >> >> RFC 4627 doesn't do comments at all.
>> >> >> 
>> >> >> This file is parsed by scripts/qapi.py, which as far as I can tell
>> >> >> ignores lines starting with '#' anywhere in the input.
>> >> >
>> >> > Not anywhere, only start with '#'
>> >> 
>> >> Isn't that what I said?
>> >> 
>> >> > | def parse_schema(fp):
>> >> > |     exprs = []
>> >> > |     raw_exprs = []
>> >> > |     expr = ''
>> >> > |     expr_eval = None
>> >> > | 
>> >> > |     for line in fp:
>> >> > |         if line.startswith('#') or line == '\n':
>> >> >
>> >> >       # ignores lines starting with '#' anywhere
>> >> >           if line.strip().startswith('#')   
>> >> >
>> >> > |             continue
>> >> > | 
>> >> >
>> >> >
>> >> > So we should not add this kind of comment for back-compat.
>> >> 
>> >> Now I'm confused.  My patch adds a line that starts with '#'.
>> >> parse_schema() ignores it.  Works as designed.  Why do you think we
>> >> shouldn't do that?
>> >
>> > The comment line in your patch doen't start with '#', it starts with
>> > blank-space. If we want qapi.py to process it, we need to do strip()
>> > first.
>> 
>> Aha.  I agree with your reading of parse_schema().  However, I get
>> exactly identical generated files with and without this comment.
>
> I tried to add a '#hello' prefix for 'serial' item, but 'serial' can still
> be parsed as before. The '#hello' prefix will be ignored in qapi.py:tokenize()
>
>   { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
>                                          #hello 'serial' : 'ChardevHostdev',
>                                          'parallel': 'ChardevHostdev',

Tokenize silently ignores characters it doesn't recognize.  Pass the
crackpipe, pal!

[...]

Re: [Qemu-devel] [PATCH 1/4] qemu-char: Fix ringbuf option size

[Luiz Capitulino]

On Thu, 27 Jun 2013 16:22:07 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Any attempt to use it trips an "opt->desc->type == QEMU_OPT_NUMBER"
> assertion.  Broken in commit 1da48c65.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Applied to the qmp branch, thanks.

> ---
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 63972ae..63a9221 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3108,7 +3108,7 @@ static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
>  
>      backend->memory = g_new0(ChardevMemory, 1);
>  
> -    val = qemu_opt_get_number(opts, "size", 0);
> +    val = qemu_opt_get_size(opts, "size", 0);
>      if (val != 0) {
>          backend->memory->has_size = true;
>          backend->memory->size = val;

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

[Luiz Capitulino]

On Thu, 27 Jun 2013 16:22:09 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> 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>

This patch doesn't apply anymore, can you respin please?

> ---
>  qemu-char.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index a8fad65..968f572 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3755,7 +3755,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);
> @@ -3773,6 +3773,9 @@ static void register_types(void)
>      register_char_driver_qapi("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL);
>      register_char_driver_qapi("pipe", CHARDEV_BACKEND_KIND_PIPE,
>                                qemu_chr_parse_pipe);
> +    /* Bug-compatibility: */
> +    register_char_driver_qapi("memory", CHARDEV_BACKEND_KIND_MEMORY,
> +                              qemu_chr_parse_ringbuf);
>  }
>  
>  type_init(register_types);

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

[Luiz Capitulino]

On Thu, 27 Jun 2013 16:22:06 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> The first patch is a straightforward bug fix.
> 
> The rest 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 "qemu-char: Fix ID
>   reuse after chardev-remove for qapi-based init" bug.
> 
> I include the compatibility gunk anyway, so we can have a more
> informed discussion.

Series looks good to me, but patch 3/4 doesn't apply anymore.
I've applied patch 1/4 anyway.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 27 Jun 2013 16:22:09 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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>
>
> This patch doesn't apply anymore, can you respin please?

Certainly.