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'.
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@redhat.com> --- 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;