| Message ID | 1390881230-14033-4-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
Amos Kong <akong@redhat.com> 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@redhat.com> > --- > 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;
On 01/27/2014 08:53 PM, Amos Kong wrote: > vm_config_groups[] contain the options which have parameter, but some > legcy options haven't been added to vm_config_groups[]. s/legcy/legacy/ > > 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 s/marcos/macros/ > 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[]. s/lost/lose/ > The options have no arguments will also be returned (eg: -enable-fips) s/options/options that/ > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > util/qemu-config.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) >
On Tue, Feb 11, 2014 at 01:03:05PM +0100, Markus Armbruster wrote: > Amos Kong <akong@redhat.com> 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@redhat.com> > > --- > > 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? Right, it's redundant work. > > + > > + 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. Ok > > + > > + 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? Schema change was wrongly included to patch 5/5. > 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" > } I touched an error in the past, so convert the has_arg to string. | In file included from util/qemu-config.c:160:0: | ./qemu-options.def: In function ‘qmp_query_command_line_options’: | ./qemu-options.def:10:16: error: ‘HAS_ARG’ undeclared (first use in this function) | DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ | ^ | ./qemu-options-wrapper.h:32:5: note: in definition of macro ‘DEF’ | opt_arg, | ^ | ./qemu-options.def:10:16: note: each undeclared identifier is reported only once for each function it appears in | DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ | ^ | ./qemu-options-wrapper.h:32:5: note: in definition of macro ‘DEF’ | opt_arg, | ^ This problem can be solved by defining HAS_ARG in qemu-config.c as in vl.c + #define HAS_ARG 0x0001 > Then you can simply test option_has_arg[i]. > > Or reuse vl.c's table. OK. > > + > > + 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;
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" + }; + + 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]); + + if (!strcmp("HAS_ARG", option_hasargs[i])) { + info->has_parameters = true; + } + + 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;
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@redhat.com> --- util/qemu-config.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)