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

[Amos Kong <akong@redhat.com>]

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;

-- 
			Amos.