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

[Lei Li <lilei@linux.vnet.ibm.com>]

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