Re: [libvirt] [Qemu-devel] [PATCH 3/5] query-command-line-options: query all the options in qemu-options.hx

Amos Kong <akong(a)redhat.com&gt; writes: > vm_config_groups[] contain the options which have parameter, but some > legcy options haven't been added to vm_config_groups[]. > > All the options can be found in qemu-options.hx, this patch used two > new marcos to generate two tables, we can check if the option name is > valid and if the option has arguments. > > This patch also try to visit all the options in option_names[], then > we won't lost the legacy options that weren't added to vm_config_groups[]. > The options have no arguments will also be returned (eg: -enable-fips) > > Signed-off-by: Amos Kong <akong(a)redhat.com&gt; > --- > util/qemu-config.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/util/qemu-config.c b/util/qemu-config.c > index d624d92..de233d8 100644 > --- a/util/qemu-config.c > +++ b/util/qemu-config.c > @@ -78,6 +78,17 @@ 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) > { > @@ -139,16 +150,34 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option, > CommandLineOptionInfo *info; > int i; > > - for (i = 0; vm_config_groups[i] != NULL; i++) { > - if (!has_option || !strcmp(option, vm_config_groups[i]->name)) { > + char const *option_names[] = { > +#define QEMU_OPTIONS_GENERATE_NAME > +#include "qemu-options-wrapper.h" > + }; > + > + char const *option_hasargs[] = { > +#define QEMU_OPTIONS_GENERATE_HASARG > +#include "qemu-options-wrapper.h" > + }; Both tables are technically redundant. The same information is already in vl.c's qemu_options[]. That one also includes -h, which the tables here miss. Duplicating tables can be okay, but I suspect using the existing one would be simpler. Have you tried?

> + > + for (i = 0; i < sizeof(option_names) / sizeof(char *); i++) { > + if (!has_option || !strcmp(option, option_names[i])) { > info = g_malloc0(sizeof(*info)); > - info->option = g_strdup(vm_config_groups[i]->name); > - if (!strcmp("drive", vm_config_groups[i]->name)) { > + info->option = g_strdup(option_names[i]); > + > + int idx = get_group_index(option_names[i]); Variable declaration follows statement. Please declare int idx at the beginning of a block. I'd declare it right at the beginning, together with int i.

> + > + if (!strcmp("HAS_ARG", option_hasargs[i])) { > + info->has_parameters = true; > + } qemu/util/qemu-config.c:171:21: error: ‘CommandLineOptionInfo’ has no member named ‘has_parameters’ Did you forget to include a schema change?

Awfully roundabout way to test whether the option takes an argument. Here's the other part, from PATCH 2/5: +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \ + stringify(opt_arg), Please do it more like vl.c: #define HAS_ARG true bool option_has_arg[] = { #define QEMU_OPTIONS_GENERATE_HASARG #include "qemu-options-wrapper.h" }

Then you can simply test option_has_arg[i]. Or reuse vl.c's table.

> + > + if (!strcmp("drive", option_names[i])) { > info->parameters = get_drive_infolist(); > - } else { > + } else if (idx >= 0) { > info->parameters = > - get_param_infolist(vm_config_groups[i]->desc); > + get_param_infolist(vm_config_groups[idx]->desc); > } > + > entry = g_malloc0(sizeof(*entry)); > entry->value = info; > entry->next = conf_list;