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: 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. 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? Yes. 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;