[Qemu-devel] [PATCH 0/2 v2] Chardev related fixes

[Qemu-devel] [PATCH 0/2 v2] Chardev related fixes

[Lei Li]

This series tries to fix the consistency of char devices
ringbuf and a regression of chardev filename.

Changes since v1:
  - Rename the struct related to new qapi ringbuf to memory, and
    fix the wrong description in qemu-option from Paolo.

Lei Li (2):
  chardev: Make the name of memory device consistent
  chardev: Get filename for new qapi backend

 qapi-schema.json |    6 +++---
 qemu-char.c      |   18 ++++++++++--------
 qemu-options.hx  |    6 +++---
 3 files changed, 16 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/2] chardev: Make the name of memory device consistent

[Lei Li]

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' } }
 
 ##
 # @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}).
-- 
1.7.7.6

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

[Lei Li]

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}).
>>


-- 
Lei

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

[Eric Blake]

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

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.

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

[Paolo Bonzini]

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

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

[Eric Blake]

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

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>

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

[Qemu-devel] [PATCH 2/2] chardev: Get filename for new qapi backend

[Lei Li]

This patch sets the filename when the new qapi backend
init from opts.

The previous patch and discussions as link below:

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

If anyone who have better idea to fix this please let
me know your suggestions.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 qemu-char.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index ebeed04..781e969 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3276,6 +3276,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
         ChardevReturn *ret = NULL;
         const char *id = qemu_opts_id(opts);
         const char *bid = NULL;
+        char *filename = g_strdup(qemu_opt_get(opts, "backend"));
 
         if (qemu_opt_get_bool(opts, "mux", 0)) {
             bid = g_strdup_printf("%s-base", id);
@@ -3308,6 +3309,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
         }
 
         chr = qemu_chr_find(id);
+        chr->filename = filename;
 
     qapi_out:
         qapi_free_ChardevBackend(backend);
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 0/2 v2] Chardev related fixes

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori