11:51 p.m.
This patchset fixed some issues of query-command-line-options:
* some new options that haven't argument can't be queried. (eg: -enable-fips)
* some legacy options that have argument can't be queried. (eg: -vnc display)
More discussion:
http://marc.info/?l=qemu-devel&m=139081830416684&w=2
V2: remove duplicate option tables, update schema
V3: fix typo in commitlog and export qemu_options talbe
Amos Kong (2):
qmp: rename query_option_descs() to get_param_infolist()
query-command-line-options: query all the options in qemu-options.hx
qapi-schema.json | 8 ++++++--
qemu-options.h | 18 ++++++++++++++++++
util/qemu-config.c | 41 ++++++++++++++++++++++++++++++++---------
vl.c | 17 -----------------
4 files changed, 56 insertions(+), 28 deletions(-)
--
1.8.5.3
11:51 p.m.
New subject: [libvirt] [PATCH v3 1/2] qmp: rename query_option_descs() to get_param_infolist()
Signed-off-by: Amos Kong <akong(a)redhat.com>
---
util/qemu-config.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 9298f55..d624d92 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -39,7 +39,7 @@ QemuOptsList *qemu_find_opts(const char *group)
return ret;
}
-static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
+static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc *desc)
{
CommandLineParameterInfoList *param_list = NULL, *entry;
CommandLineParameterInfo *info;
@@ -120,9 +120,9 @@ static CommandLineParameterInfoList *get_drive_infolist(void)
for (i = 0; drive_config_groups[i] != NULL; i++) {
if (!head) {
- head = query_option_descs(drive_config_groups[i]->desc);
+ head = get_param_infolist(drive_config_groups[i]->desc);
} else {
- cur = query_option_descs(drive_config_groups[i]->desc);
+ cur = get_param_infolist(drive_config_groups[i]->desc);
connect_infolist(head, cur);
}
}
@@ -147,7 +147,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool
has_option,
info->parameters = get_drive_infolist();
} else {
info->parameters =
- query_option_descs(vm_config_groups[i]->desc);
+ get_param_infolist(vm_config_groups[i]->desc);
}
entry = g_malloc0(sizeof(*entry));
entry->value = info;
--
1.8.5.3
3:17 p.m.
New subject: [libvirt] [PATCH v3 1/2] qmp: rename query_option_descs() to get_param_infolist()
On 03/03/2014 10:51 PM, Amos Kong wrote:
Signed-off-by: Amos Kong <akong(a)redhat.com>
---
util/qemu-config.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
No change from v2. To ease my review time, it is reasonable if you
amend the commit message to include my:
Reviewed-by: Eric Blake <eblake(a)redhat.com>
line, that way I know I don't have to focus heavily on a second review
of the same content. (http://wiki.qemu.org/Contribute/SubmitAPatch has
more hints about effective use of R-b tags)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:51 p.m.
New subject: [libvirt] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx
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(a)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(-)
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");
--
1.8.5.3
4:03 p.m.
New subject: [libvirt] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx
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(a)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
12:40 a.m.
New subject: [libvirt] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx
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(a)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?
--
Amos.
9:58 a.m.
New subject: [libvirt] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:50 p.m.
New subject: [libvirt] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:32 p.m.
New subject: [libvirt] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx
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.
--
Amos.
3914
days inactive
3916
days old
8 comments
2 participants
participants (2)
-
Amos Kong -
Eric Blake