Eric Blake <eblake@xxxxxxxxxx> writes:
> On 03/07/2014 02:54 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@xxxxxxxxxx> writes:
>>
>>> On 03/05/2014 07:36 PM, Amos Kong wrote:
>>>> vm_config_groups[] only contains part of the options which have
>>>> argument, and all options which have no argument aren't added
>>>> to vm_config_groups[]. Current query-command-line-options only
>>>> checks options from vm_config_groups[], so some options will
>>>> be lost.
>>>>
>
>> Example: -device takes unspecified parameters. -cdrom doesn't take
>> parameters, it takes a file name. Yet, the command reports the same
>> for both: "parameters": [], "argument": true.
>>
>> Looks like we need a tri-state: option takes no argument, QemuOpts
>> argument, or other argument.
>
> I don't buy that. '-cdrom filename' could easily be re-written [in a
> future qemu version] to use QemuOpts with an implied parameter name
> (we've done that elsewhere, such as for '-machine'). In other words, I
> think we could make it become shorthand for '-cdrom file=filename', at
> which point the QemuOpts spelling is available and would now show up as
> "parameters":[{"name":"file"...}]. Thus, in converting -cdrom to
> QemuOpts, we can still maintain command-line back-compat, while making
> the query-command-line-options output more featureful. In other words,
> _for now_ it takes unspecified parameters, and the fact that it is only
> a single parameter in the form 'filename' rather than a more typical
> parameter 'file=filename' is not a show-stopper.
Incompatible change for funny filenames: -cdrom you,break=me.
Besides breaking funny filenames, we'd also buy ourselves some stupid
-readconfig / -writeconfig trouble. Let me explain.
-cdrom F is effectively sugar for "-drive media=cdrom,index=2,file=FF",
where FF is F with comma doubled.
-writeconfig writes out desugared QemuOpts. Therefore, "-cdrom r7.iso"
gets written as
[drive]
media = "cdrom"
index = "2"
file = "r7.iso"
which -readconfig can read.
If we convert -cdrom to QemuOpts, it gets written out like this:
[cdrom]
file = "r7.iso"
If we continue to desugar it, it'll *also* get written out as before.
Either we *delete* the sugared QemuOpts to avoid duplication, or we
*stop* desugaring. The latter breaks -readconfig of existing
configuration files, and complicates the code reading configuration from
QemuOpts.
I don't think any of the old non-QemuOpts options that have become sugar
for newer, more flexible QemuOpts options should be converted to
QemuOpts.
> So your idea of a tri-state (QemuOpts, no argument, or other argument)
> doesn't add anything - any option that takes "other argument" could be
> converted to take QemuOpts, and from the command line, we can't tell the
> difference from whether something was implemented by QemuOpts, only by
> whether we have introspection on what the argument consists of.
I doubt we can convert all existing options to QemuOpts without breaking
backward compatibility and complicating the code.
> Meanwhile, it DOES point out that our use of implicit argument in
> QemuOpts ought to be exposed to the introspection mechanism, for
> introspection to be fully descriptive. That is, maybe we should modify
> our introspection to add a new 'implied-name':
>
> ##
> # @CommandLineParameterInfo:
> #
> ...
> # @implied-name: #optional, if present and true, the parameter can be
> # specified as '-option value' instead of the preferred
> # spelling of '-option name=value' (since 2.0)
> # Since 1.5
> { 'type': 'CommandLineParameterInfo',
> 'data': { 'name': 'str',
> 'type': 'CommandLineParameterType',
> '*help': 'str', '*implied-name': 'bool' } }
The only use for implied-name I can think of is interpreting a user's
command line. Is that a real use case?
>>
>> parameters is [] unless it's a QemuOpts argument. Then it lists the
>> recognized parameters.
>
> This part is still true. When parameters[] is non-empty, it is a
> QemuOpts and we know all recognized parameters (well, more precisely,
> the subset of QemuOpts that were explicitly called out - given your
> point 2 about the mess of -drive); when it is empty, then all we know is
> whether the argument is a boolean or takes unspecified arguments (where
> the conversion of those unknown arguments to QemuOpts will be what
> finally lets us introspect the format of those unknown arguments).
QemuOpts argument with only unspecified parameters is not the same as
non-QemuOpts argument. I don't think conflating the two is useful.
>> 2. Our dear friend -drive is more complicated than you might think
>>
>> We special-case it to report the union of drive_config_groups[],
>> which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
>> qemu_drive_opts. The latter accepts unspecified parameters.
>>
>> I believe qemu_drive_opts is actually not used by the (complex!) code
>> parsing the argument of -drive.
>>
>> Nevertheless, said code accepts more than just qemu_legacy_drive_opts
>> and qemu_common_drive_opts, namely driver-specific parameters.
>>
>> Until we define those properly in a schema, I guess the best we can
>> do is add one more case: option takes QemuOpts argument, but
>> parameters is not exhaustive.
>
> We already know 'query-command-line-options' is not a full introspection
> yet. So far, libvirt has managed to get by on partial information (in
> fact, the whole hack for special-casing -drive to merge multiple lists
> together was precisely to avoid a regression with at least providing the
> partial information that libvirt was actually using). Documenting that
> QemuOpts information may be incomplete may be nice, but shouldn't hold
> up the initial purpose of this patch which is to document non-QemuOpts
> options. And knowing that an option takes unspecified arguments is
> still better than not knowing about the option at all.
If all we want is a quick fix for "I can't see whether -frobnicate is
supported", then let's add a command to dump qemu_options[], and leave
query-command-line-options broken as designed.
But if we want proper command line introspection, then let's do it
properly: no quick hacks, no half-truths.
I can't get contents right and do backward compatibility acrobatics at
the same time. I need to come up with the data to convey first, and a
way to shoehorn it into the existing command second. *If* we choose to
shoehorn rather than deprecate & replace.
>>>> This patch also fixes options to match their actual command-line
>>>> spelling rather than an alternate name associated with the
>>>> option table in use by the command.
>>>
>>> Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
>>> from "acpi" to "acpitable" to match the command line option? Same for
>>> vl.c and qemu_boot_opts from "boot-opts" to "boot"? Same for vl.c and
>>> qemu_smp_opts from "smp-opts" to "smp"? Those were the obvious
>>> mismatches I found where the command line was spelled differently than
>>> the vm_config_groups entry.
>>
>> Without such a change, the command lies, because it fails to connect the
>> option to its QemuOptsList. Example:
>>
>> {"parameters": [], "option": "acpitable", "argument": true},
>>
>> However, the vm_config_groups[].name values are ABI: they're the section
>> names recognized by -readconfig and produced by -writeconfig. Thus,
>> this is an incompatible change. It's also an improvement of sorts:
>> things become more consistent.
>
> Ouch. I did not realize they were ABI. 'query-command-line-options'
> should expose the command line spelling, but maybe that argues that we
> need to enhance our QAPI introspection to make it easier to document the
> special cases:
>
> ##
> # @CommandLineOptionInfo:
> ...
> # @config-name: #optional if present, the command line spelling differs
> # from the name used by -readconfig (since 2.0)
> # Since 1.5
> ##
> { 'type': 'CommandLineOptionInfo',
> 'data': { 'option': 'str', '*config-name':'str',
> 'parameters': ['CommandLineParameterInfo'] } }
>
> and where we would expose:
>
> {"parameters": [], "option": "acpitable", "config-name": "acpi",
> "argument": true},
>
> or even combining my above suggestions:
>
> {"option":"M", "parameters":[], "config-name":"machine",
> "argument": true},
> {"option":"machine", "parameters":[
> {"name": "firmware", "help": "firmware image", "type": "string"},
> {"name": "type", "implied-name": true, "help": "emulated machine",
> "type": "string"}, ...]},
>
> to make it a bit more obvious that '-M str' and '-machine str' are both
> shorthands for the preferred '-machine type=str', and that the same
> effect is reached via a config file that has a [machine] section.
Use case for the introspection into the desugaring of -M?
Can't cover less trivial desugarings, like -cdrom.
We got more sugar than a jelly doughnut with radioactive pink frosting!
>> We could avoid it with a suitable mapping from option name to option
>> group name. Simplest way to do that is store only the exceptions from
>> the rule "the names are the same".
>>
>
> Yes. We've identified at least 3 exceptions now (acpitable, boot, smp),
> and exposing those exceptions in the introspection is a good idea, to
> make us quit adding new ones.
It'll make us quit adding new ones only if we can come up with a test
that breaks when we add new ones :)
>> Do we care?
>>
>>> This is a bug fix patch, so let's shoot to get it into 2.0.
>>
>> Yes.
>
> How much work are we able to do before hard freeze? How much work are we
> willing to accept as bug fix after hard freeze?
I don't know.
Is better command line introspection in 2.0 worth the risk that comes
with softening up the hard freeze?
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list