From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38343) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsPTI-0005JJ-OU for qemu-devel@nongnu.org; Thu, 27 Jun 2013 23:35:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UsPTH-0005V7-3e for qemu-devel@nongnu.org; Thu, 27 Jun 2013 23:35:40 -0400 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:57704) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsPTG-0005U4-EF for qemu-devel@nongnu.org; Thu, 27 Jun 2013 23:35:39 -0400 Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Jun 2013 08:58:20 +0530 Message-ID: <51CD0476.20301@linux.vnet.ibm.com> Date: Fri, 28 Jun 2013 11:35:18 +0800 From: Lei Li MIME-Version: 1.0 References: <1372342930-28684-1-git-send-email-armbru@redhat.com> <1372342930-28684-3-git-send-email-armbru@redhat.com> In-Reply-To: <1372342930-28684-3-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/4] Revert "chardev: Make the name of memory device consistent" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: aliguori@us.ibm.com, qemu-devel , qemu-stable@nongnu.org, lcapitulino@redhat.com, kraxel@redhat.com, Paolo Bonzini 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 > --- > 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