On 04/28/2016 08:09 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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
