Patchwork [1/2] chardev: Make the name of memory device consistent

login
register
mail settings
Submitter Lei Li
Date May 21, 2013, 10:27 a.m.
Message ID <1369132079-11377-2-git-send-email-lilei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/245260/
State New
Headers show

Comments

Lei Li - May 21, 2013, 10:27 a.m.
Now we have memory char device, but the backend name of it
is a little confusion. We actually register it by 'memory', but
the description in qemu-option, the name of open functions
and the new api backend called it 'ringbuf'. It should keep
consistent. This patch named it all to 'memory'.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qapi-schema.json |    6 +++---
 qemu-char.c      |   16 ++++++++--------
 qemu-options.hx  |    6 +++---
 3 files changed, 14 insertions(+), 14 deletions(-)
Lei Li - May 21, 2013, 12:19 p.m.
On 05/21/2013 06:34 PM, Paolo Bonzini wrote:
> Il 21/05/2013 12:27, Lei Li ha scritto:
>> Now we have memory char device, but the backend name of it
>> is a little confusion. We actually register it by 'memory', but
>> the description in qemu-option, the name of open functions
>> and the new api backend called it 'ringbuf'. It should keep
>> consistent. This patch named it all to 'memory'.
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

This might miss cc to all. :)

>
>> ---
>>   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 9302e7d..664b31f 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3286,7 +3286,7 @@
>>                                    '*rows'   : 'int' } }
>>   
>>   ##
>> -# @ChardevRingbuf:
>> +# @ChardevMemory:
>>   #
>>   # Configuration info for memory chardevs
>>   #
>> @@ -3294,7 +3294,7 @@
>>   #
>>   # Since: 1.5
>>   ##
>> -{ 'type': 'ChardevRingbuf', 'data': { '*size'  : 'int' } }
>> +{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
>>   
>>   ##
>>   # @ChardevBackend:
>> @@ -3321,7 +3321,7 @@
>>                                          'spicevmc' : 'ChardevSpiceChannel',
>>                                          'spiceport' : 'ChardevSpicePort',
>>                                          'vc'     : 'ChardevVC',
>> -                                       'memory' : 'ChardevRingbuf' } }
>> +                                       'memory' : 'ChardevMemory' } }
>>   
>>   ##
>>   # @ChardevReturn:
>> diff --git a/qemu-char.c b/qemu-char.c
>> index cff2896..ebeed04 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2875,8 +2875,8 @@ static void ringbuf_chr_close(struct CharDriverState *chr)
>>       chr->opaque = NULL;
>>   }
>>   
>> -static CharDriverState *qemu_chr_open_ringbuf(ChardevRingbuf *opts,
>> -                                              Error **errp)
>> +static CharDriverState *qemu_chr_open_memory(ChardevMemory *opts,
>> +                                             Error **errp)
>>   {
>>       CharDriverState *chr;
>>       RingBufCharDriver *d;
>> @@ -2888,7 +2888,7 @@ static CharDriverState *qemu_chr_open_ringbuf(ChardevRingbuf *opts,
>>   
>>       /* The size must be power of 2 */
>>       if (d->size & (d->size - 1)) {
>> -        error_setg(errp, "size of ringbuf chardev must be power of two");
>> +        error_setg(errp, "size of memory chardev must be power of two");
>>           goto fail;
>>       }
>>   
>> @@ -3190,12 +3190,12 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
>>       backend->pipe->device = g_strdup(device);
>>   }
>>   
>> -static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
>> -                                   Error **errp)
>> +static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
>> +                                  Error **errp)
>>   {
>>       int val;
>>   
>> -    backend->memory = g_new0(ChardevRingbuf, 1);
>> +    backend->memory = g_new0(ChardevMemory, 1);
>>   
>>       val = qemu_opt_get_number(opts, "size", 0);
>>       if (val != 0) {
>> @@ -3787,7 +3787,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_ringbuf(backend->memory, errp);
>> +        chr = qemu_chr_open_memory(backend->memory, errp);
>>           break;
>>       default:
>>           error_setg(errp, "unknown chardev backend (%d)", backend->kind);
>> @@ -3832,7 +3832,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_ringbuf);
>> +                              qemu_chr_parse_memory);
>>       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 fb62b75..fb3961d 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -1779,7 +1779,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 ringbuf,id=id[,size=size]\n"
>> +    "-chardev memory,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
>> @@ -1817,7 +1817,7 @@ Backend is one of:
>>   @option{udp},
>>   @option{msmouse},
>>   @option{vc},
>> -@option{ringbuf},
>> +@option{memory},
>>   @option{file},
>>   @option{pipe},
>>   @option{console},
>> @@ -1926,7 +1926,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 ringbuf ,id=@var{id} [,size=@var{size}]
>> +@item -chardev memory ,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}).
>>
Eric Blake - May 21, 2013, 12:38 p.m.
On 05/21/2013 04:27 AM, Lei Li wrote:
> Now we have memory char device, but the backend name of it
> is a little confusion. We actually register it by 'memory', but
> the description in qemu-option, the name of open functions
> and the new api backend called it 'ringbuf'. It should keep
> consistent. This patch named it all to 'memory'.
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.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 9302e7d..664b31f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3286,7 +3286,7 @@
>                                   '*rows'   : 'int' } }
>  
>  ##
> -# @ChardevRingbuf:
> +# @ChardevMemory:
>  #
>  # Configuration info for memory chardevs
>  #
> @@ -3294,7 +3294,7 @@
>  #
>  # Since: 1.5
>  ##
> -{ 'type': 'ChardevRingbuf', 'data': { '*size'  : 'int' } }
> +{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }

Since we don't have introspection (yet), this change is fine.

> +++ b/qemu-options.hx
> @@ -1779,7 +1779,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 ringbuf,id=id[,size=size]\n"
> +    "-chardev memory,id=id[,size=size]\n"

This change impacts the command line.  Have you tested whether
'query-command-line-options' exposes the difference?  Libvirt does not
(yet) expose a ringbuf/memory device.  Assuming we add support in the
future, we have several possibilities:

1. we have some way to detect the command line naming, and can thus
support the device for both qemu 1.5 and 1.6
2. we can't detect the command line naming, so we can only use the
device in 1.6 or newer even though it was technically available in 1.5
3. qemu continues to support ringbuf on the command line as a (possibly
undocumented) alias forever, so that libvirt would ALWAYS use ringbuf
regardless of qemu version

My preference would be #1 or #3; but I could live with #2 as a last resort.

As written, I don't know if your patch is option #1 or #2.  I don't know
whether query-command-line-options can expose the difference, or whether
it can be enhanced to expose the difference in time for 1.0.6 (where if
the enhancement is not present, we assume 1.5 behavior of the old name).
 If you can prove we can introspect the change in the command line (ie.
prove this is option #1), then you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

although I still think it might be worth considering option #3 in a
respin or as a followup patch.
Paolo Bonzini - May 21, 2013, 12:50 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 21/05/2013 14:38, Eric Blake ha scritto:
> On 05/21/2013 04:27 AM, Lei Li wrote:
>> Now we have memory char device, but the backend name of it is a
>> little confusion. We actually register it by 'memory', but the
>> description in qemu-option, the name of open functions and the
>> new api backend called it 'ringbuf'. It should keep consistent.
>> This patch named it all to 'memory'.
>> 
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.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
>> 9302e7d..664b31f 100644 --- a/qapi-schema.json +++
>> b/qapi-schema.json @@ -3286,7 +3286,7 @@ '*rows'   : 'int' } }
>> 
>> ## -# @ChardevRingbuf: +# @ChardevMemory: # # Configuration info
>> for memory chardevs # @@ -3294,7 +3294,7 @@ # # Since: 1.5 ## -{
>> 'type': 'ChardevRingbuf', 'data': { '*size'  : 'int' } } +{
>> 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
> 
> Since we don't have introspection (yet), this change is fine.
> 
>> +++ b/qemu-options.hx @@ -1779,7 +1779,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 ringbuf,id=id[,size=size]\n" +    "-chardev
>> memory,id=id[,size=size]\n"
> 
> This change impacts the command line.

This is just the help text.  "qemu-system-x86_64 -chardev
memory,id=foo" works here on 1.4.1.

Paolo

  Have you tested whether
> 'query-command-line-options' exposes the difference?  Libvirt does
> not (yet) expose a ringbuf/memory device.  Assuming we add support
> in the future, we have several possibilities:
> 
> 1. we have some way to detect the command line naming, and can
> thus support the device for both qemu 1.5 and 1.6 2. we can't
> detect the command line naming, so we can only use the device in
> 1.6 or newer even though it was technically available in 1.5 3.
> qemu continues to support ringbuf on the command line as a
> (possibly undocumented) alias forever, so that libvirt would ALWAYS
> use ringbuf regardless of qemu version
> 
> My preference would be #1 or #3; but I could live with #2 as a last
> resort.
> 
> As written, I don't know if your patch is option #1 or #2.  I don't
> know whether query-command-line-options can expose the difference,
> or whether it can be enhanced to expose the difference in time for
> 1.0.6 (where if the enhancement is not present, we assume 1.5
> behavior of the old name). If you can prove we can introspect the
> change in the command line (ie. prove this is option #1), then you
> can add: Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> although I still think it might be worth considering option #3 in
> a respin or as a followup patch.
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRm22XAAoJEBvWZb6bTYbyoA0P/0+aTY+L0f10PAS9HenLcMHN
4E4EDju2mhHSFDwiH05x8rax+/YNDPuGq96OD9Z2HJdtngDHUztmKmpB12kcmR72
skGgCafXatWk9In8DQKhKGFtHmdLStKg7DfDe8US8muxjEM+IXKFxynjkrycKPYn
IttHBzeorZSHlLoAAz4Tlj096qt5oEvnIBYoJGIeMLCSbWDdSVk8FBvYPI2jfjkM
HkudPq1C3Z0k1Xl9s4yUkk/5wztLVxaiO1Bh0JU50txUfQpFrFFHA9C0RU/XPja/
K2ZUQzqL7LpcOvMweV4oCOdOL8PXPPD0JimmrLRpT40YyrNTFLv+cwmhGv/9Vseo
EpzWrSpYWHL/kmioxu7voHYuNqLNvizucboJz/pLs72dQubW+RzeRrieVHjJxUyK
dnqFaspHOAeG9s5YKvwG1ObnORPWGdQvr035NPkkYgQX2R7ndEICuIKr5nqcdjXR
HNZv59f/bxEEVRwx6wHrFsQ+6/417xIXwXJenZdZiZVikiCoNuaJekC+5fLYy1Q8
AamQq/xkuOthW8/s0Nykc3JqrVGZJ1RiYI7rDj6stMciFNlkWdTsYESNpi/Qq5Jb
qTkIea2O0dEQ9nRt3/sOpOITBST4nIEdzWnvRqZXPEocwhqbNwH/C4qe65mJK+RE
NTPjDtsKpDTODSAEl4tq
=Gcyt
-----END PGP SIGNATURE-----
Eric Blake - May 21, 2013, 12:57 p.m.
On 05/21/2013 06:50 AM, Paolo Bonzini wrote:

>>> -    "-chardev ringbuf,id=id[,size=size]\n" +    "-chardev
>>> memory,id=id[,size=size]\n"
> 
>> This change impacts the command line.
> 
> This is just the help text.  "qemu-system-x86_64 -chardev
> memory,id=foo" works here on 1.4.1.

Good to know.  In that case:

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

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index 9302e7d..664b31f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3286,7 +3286,7 @@ 
                                  '*rows'   : 'int' } }
 
 ##
-# @ChardevRingbuf:
+# @ChardevMemory:
 #
 # Configuration info for memory chardevs
 #
@@ -3294,7 +3294,7 @@ 
 #
 # Since: 1.5
 ##
-{ 'type': 'ChardevRingbuf', 'data': { '*size'  : 'int' } }
+{ 'type': 'ChardevMemory', 'data': { '*size'  : 'int' } }
 
 ##
 # @ChardevBackend:
@@ -3321,7 +3321,7 @@ 
                                        'spicevmc' : 'ChardevSpiceChannel',
                                        'spiceport' : 'ChardevSpicePort',
                                        'vc'     : 'ChardevVC',
-                                       'memory' : 'ChardevRingbuf' } }
+                                       'memory' : 'ChardevMemory' } }
 
 ##
 # @ChardevReturn:
diff --git a/qemu-char.c b/qemu-char.c
index cff2896..ebeed04 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2875,8 +2875,8 @@  static void ringbuf_chr_close(struct CharDriverState *chr)
     chr->opaque = NULL;
 }
 
-static CharDriverState *qemu_chr_open_ringbuf(ChardevRingbuf *opts,
-                                              Error **errp)
+static CharDriverState *qemu_chr_open_memory(ChardevMemory *opts,
+                                             Error **errp)
 {
     CharDriverState *chr;
     RingBufCharDriver *d;
@@ -2888,7 +2888,7 @@  static CharDriverState *qemu_chr_open_ringbuf(ChardevRingbuf *opts,
 
     /* The size must be power of 2 */
     if (d->size & (d->size - 1)) {
-        error_setg(errp, "size of ringbuf chardev must be power of two");
+        error_setg(errp, "size of memory chardev must be power of two");
         goto fail;
     }
 
@@ -3190,12 +3190,12 @@  static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
     backend->pipe->device = g_strdup(device);
 }
 
-static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
-                                   Error **errp)
+static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
+                                  Error **errp)
 {
     int val;
 
-    backend->memory = g_new0(ChardevRingbuf, 1);
+    backend->memory = g_new0(ChardevMemory, 1);
 
     val = qemu_opt_get_number(opts, "size", 0);
     if (val != 0) {
@@ -3787,7 +3787,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_ringbuf(backend->memory, errp);
+        chr = qemu_chr_open_memory(backend->memory, errp);
         break;
     default:
         error_setg(errp, "unknown chardev backend (%d)", backend->kind);
@@ -3832,7 +3832,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_ringbuf);
+                              qemu_chr_parse_memory);
     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 fb62b75..fb3961d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1779,7 +1779,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 ringbuf,id=id[,size=size]\n"
+    "-chardev memory,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
@@ -1817,7 +1817,7 @@  Backend is one of:
 @option{udp},
 @option{msmouse},
 @option{vc},
-@option{ringbuf},
+@option{memory},
 @option{file},
 @option{pipe},
 @option{console},
@@ -1926,7 +1926,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 ringbuf ,id=@var{id} [,size=@var{size}]
+@item -chardev memory ,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}).