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/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'.

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.

parameters is [] unless it's a QemuOpts argument. Then it lists the recognized parameters.

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.

>> 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 <akong(a)redhat.com&gt; >> --- >> 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;