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

[Amos Kong]

On Fri, Mar 07, 2014 at 10:54:09AM +0100, 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.
> >> 
> >> 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'?

We can use a enum state.

|  { 'enum': 'ArgumentStateType',
|    'data': ['no-argument', 'unspecified-parameters-argument',
|             'non-parameter-argument']
|  }
|  
|  { '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?

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?
 
>    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
 
> >> 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.
> 
> 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".
> 
> Do we care?
> 
> > This is a bug fix patch, so let's shoot to get it into 2.0.
> 
> Yes.
> 
> >> Signed-off-by: Amos Kong <address@hidden>
> >> ---
> >>  qapi-schema.json   |  8 ++++++--
> >>  qemu-options.h     | 10 ++++++++++
> >>  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
> >>  vl.c               | 15 ---------------
> >>  4 files changed, 54 insertions(+), 23 deletions(-)
> >
> >> 
> >> +++ b/util/qemu-config.c
> >> @@ -6,6 +6,16 @@
> >>  #include "hw/qdev.h"
> >>  #include "qapi/error.h"
> >>  #include "qmp-commands.h"
> >> +#include "qemu-options.h"
> >> +
> >> +#define HAS_ARG 0x0001
> >
> > Hmm, we are now duplicating this macro between here and vl.c.  I'd
> > prefer it gets hoisted into the .h file, so that it doesn't get out of
> > sync between the two clients.
> 
> Flag values should be defined next to the flags type or variable, in
> this case next to QEMUOption.  The patch hoists QEMUOption vl.c to
> qemu-options.h.
> 
> It does that because it spreads option handling to another file.
> Before, it's all in vl.c.  After, qemu_options[] and
> qmp_query_command_line_options() are in qemu-config.c, and lookup_opts()
> stays in vl.c.  Not thrilled by that.  Can we keep it in one place?
> 
> qemu-config.c isn't about the command line.  I suspect
> qmp_query_command_line_options() ended up there just because its
> problematic design choice *not* to work on command line options, but on
> vm_config_groups[].  This series fixes that design choice.
> 
> We can't simply move the new qmp_query_command_line_options() out,
> because it still uses vm_config_groups[], which is static.  I take that
> as a sign of us not having gotten the interfaces quite right, yet.
> 
> I append a quick sketch.  It needs a bit more work to fully separate
> qmp_query_command_line_options() and the helpers dealing with
> CommandLineParameterInfoList from the QemuOpts code.
> 
> Since 2.0 is closing in fast, I won't insist on clean separation now.
> We can do that on top.
> 
> 
> 
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 2f89b92..45f48da 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -49,7 +49,7 @@ QemuOptsList *qemu_find_opts(const char *group)
>      return ret;
>  }
>  
> -static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc 
> *desc)
> +static CommandLineParameterInfoList *get_param_info(const QemuOptDesc *desc)
>  {
>      CommandLineParameterInfoList *param_list = NULL, *entry;
>      CommandLineParameterInfo *info;
> @@ -88,17 +88,6 @@ static CommandLineParameterInfoList 
> *get_param_infolist(const QemuOptDesc *desc)
>      return param_list;
>  }
>  
> -static int get_group_index(const char *name)
> -{
> -    int i;
> -
> -    for (i = 0; vm_config_groups[i] != NULL; i++) {
> -        if (!strcmp(vm_config_groups[i]->name, name)) {
> -            return i;
> -        }
> -    }
> -    return -1;
> -}
>  /* remove repeated entry from the info list */
>  static void cleanup_infolist(CommandLineParameterInfoList *head)
>  {
> @@ -134,16 +123,16 @@ static void 
> connect_infolist(CommandLineParameterInfoList *head,
>  }
>  
>  /* access all the local QemuOptsLists for drive option */
> -static CommandLineParameterInfoList *get_drive_infolist(void)
> +static CommandLineParameterInfoList *get_drive_param_info(void)
>  {
>      CommandLineParameterInfoList *head = NULL, *cur;
>      int i;
>  
>      for (i = 0; drive_config_groups[i] != NULL; i++) {
>          if (!head) {
> -            head = get_param_infolist(drive_config_groups[i]->desc);
> +            head = get_param_info(drive_config_groups[i]->desc);
>          } else {
> -            cur = get_param_infolist(drive_config_groups[i]->desc);
> +            cur = get_param_info(drive_config_groups[i]->desc);
>              connect_infolist(head, cur);
>          }
>      }
> @@ -159,28 +148,25 @@ CommandLineOptionInfoList 
> *qmp_query_command_line_options(bool has_option,
>      CommandLineOptionInfoList *conf_list = NULL, *entry;
>      CommandLineOptionInfo *info;
>      int i;
> +    QemuOptsList *list;
>  
>      for (i = 0; qemu_options[i].name; i++) {
>          if (!has_option || !strcmp(option, qemu_options[i].name)) {
>              info = g_malloc0(sizeof(*info));
>              info->option = g_strdup(qemu_options[i].name);
>  
> -            int idx = get_group_index(qemu_options[i].name);
> -
> -            if (qemu_options[i].flags) {
> -                info->argument = true;
> -            }
> -
>              if (!strcmp("drive", qemu_options[i].name)) {
> -                info->parameters = get_drive_infolist();
> -            } else if (idx >= 0) {
> -                info->parameters =
> -                    get_param_infolist(vm_config_groups[idx]->desc);
> +                info->parameters = get_drive_param_info();
> +            } else {
> +                list = qemu_find_opts_err(qemu_options[i].name, NULL);
> +                info->parameters = list ? get_param_info(list->desc) : NULL;
>              }
>  
>              if (!info->parameters) {
>                  info->has_argument = true;
> +                info->argument = qemu_options[i].flags;
>              }
> +
>              entry = g_malloc0(sizeof(*entry));
>              entry->value = info;
>              entry->next = conf_list;