4:04 a.m.
Eric Blake <eblake(a)redhat.com> writes:
On 03/07/2014 02:54 AM, Markus Armbruster wrote:
> Eric Blake <eblake(a)redhat.com> 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?