Re: [Qemu-devel] [PATCH v5] monitor: introduce query-command-line-options

[Eric Blake]

On 04/25/2013 07:20 AM, Luiz Capitulino wrote:
> On Thu, 25 Apr 2013 17:50:35 +0800
> Amos Kong <address@hidden> wrote:
> 
>> Libvirt has no way to probe if an option or property is supported,
>> This patch introduces a new qmp command to query command line
>> option information. hmp command isn't added because it's not needed.
>>
>> Signed-off-by: Amos Kong <address@hidden>
>> CC: Luiz Capitulino <address@hidden>
>> CC: Osier Yang <address@hidden>
>> CC: Anthony Liguori <address@hidden>
> 
> This version looks good to me, but it has two small problems.
> 
> First, some options like -drive, -net, -netdev and -device have an empty
> QemuOptsList, meaning that option validation is not done upfront.
> 
> The other problem is that we're returning some options which seem to be
> created at run-time and I honestly have never heard of them, like tmpdev
> and acpi.
> 
> I think there are two possible fixes for both issues:
> 
>  1. Do nothing, accept patch as is and fix things in 1.6

I can live with this for 1.5.  Initially, libvirt will probably only be
querying whether a set of known option names exist, and not looking at
the parameter definitions tied to the option nor trying to invoke
command line options that libvirt doesn't already know.  Put another
way, the driving factor to get this into 1.5 is so that libvirt knows
whether it can use -mem-merge:

https://www.redhat.com/archives/libvir-list/2013-April/msg01263.html

While I anticipate that future libvirt versions may get fancier and use
even more details of option introspection, such as learning whether a
parameter was added in a later release, that's not the driving factor
right now.

> 
>  2. Don't return an option when 'desc' is NULL

That might backfire; it's better to output too much than too little.

> 
> I'm willing to do 1, as those options don't return any options anyway and
> I don't believe any client will mess with them (worst case the client will 
> pass
> an option qemu doesn't accept and will refuse to run).

Yep, and libvirt certainly won't be calling 'qemu -tmpdev'.

> 
> I have one more comment below, but case we accept to do 1 I can fix it myself.
> 

>> +{'command': 'query-command-line-options', 'data': { '*option': 'str' },
> 
> I'm not a huge fan of option being optional, I prefer dropping it and making
> the command simpler. But I won't refuse the patch because of that.

I'm leaving that up to you.  I personally like the idea of filtering (I
would definitely use it via 'virsh qemu-monitor-command' when doing
development work on libvirt), but it's not essential.

>> +    if (conf_list == NULL) {
>> +        error_set(errp, QERR_INVALID_OPTION_GROUP, option);
> 
> You should be using error_setg() like:
> 
>  error_setg(errp, "invalid option name: %s", option);
> 
> But I can fix that myself in qmp branch.

Based on include/qapi/qmp/qerror.h, the message you would be replacing is:

#define QERR_INVALID_OPTION_GROUP \
    ERROR_CLASS_GENERIC_ERROR, "There is no option group '%s'"

Yeah, "invalid option name: foo" reads better than "There is no option
group 'foo'".

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

signature.asc
Description: OpenPGP digital signature