Re: [Qemu-devel] [PCTCH v2 2/2] query-command-line-options: query all the options in qemu-options.hx

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3098 bytes --]

On 03/03/2014 10:41 AM, Amos Kong wrote:

s/PCTCH/PATCH/ in the subject, although it doesn't affect patch application.

> vm_config_groups[] only contains part of the options which have
> argument, and all options which have no argument isn't added to

s/isn't/aren't/

> vm_config_groups[]. Current query-command-line-options only
> checkes options from vm_config_groups[], so some options will

s/checkes/checks/

> be lost.
> 
> We have some macros 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 lost the legacy options that weren't added to

s/lost/lose/

> vm_config_groups[]. The options have no argument will also be

s/have/that have/

> 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 "arguments" to present
> if the option takes unspecified arguments.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json   |  8 ++++++--
>  util/qemu-config.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..0bd8e12 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3944,12 +3944,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)

I like it.

>  
> +#define HAS_ARG 0x0001
> +
> +typedef struct QEMUOption {
> +    const char *name;
> +    int flags;
> +    int index;
> +    uint32_t arch_mask;

Where is arch_mask used by this patch?

> +} QEMUOption;
> +
>  CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>                                                            const char *option,
>                                                            Error **errp)
> @@ -139,15 +161,33 @@ 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)) {
> +    static const QEMUOption qemu_options[] = {
> +        { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +        { NULL },
> +    };

This looks identical to what is already in vl.c.  Why do we need two
static tables with identical contents?  Can't you just export the
existing table and have it just once?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]