Re: [libvirt] [Qemu-devel] [PATCH v4 2/2] query-command-line-options: query all the options in qemu-options.hx

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