[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/2] query-command-line-options: query all th
From: |
Amos Kong |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx |
Date: |
Wed, 5 Mar 2014 14:40:14 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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