Re: [Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options

[Eric Blake <eblake@redhat.com>]

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

On 02/11/2014 05:19 AM, Markus Armbruster wrote:
> [Note cc: Eric]
> 
> Amos Kong <akong@redhat.com> writes:
> 
>> Some legacy options that have arguments weren't added to
>> vm_config_groups[], so query-command-line-options returns a
>> NULL parameters infolist. This patch try to return help message
>> for this kind of legacy options.
>>
>> Example:
>>  {
>>      "helpmsg": "\"-vnc display    start a VNC server on display\\n\"",
>>      "parameters": [
>>      ],
>>      "option": "vnc"
>>  },
> 
> Do we have prospective users for this feature?

Libvirt probably won't care about helpmsg other than the fact that it
gets logged as part of the QMP reply, and the log is more legible if
human-readable text is included.  I don't care if you add it or omit it;
the REAL change here is...

>>  ##
>>  { 'type': 'CommandLineOptionInfo',
>> -  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
>> +  'data': { 'option': 'str',
>> +            '*parameters': ['CommandLineParameterInfo'],

changing parameters from mandatory to optional, and omitting it when
supplying information on an arg-less option.  And that is what libvirt
wants, for learning about things like -enable-fips.

>> +            '*helpmsg': 'str' } }
>>  
>>  ##
>>  # @query-command-line-options:
> 
> Aha, here's the schema change missing in PATCH 3/5.

Indeed; please resubmit the series so that every patch builds on its own.

> 
> The schema needs to cover these kinds of options:
> 
> 1. No argument
> 
>    { 'option': OPT-NAME }

This is newly allowed by your schema change.

> 
> 2. QemuOpts argument
> 
> 2a. with known parameters (QemuOptsList member desc[] not empty)
> 
>    { 'option': OPT-NAME,
>      'parameters': [ { 'name': PAR-NAME, 'type': PAR-TYPE }, ... ] }

Existing before your patch.

> 
> 2b. with unknown parameters (desc[] empty)
> 
>    { 'option': OPT-NAME, parameters: [] }

Existing before your patch.

> 
> 3. Other argument
> 
>    { 'option': OPT-NAME, ? }
> 
> This patch adds 3.  We need to decide what we want there.
> 
> Any particular reason why we have to invent something new, and can't
> simply fold it into 2b?

Or even into 1?  That is, the presence of 'parameters' is a witness of
whether a flag is boolean (-enable-fips) or takes arguments (-device);
then the length of the 'parameters' array is a witness of whether we
know all option arguments (modern code) or have unknown parameters
(older options that have not yet been converted to modern form).

> 
> Note that I completely left out help strings, both on the option and on
> the parameter level.  Both for brevity of presentation, and because I'd
> like to see a use before we add them.

Libvirt won't be hurt if you don't present help strings.

-- 
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: 604 bytes --]