Re: [Qemu-devel] [PATCH v3 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: 5388 bytes --]

On 03/03/2014 10:51 PM, Amos Kong wrote:
> 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 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 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 "arguments" to present

Here you call it "arguments", but in the code you call it "argument".

> if the option takes unspecified arguments.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json   |  8 ++++++--
>  qemu-options.h     | 18 ++++++++++++++++++
>  util/qemu-config.c | 35 +++++++++++++++++++++++++++++------
>  vl.c               | 17 -----------------
>  4 files changed, 53 insertions(+), 25 deletions(-)

Umm, did you test this?

$ printf %s\\n \
  '{"execute":"qmp_capabilities"}' \
  '{"execute":"query-command-line-options"}' \
  '{"execute":"quit"}' \
 | ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio | grep fips
$

I was expecting -enable-fips to appear somewhere in the output.
Something's not right, but I'm not going to figure out what.  Here's
hoping v4 actually gets it working.

> 
> 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)

For that matter, even this wasn't true.  In my testing, I see the same
thing as pre-patch for the -smbios option:

{"parameters": [], "option": "smbios"}

but the docs imply that I should now see:

{"parameters": [], "option": "smbios", "argument": true}

> +++ b/qemu-options.h
> @@ -25,6 +25,8 @@
>   * THE SOFTWARE.
>   */
>  
> +#include "sysemu/arch_init.h"
> +
>  #ifndef _QEMU_OPTIONS_H_
>  #define _QEMU_OPTIONS_H_
>  
> @@ -33,4 +35,20 @@ enum {
>  #include "qemu-options-wrapper.h"
>  };
>  
> +#define HAS_ARG 0x0001

Defining this non-namespace-friendly macro in a header seems risky.  Can
you localize its use, by using it only in the .c file that needs it,
and/or #undef it when done using it?

> +
> +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 },
> +};

Sticking a static array in a header is even worse than the v2 - now
every .c file that includes this .h has its own copy of the variable.
You really want the .h to just declare the variable as extern, then have
a single .c file actually implement it.

> +    for (i = 0; qemu_options[i].name; i++) {
> +        if (!has_option || !strcmp(option, qemu_options[i].name)) {
>              info = g_malloc0(sizeof(*info));

defaults info->has_argument to false and info->argument to false...

> -            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);
> +
> +            if (qemu_options[i].flags) {
> +                info->argument = true;
> +            }

...changes info->argument to true for options that take unspecified
arguments (such as -smbios), but with no effect to output unless...

> +
> +            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;
>              }

...this line gets executed.  I guess info->parameters is false for both
boolean options (where info->argument remains at its default of false)
and for unspecified arguments (where info->argument was set to true
above), while omitting the argument output for options that take named
options?  But while it looks okay in theory, the implementation was
still broken based on my testing, so I'm not sure what went wrong.

-- 
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 --]