qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v4 2/2] query-command-line-options: query all th


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v4 2/2] query-command-line-options: query all the options in qemu-options.hx
Date: Thu, 06 Mar 2014 11:50:34 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux)

Amos Kong <address@hidden> writes:

> vm_config_groups[] only contains part of the options which have
> argument, and all options which have no argument aren't added
> to vm_config_groups[]. Current query-command-line-options only
> checks options from vm_config_groups[], so some options will
> be lost.
>
> We have macro in qemu-options.hx to generate a table that
> contains all the options. This patch tries to query options
> from the table.
>
> Then we won't lose the legacy options that weren't added to
> vm_config_groups[] (eg: -vnc, -smbios). The options that have
> no argument will also be returned (eg: -enable-fips)
>
> Some options that have argument have a NULL desc list, some
> options don't have argument, and "parameters" is mandatory
> in the past. So we add a new field "argument" to present
> if the option takes unspecified arguments.
>
> This patch also fixes options to match their actual command-line
> spelling rather than an alternate name associated with the
> option table in use by the command.
>
> Signed-off-by: Amos Kong <address@hidden>
> ---
>  qapi-schema.json   |  8 ++++++--
>  qemu-options.h     | 10 ++++++++++
>  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  vl.c               | 15 ---------------
>  4 files changed, 54 insertions(+), 23 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 193e7e4..4ab0d16 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4070,12 +4070,16 @@
>  #
>  # @option: option name
>  #
> -# @parameters: an array of @CommandLineParameterInfo
> +# @parameters: array of @CommandLineParameterInfo, possibly empty
> +# @argument: @optional present if the @parameters array is empty. If
> +#            true, then the option takes unspecified arguments, if
> +#            false, then the option is merely a boolean flag (since 2.0)

Suggest "if false, then the option takes no argument".

I'd call it something like 'unspecified-parameters' to emphasize the
connection with parameters.

>  #
>  # Since 1.5
>  ##
>  { 'type': 'CommandLineOptionInfo',
> -  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
> +  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
> +            '*argument': 'bool' } }
>  
>  ##
>  # @query-command-line-options:
> diff --git a/qemu-options.h b/qemu-options.h
> index 89a009e..8a515e0 100644
> --- a/qemu-options.h
> +++ b/qemu-options.h
> @@ -25,6 +25,8 @@
>   * THE SOFTWARE.
>   */
>  
> +#include "sysemu/arch_init.h"
> +

Why do you need this header?

If you need it, you should #include within the multiple-inclusion guard!

>  #ifndef _QEMU_OPTIONS_H_
>  #define _QEMU_OPTIONS_H_
>  
> @@ -33,4 +35,12 @@ enum {
>  #include "qemu-options-wrapper.h"
>  };
>  
> +typedef struct QEMUOption {
> +    const char *name;
> +    int flags;
> +    int index;
> +    uint32_t arch_mask;
> +} QEMUOption;
> +
> +extern const QEMUOption qemu_options[];
>  #endif
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index d2facfd..2f89b92 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -6,6 +6,16 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +#define HAS_ARG 0x0001
> +
> +const QEMUOption qemu_options[] = {
> +    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +    { NULL },
> +};
>  
>  static QemuOptsList *vm_config_groups[32];
>  static QemuOptsList *drive_config_groups[4];
> @@ -78,6 +88,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,15 +160,26 @@ 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)) {
> +    for (i = 0; qemu_options[i].name; i++) {
> +        if (!has_option || !strcmp(option, qemu_options[i].name)) {
>              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(qemu_options[i].name);
> +
> +            int idx = get_group_index(qemu_options[i].name);

Style nit: declaration after statement.  Suggest to declare it together
with i above, as "int i, idx;".

> +
> +            if (qemu_options[i].flags) {
> +                info->argument = true;
> +            }

The existing code using flags tests flags & HAS_ARG, see lookup_opt()).
Suggest to stick to that for consistency.

Please move the assigment next to the assignment to info->has_argument,
as shown below.

> +
> +            if (!strcmp("drive", qemu_options[i].name)) {
>                  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);
> +            }
> +
> +            if (!info->parameters) {
> +                info->has_argument = true;

                   info->argument = qemu_options[i].flags & HAS_ARG;

>              }
>              entry = g_malloc0(sizeof(*entry));
>              entry->value = info;
> diff --git a/vl.c b/vl.c
> index 685a7a9..6269762 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -111,7 +111,6 @@ int main(int argc, char **argv)
>  #include "trace/control.h"
>  #include "qemu/queue.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/arch_init.h"
>  #include "qemu/osdep.h"
>  
>  #include "ui/qemu-spice.h"
> @@ -1992,20 +1991,6 @@ static void help(int exitcode)
>  
>  #define HAS_ARG 0x0001
>  
> -typedef struct QEMUOption {
> -    const char *name;
> -    int flags;
> -    int index;
> -    uint32_t arch_mask;
> -} QEMUOption;
> -
> -static const QEMUOption qemu_options[] = {
> -    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> -#define QEMU_OPTIONS_GENERATE_OPTIONS
> -#include "qemu-options-wrapper.h"
> -    { NULL },
> -};
> -
>  static bool vga_available(void)
>  {
>      return object_class_by_name("VGA") || object_class_by_name("isa-vga");



reply via email to

[Prev in Thread] Current Thread [Next in Thread]