| Message ID | 1393912317-26221-3-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 <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 > $ {"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?
On 03/04/2014 11:40 PM, Amos Kong wrote: >> 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) Hmm, maybe I was testing a stale binary. Let me try re-running tests on my end - I recently changed my choice of configure arguments to speed up build time by building fewer binaries, so maybe I tested on a binary that my configure arguments no longer rebuild.
On 03/05/2014 08:58 AM, Eric Blake wrote: > On 03/04/2014 11:40 PM, Amos Kong wrote: > >>> 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) > > Hmm, maybe I was testing a stale binary. Let me try re-running tests on > my end - I recently changed my choice of configure arguments to speed up > build time by building fewer binaries, so maybe I tested on a binary > that my configure arguments no longer rebuild. Aha, it WAS my configure options at fault. Apologies for the mis-test on my side. I can now confirm that pre-patch, I see (rearranged a subset of entries, and newlines added for legibility): {"parameters": [], "option": "smbios"}, {"parameters": [{"name": "file", "type": "string"}, {"name": "events", "type": "string"}], "option": "trace"}, and no fips, while post-patch, I see: {"parameters": [], "option": "enable-fips", "argument": false}, {"parameters": [], "option": "smbios", "argument": true}, {"parameters": [{"name": "file", "type": "string"}, {"name": "events", "type": "string"}], "option": "trace"}, which matches the docs. However, I did notice that pre-patch, I see: {"parameters": [], "option": "acpi"} while post-patch, there is no hit for "acpi", but there is a new: {"parameters": [], "option": "acpitable", "argument": true} What's going on there? It is a potential regression if we fail to list an option post-patch that was listed pre-patch. Then again, looking at the actual -help text, I see my particular qemu binary mentions only "-acpitable [sig=str]..." in the help text (pre- and post-patch), as well as this test to prove there is no '-acpi': $ ./x86_64-softmmu/qemu-system-x86_64 -acpi qemu-system-x86_64: -acpi: invalid option $ ./x86_64-softmmu/qemu-system-x86_64 -acpitable qemu-system-x86_64: -acpitable: requires an argument so it looks like your patch was silently fixing a bug. Indeed, reading vl.c, I see: case QEMU_OPTION_acpitable: opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1); if (!opts) { exit(1); } do_acpitable_option(opts); so the option table named "acpi" is indeed for the command line argument "acpitable". It would be nice to mention bonus bug fixes like that in the commit message (that is, you are not only adding support for flags like -enable-fips, you are also fixing options to match their actual command-line spelling rather than an alternate name associated with the option table in use by the command). So, at this point, we still need a v4 to avoid the duplicate static tables, but I am now set up to give a Tested-by.
On Wed, Mar 05, 2014 at 11:50:22AM -0700, Eric Blake wrote: > On 03/05/2014 08:58 AM, Eric Blake wrote: > > On 03/04/2014 11:40 PM, Amos Kong wrote: > > > >>> 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) > > > > Hmm, maybe I was testing a stale binary. Let me try re-running tests on > > my end - I recently changed my choice of configure arguments to speed up > > build time by building fewer binaries, so maybe I tested on a binary > > that my configure arguments no longer rebuild. > > Aha, it WAS my configure options at fault. Apologies for the mis-test > on my side. I can now confirm that pre-patch, I see (rearranged a > subset of entries, and newlines added for legibility): > > {"parameters": [], "option": "smbios"}, > {"parameters": [{"name": "file", "type": "string"}, > {"name": "events", "type": "string"}], "option": "trace"}, > > and no fips, while post-patch, I see: > > {"parameters": [], "option": "enable-fips", "argument": false}, > {"parameters": [], "option": "smbios", "argument": true}, > {"parameters": [{"name": "file", "type": "string"}, > {"name": "events", "type": "string"}], "option": "trace"}, > > which matches the docs. However, I did notice that pre-patch, I see: > > {"parameters": [], "option": "acpi"} > > while post-patch, there is no hit for "acpi", but there is a new: > > {"parameters": [], "option": "acpitable", "argument": true} > > What's going on there? It is a potential regression if we fail to list > an option post-patch that was listed pre-patch. Then again, looking at > the actual -help text, I see my particular qemu binary mentions only > "-acpitable [sig=str]..." in the help text (pre- and post-patch), as > well as this test to prove there is no '-acpi': > > $ ./x86_64-softmmu/qemu-system-x86_64 -acpi > qemu-system-x86_64: -acpi: invalid option > $ ./x86_64-softmmu/qemu-system-x86_64 -acpitable > qemu-system-x86_64: -acpitable: requires an argument > > so it looks like your patch was silently fixing a bug. Indeed, reading > vl.c, I see: > > case QEMU_OPTION_acpitable: > opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1); > if (!opts) { > exit(1); > } > do_acpitable_option(opts); > > so the option table named "acpi" is indeed for the command line argument > "acpitable". Can we update all the name in option tables to match with actual command-line spelling? (we can use another patch to fix it) > It would be nice to mention bonus bug fixes like that in the commit > message (that is, you are not only adding support for flags like > -enable-fips, you are also fixing options to match their actual > command-line spelling rather than an alternate name associated with the > option table in use by the command). > > So, at this point, we still need a v4 to avoid the duplicate static > tables, but I am now set up to give a Tested-by. Thanks for your confirm, I will post a V4.
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) # # 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..f729965 100644 --- a/qemu-options.h +++ 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 + +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 }, +}; + #endif diff --git a/util/qemu-config.c b/util/qemu-config.c index d624d92..b82ac52 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -6,6 +6,7 @@ #include "hw/qdev.h" #include "qapi/error.h" #include "qmp-commands.h" +#include "qemu-options.h" static QemuOptsList *vm_config_groups[32]; static QemuOptsList *drive_config_groups[4]; @@ -78,6 +79,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 +151,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); + + if (qemu_options[i].flags) { + info->argument = true; + } + + 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; } entry = g_malloc0(sizeof(*entry)); entry->value = info; diff --git a/vl.c b/vl.c index 7f4fe0d..b8c674a 100644 --- a/vl.c +++ b/vl.c @@ -165,7 +165,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" @@ -2046,22 +2045,6 @@ static void help(int exitcode) exit(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");
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 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(-)