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

[Amos Kong <akong@redhat.com>]

On Wed, Mar 26, 2014 at 02:15:18PM +0100, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
> > On Fri, Mar 07, 2014 at 10:54:09AM +0100, Markus Armbruster wrote:
> >> Eric Blake <eblake@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.
> >> >> 
> >> >> We have macro in qemu-options.hx to generate a table that
> >> >> contains all the options. This patch tries to query options
> >> >> from the table.
> >> >> 
> >> >> Then we won't lose the legacy options that weren't added to
> >> >> vm_config_groups[] (eg: -vnc, -smbios). The options that have
> >> >> no argument will also be returned (eg: -enable-fips)
> >> >> 
> >> >> Some options that have argument have a NULL desc list, some
> >> >> options don't have argument, and "parameters" is mandatory
> >> >> in the past. So we add a new field "argument" to present
> >> >> if the option takes unspecified arguments.
> >> >
> >> > I like Markus' suggestion of naming the new field
> >> > 'unspecified-parameters' rather than 'argument'.
> >  
> > Hi Markus,
> >
> >> Looking again, there are more problems.
> >> 
> >> 1. Non-parameter argument vs. parameter argument taking unspecified
> >>    parameters
> >> 
> >>    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.
> >
> > '-cdrom' is the 'other argument' == 'Non-parameter argument'?
> 
> Let me clarify my terminology:
> 
> * A "parameter argument" is an option argument of the form KEY=VALUE,...
>   Since parameter arguments should always be parsed with QemuOpts[*], I
>   use the term "QemuOpts argument" interchangeably.
> 
> * A "non-parameter argument" or "other argument" is an option argument
>   that doesn't use this form.
> 
> Does that answer your question?

Got it, thanks.
 
> > We can use a enum state.
> 
> I'm not sure I got that.
> 
> > |  { 'enum': 'ArgumentStateType',
> > |    'data': ['no-argument', 'unspecified-parameters-argument',
> > |             'non-parameter-argument']
> > |  }


        {'enum': 'ArgumentStateType',
         'data': ['no-argument', 'qemuopts-argument', 'non-param-argument']
        }

     no-argument:         -enable-kvm
     qemuopts-argument:   -boot order=c,menu=on
     non-param-argument:  -cdrom file


     I don't know if it's the tri-state you suggested in previous reply.

> > |  
> > |  { 'type': 'CommandLineOptionInfo',
> > |    'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
> > |              '*argument-state': 'ArgumentStateType' } }
> >  
> >>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
> >>    recognized parameters.
> >
> > How about balloon? we should treat as 'taking unspecified parameters'?
> >
> >     "-balloon none   disable balloon device\n"
> >     "-balloon virtio[,addr=str]\n"
> >
> > I think we can only check this from help message in qemu-options.hx,
> > is it a stable/acceptable method?
> 
> -balloon is yet another sugar option:
> 
> * "-balloon none" does nothing.  It could suppress a default balloon
>   device, if such a thing existed.
> 
> * "-balloon virtio,KEY=VALUE..." is sugar for "-device
>   virtio-balloon,KEY=VALUE...".  Keys other than "addr" are
>   undocumented.
> 
> The actual option argument parsing is ad hoc, not QemuOpts.
> 
> I sure hope something like this would not pass review today.
> 
> My advice to tools introspecting the command line is to avoid sugared
> options, unless the desugaring encapsulates something they don't want to
> know.
> 
> > We introduce query-command-line-options command to avoid libvirt to
> > check qemu commandline information by scanning qemu's help message,
> > it's not strict & stable.
> >  
> >> 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'm confused here. But there is only one option (-drive), we should
> > return merged desc table (legacy + common).
> >
> > Desc table of qemu_drive_opts is NULL, then -drive can also take
> > unspecified parameters?
> 
> Yes: driver-specific parameters.
> 
> -drive takes currently takes unspecified parameters (the driver-specific
> parameters) in addition to a number of specified parameters (the common
> and legacy 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.
> >
> >
> > Thanks, Amos
> 
> 
> [*] Leftovers still parsed by other means, if any, should be converted
> to QemuOpts.

-- 
			Amos.