On Tue, Feb 11, 2014 at 01:03:05PM +0100, Markus Armbruster wrote:
> Amos Kong <akong@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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;
--
Amos.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list