[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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;


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]