Re: [Qemu-devel] [PATCH v15 08/23] monitor: Let generated code validate arguments

[Eric Blake]

On 04/28/2016 08:09 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Having to manually call out the set of expected arguments in
>> qmp-commands.hx, in addition to what is already in *.json,
>> is tedious and prone to error.  The only reason we use
>> .args_type is for checking if there is any excess arguments
>> or incorrectly typed arguments during qmp_check_client_args(),
>> but a strict QMP Input visitor also does that checking.
> 
> We also use it for completion.

Does scripts/qmp/qmp-shell do completion?  It's a script, is it parsing
the .hx file?

I know we do completion for HMP, but this is QMP that I'm tweaking, and
other than qmp-shell, I don't know of any qemu code that wants to do QMP
completion.  We have query-qmp-schema that could be used to provide
completion, if needed.

>> and for commands with at least one (possibly-optional) argument,
>> the output changes from:
>>
>> {"execute":"query-command-line-options","arguments":{"a":1}}
>> {"error": {"class": "GenericError", "desc": "Invalid parameter 'a'"}}
>>

QERR_INVALID_PARAMETER

>> to:
>>
>> {"execute":"query-command-line-options","arguments":{"a":1}}
>> {"error": {"class": "GenericError", "desc": "QMP input object member 'a' is 
>> unexpected"}}
> 
> This error message becomes rather generic.  Tolerable.  Can we restore
> the old message without trouble?

QERR_QMP_EXTRA_MEMBER

Yes, it's quite easy to switch between the two error message strings by
tweaking the choice of message in qmp_input_check_struct() (at the end
of the series; it's in qmp_input_pop() at this point of the series).

>>
>> -    (void)args;
>> -''')
>> +    if (args && qdict_size(args)) {
>> +        error_setg(errp, "Command '%%s' does not take arguments", 
>> "%(name)s");
>> +        return;
>> +    }
>> +''',
>> +                     name=name)
>>
>>      ret += gen_call(name, arg_type, ret_type)
>>
> 
> Works for me.
> 
> Naive question: is the special case "no arguments" really necessary
> here?  Could we visit the empty struct instead?

If we had an empty struct visitor around. But right now the generator
special-cases ':empty' so that it has no generated functions.

>> @@ -2362,7 +2281,7 @@ EQMP
>>
>>      {
>>          .name       = "query-qmp-schema",
>> -        .args_type  = "",
>> +    .args_type  = "",
>>          .mhandler.cmd_new = qmp_query_qmp_schema,
>>      },
> 
> Spurious hunk.

Whoops. I first deleted it, then realized this was one of the few places
that doesn't use qmp_marshal_ so I had to restore it, but restored it
with the wrong whitespace.

> 
> Entries defining anything beyond .name and .mhandler.cmd_new:
> 
> * device_add, qmp_capabilities
> 
>   Not QAPIfied, need everything.
> 
> * netdev_add, query-qmp-schema
> 
>   Need .args_type because of 'gen': false.
> 
> * client_migrate_info, dump-guest-memory, query-dump, getfd, closefd,
>   add-fd, remove-fd, query-fdsets, migrate-set-capabilities
> 
>   Define .params and/or .help.  Does this make any sense?
> 
> A comment explaining which members need to be set would be nice.
> 
> Have you checked Marc-André's work for overlap?  Cc'ing him.

Marc-André QAPIfies device_add and qmp_capabilities, then completely
eliminates qmp-commands.hx.  This probably duplicates some of the work
in his queue, but should be fine in the short term.  As for whether any
of the other fields are needed or useful, I didn't check (but suspect
that no, they are not needed, again because this is QMP not HMP and that
only HMP 'info cmd' cares).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature