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

[Amos Kong]

On Tue, Mar 04, 2014 at 03:03:08PM -0700, Eric Blake wrote:

Hi Eric,

> 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 <address@hidden>
> > ---
> >  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
> $
 
{"return": [{"parameters": [{"name": "timestamp", "type": "boolean"}], 
"option": "msg"}... {"parameters": [], "option": "enable-fips", "argument": 
false}, ...

the output of query-command-line-options is one-line, it contains all
the options. I can find 'englab-fips' in the output.

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

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

(I was testing with latest qemu upstream + my patches, attached the
output file)

> > +++ 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?

I will define it in vl.c & qemu-config.c
 
> > +
> > +typedef struct QEMUOption {
> > +    const char *name;
> > +    int flags;
> > +    int index;
> > +    uint32_t arch_mask;
> > +} QEMUOption;

Keep this in qemu-options.h

> > +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.

I will implement it in qemu-config.c when I post V4, thanks
 
> > +    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) {

                     if flags == HAS_ARG == 0x1 ---> True

> > +                info->argument = true;

                    +#  If true, then the option takes unspecified arguments,

> > +            }

                 else { /// default case

                      +# if false, then the option is merely a boolean flag
                 }
> 
> ...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;

  //  # @argument: @optional present if the @parameters array is empty.

  If info->parameters is NULL, the array is empty, then @argument presents.

> >              }

                 else {
                     @argument won't present 
                     option has argument and array isn't empty
                 }

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

I can only confirm the issue of macro/table definition. Can you help
to re-check if something is wrong in your environment?

-- 
                        Amos.

query-command-line-options.output.txt
Description: Text document