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

[Amos Kong]

On Tue, Mar 11, 2014 at 10:04:56AM +0100, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
> > On 03/07/2014 02:54 AM, Markus Armbruster wrote:
> >> Eric Blake <address@hidden> 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' } }
 
How can we get this information? it's not good to rely on the help message.

And the parameters [] only have content when the option have a non-NULL desc
table, so we always just return a NULL parameters list, the 'implied-name'
information will be lost.

I thinks Markus's suggestion is fine, we can use tri-state (no-arg,
unsuecified-para-arg, no-para-arg).


Thanks, Amos

> 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?

-- 
                        Amos.