[Qemu-devel] [PATCH] qmp-commands: fix incorrect uses of ":O" specifier

[Qemu-devel] [PATCH] qmp-commands: fix incorrect uses of ":O" specifier

[Paolo Bonzini]

As far as the QMP parser is concerned, neither the 'O' nor the 'q' format specifiers
put any constraint on the command.  However, there are two differences:

1) from a documentation point of view 'O' says that this command takes
a dictionary.  The dictionary will be converted to QemuOpts in the
handler to match the corresponding HMP command.

2) 'O' sets QMP_ACCEPT_UNKNOWNS, resulting in the command accepting invalid
extra arguments.  For example the following is accepted:

   { "execute": "send-key",
        "arguments": { "keys": [ { "type": "qcode", "data": "ctrl" },
                                 { "type": "qcode", "data": "alt" },
                                 { "type": "qcode", "data": "delete" } ], "foo": "bar" } }

Neither send-key nor migrate-set-capabilities take a QemuOpts-like
dictionary; they take an array of dictionaries.  And neither command
really wants to have extra unknown arguments.  Thus, the right
specifier to use in this case is 'q'; with this patch the above
command fails with

   {"error": {"class": "GenericError", "desc": "Invalid parameter 'foo'"}}

as intended.

Reported-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qmp-commands.hx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 3a42ad0..09f48ba 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -332,7 +332,7 @@ EQMP
 
     {
         .name       = "send-key",
-        .args_type  = "keys:O,hold-time:i?",
+        .args_type  = "keys:q,hold-time:i?",
         .mhandler.cmd_new = qmp_marshal_input_send_key,
     },
 
@@ -3288,7 +3288,7 @@ EQMP
 
     {
         .name       = "migrate-set-capabilities",
-        .args_type  = "capabilities:O",
+        .args_type  = "capabilities:q",
         .params     = "capability:s,state:b",
 	.mhandler.cmd_new = qmp_marshal_input_migrate_set_capabilities,
     },
-- 
2.3.5

Re: [Qemu-devel] [PATCH] qmp-commands: fix incorrect uses of ":O" specifier

[Alberto Garcia]

On Wed 15 Apr 2015 01:30:04 PM CEST, Paolo Bonzini wrote:

> As far as the QMP parser is concerned, neither the 'O' nor the 'q'
> format specifiers put any constraint on the command.  However, there
> are two differences:
>
> 1) from a documentation point of view 'O' says that this command takes
> a dictionary.  The dictionary will be converted to QemuOpts in the
> handler to match the corresponding HMP command.

Is that documentation the comments in monitor.c? It could also be a good
moment to document there 'q' as well.

Otherwise the patch looks good to me,

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH] qmp-commands: fix incorrect uses of ":O" specifier

[Paolo Bonzini]

On 15/04/2015 14:18, Alberto Garcia wrote:
> On Wed 15 Apr 2015 01:30:04 PM CEST, Paolo Bonzini wrote:
> 
>> As far as the QMP parser is concerned, neither the 'O' nor the 'q'
>> format specifiers put any constraint on the command.  However, there
>> are two differences:
>>
>> 1) from a documentation point of view 'O' says that this command takes
>> a dictionary.  The dictionary will be converted to QemuOpts in the
>> handler to match the corresponding HMP command.
> 
> Is that documentation the comments in monitor.c? It could also be a good
> moment to document there 'q' as well.

No, I mean what you can learn by parsing the .args_type line.

'foo:O' means there will typically be no argument named foo, and the
dictionary will be treated as a series of key/value pairs.

'foo:q' means there will be an argument named foo, which is a JSON
object of some kind.

Paolo

> Otherwise the patch looks good to me,
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 

Re: [Qemu-devel] [PATCH] qmp-commands: fix incorrect uses of ":O" specifier

[Luiz Capitulino]

On Wed, 15 Apr 2015 13:30:04 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> As far as the QMP parser is concerned, neither the 'O' nor the 'q' format specifiers
> put any constraint on the command.  However, there are two differences:
> 
> 1) from a documentation point of view 'O' says that this command takes
> a dictionary.  The dictionary will be converted to QemuOpts in the
> handler to match the corresponding HMP command.
> 
> 2) 'O' sets QMP_ACCEPT_UNKNOWNS, resulting in the command accepting invalid
> extra arguments.  For example the following is accepted:
> 
>    { "execute": "send-key",
>         "arguments": { "keys": [ { "type": "qcode", "data": "ctrl" },
>                                  { "type": "qcode", "data": "alt" },
>                                  { "type": "qcode", "data": "delete" } ], "foo": "bar" } }
> 
> Neither send-key nor migrate-set-capabilities take a QemuOpts-like
> dictionary; they take an array of dictionaries.  And neither command
> really wants to have extra unknown arguments.  Thus, the right
> specifier to use in this case is 'q'; with this patch the above
> command fails with
> 
>    {"error": {"class": "GenericError", "desc": "Invalid parameter 'foo'"}}
> 
> as intended.
> 
> Reported-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Applied to the qmp branch, thanks.

> ---
>  qmp-commands.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 3a42ad0..09f48ba 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -332,7 +332,7 @@ EQMP
>  
>      {
>          .name       = "send-key",
> -        .args_type  = "keys:O,hold-time:i?",
> +        .args_type  = "keys:q,hold-time:i?",
>          .mhandler.cmd_new = qmp_marshal_input_send_key,
>      },
>  
> @@ -3288,7 +3288,7 @@ EQMP
>  
>      {
>          .name       = "migrate-set-capabilities",
> -        .args_type  = "capabilities:O",
> +        .args_type  = "capabilities:q",
>          .params     = "capability:s,state:b",
>  	.mhandler.cmd_new = qmp_marshal_input_migrate_set_capabilities,
>      },