Eric Blake <eblake@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list