[Qemu-devel] [PATCH v3 0/2] fix query-command-line-options

[Qemu-devel] [PATCH v3 0/2] fix query-command-line-options

[Amos Kong]

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

[Qemu-devel] [PATCH v3 1/2] qmp: rename query_option_descs() to get_param_infolist()

[Amos Kong]

Signed-off-by: Amos Kong <akong@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

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

[Amos Kong]

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

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

Re: [Qemu-devel] [PATCH v3 1/2] qmp: rename query_option_descs() to get_param_infolist()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On 03/03/2014 10:51 PM, Amos Kong wrote:
> Signed-off-by: Amos Kong <akong@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@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

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

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

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 6972 bytes --]

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?

-- 
			Amos.

[-- Attachment #2: query-command-line-options.output.txt --]
[-- Type: text/plain, Size: 16513 bytes --]

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 7, "major": 1}, "package": ""}, "capabilities": []}}
{"return": {}}
{"return": [{"parameters": [{"name": "timestamp", "type": "boolean"}], "option": "msg"}, {"parameters": [], "option": "object", "argument": true}, {"parameters": [], "option": "tdf", "argument": false}, {"parameters": [], "option": "no-kvm-irqchip", "argument": false}, {"parameters": [], "option": "no-kvm-pit", "argument": false}, {"parameters": [], "option": "no-kvm-pit-reinjection", "argument": false}, {"parameters": [], "option": "no-kvm", "argument": false}, {"parameters": [], "option": "enable-fips", "argument": false}, {"parameters": [], "option": "qtest-log", "argument": true}, {"parameters": [], "option": "qtest", "argument": true}, {"parameters": [{"name": "file", "type": "string"}, {"name": "events", "type": "string"}], "option": "trace"}, {"parameters": [], "option": "no-user-config", "argument": false}, {"parameters": [], "option": "nodefconfig", "argument": false}, {"parameters": [], "option": "writeconfig", "argument": true}, {"parameters": [], "option": "readconfig", "argument": true}, {"parameters": [{"name": "enable", "type": "boolean"}], "option": "sandbox"}, {"parameters": [], "option": "old-param", "argument": false}, {"parameters": [], "option": "semihosting", "argument": false}, {"parameters": [], "option": "prom-env", "argument": true}, {"parameters": [], "option": "runas", "argument": true}, {"parameters": [], "option": "chroot", "argument": true}, {"parameters": [], "option": "nodefaults", "argument": false}, {"parameters": [], "option": "incoming", "argument": true}, {"parameters": [], "option": "tb-size", "argument": true}, {"parameters": [], "option": "show-cursor", "argument": false}, {"parameters": [], "option": "virtioconsole", "argument": true}, {"parameters": [], "option": "echr", "argument": true}, {"parameters": [], "option": "watchdog-action", "argument": true}, {"parameters": [], "option": "watchdog", "argument": true}, {"parameters": [], "option": "icount", "argument": true}, {"parameters": [{"name": "driftfix", "type": "string"}, {"name": "clock", "type": "string"}, {"name": "base", "type": "string"}], "option": "rtc"}, {"parameters": [], "option": "startdate", "argument": true}, {"parameters": [], "option": "localtime", "argument": false}, {"parameters": [], "option": "clock", "argument": true}, {"parameters": [{"name": "romfile", "type": "string"}, {"name": "bootindex", "type": "number"}], "option": "option-rom"}, {"parameters": [], "option": "daemonize", "argument": false}, {"parameters": [], "option": "loadvm", "argument": true}, {"parameters": [], "option": "no-shutdown", "argument": false}, {"parameters": [], "option": "no-reboot", "argument": false}, {"parameters": [], "option": "xen-attach", "argument": false}, {"parameters": [], "option": "xen-create", "argument": false}, {"parameters": [], "option": "xen-domid", "argument": true}, {"parameters": [], "option": "enable-kvm", "argument": false}, {"parameters": [], "option": "bios", "argument": true}, {"parameters": [], "option": "L", "argument": true}, {"parameters": [], "option": "D", "argument": true}, {"parameters": [], "option": "d", "argument": true}, {"parameters": [], "option": "s", "argument": false}, {"parameters": [], "option": "gdb", "argument": true}, {"parameters": [{"name": "mlock", "type": "boolean"}], "option": "realtime"}, {"parameters": [], "option": "S", "argument": false}, {"parameters": [], "option": "singlestep", "argument": false}, {"parameters": [], "option": "pidfile", "argument": true}, {"parameters": [], "option": "debugcon", "argument": true}, {"parameters": [{"name": "pretty", "type": "boolean"}, {"name": "default", "type": "boolean"}, {"name": "chardev", "type": "string"}, {"name": "mode", "type": "string"}], "option": "mon"}, {"parameters": [], "option": "qmp", "argument": true}, {"parameters": [], "option": "monitor", "argument": true}, {"parameters": [], "option": "parallel", "argument": true}, {"parameters": [], "option": "serial", "argument": true}, {"parameters": [], "option": "dtb", "argument": true}, {"parameters": [], "option": "initrd", "argument": true}, {"parameters": [], "option": "append", "argument": true}, {"parameters": [], "option": "kernel", "argument": true}, {"parameters": [], "option": "bt", "argument": true}, {"parameters": [{"name": "initiator-name", "help": "Initiator iqn name to use when connecting", "type": "string"}, {"name": "header-digest", "help": "HeaderDigest setting. {CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}", "type": "string"}, {"name": "password", "help": "password for CHAP authentication to target", "type": "string"}, {"name": "user", "help": "username for CHAP authentication to target", "type": "string"}], "option": "iscsi"}, {"parameters": [{"name": "chardev", "type": "string"}, {"name": "size", "type": "size"}, {"name": "debug", "type": "number"}, {"name": "name", "type": "string"}, {"name": "signal", "type": "boolean"}, {"name": "mux", "type": "boolean"}, {"name": "rows", "type": "number"}, {"name": "cols", "type": "number"}, {"name": "height", "type": "number"}, {"name": "width", "type": "number"}, {"name": "telnet", "type": "boolean"}, {"name": "delay", "type": "boolean"}, {"name": "server", "type": "boolean"}, {"name": "wait", "type": "boolean"}, {"name": "ipv6", "type": "boolean"}, {"name": "ipv4", "type": "boolean"}, {"name": "to", "type": "number"}, {"name": "localport", "type": "string"}, {"name": "localaddr", "type": "string"}, {"name": "port", "type": "string"}, {"name": "host", "type": "string"}, {"name": "path", "type": "string"}, {"name": "backend", "type": "string"}], "option": "chardev"}, {"parameters": [], "option": "netdev", "argument": true}, {"parameters": [], "option": "net", "argument": true}, {"parameters": [], "option": "smb", "argument": true}, {"parameters": [], "option": "redir", "argument": true}, {"parameters": [], "option": "bootp", "argument": true}, {"parameters": [], "option": "tftp", "argument": true}, {"parameters": [], "option": "smbios", "argument": true}, {"parameters": [], "option": "acpitable", "argument": true}, {"parameters": [], "option": "no-hpet", "argument": false}, {"parameters": [], "option": "no-acpi", "argument": false}, {"parameters": [], "option": "no-fd-bootchk", "argument": false}, {"parameters": [], "option": "rtc-td-hack", "argument": false}, {"parameters": [], "option": "win2k-hack", "argument": false}, {"parameters": [], "option": "vnc", "argument": true}, {"parameters": [], "option": "g", "argument": true}, {"parameters": [], "option": "full-screen", "argument": false}, {"parameters": [], "option": "vga", "argument": true}, {"parameters": [], "option": "rotate", "argument": true}, {"parameters": [], "option": "portrait", "argument": false}, {"parameters": [{"name": "seamless-migration", "type": "boolean"}, {"name": "playback-compression", "type": "boolean"}, {"name": "agent-mouse", "type": "boolean"}, {"name": "streaming-video", "type": "string"}, {"name": "zlib-glz-wan-compression", "type": "string"}, {"name": "jpeg-wan-compression", "type": "string"}, {"name": "image-compression", "type": "string"}, {"name": "plaintext-channel", "type": "string"}, {"name": "tls-channel", "type": "string"}, {"name": "tls-ciphers", "type": "string"}, {"name": "x509-dh-key-file", "type": "string"}, {"name": "x509-cacert-file", "type": "string"}, {"name": "x509-cert-file", "type": "string"}, {"name": "x509-key-password", "type": "string"}, {"name": "x509-key-file", "type": "string"}, {"name": "x509-dir", "type": "string"}, {"name": "sasl", "type": "boolean"}, {"name": "disable-agent-file-xfer", "type": "boolean"}, {"name": "disable-copy-paste", "type": "boolean"}, {"name": "disable-ticketing", "type": "boolean"}, {"name": "password", "type": "string"}, {"name": "ipv6", "type": "boolean"}, {"name": "ipv4", "type": "boolean"}, {"name": "addr", "type": "string"}, {"name": "tls-port", "type": "number"}, {"name": "port", "type": "number"}], "option": "spice"}, {"parameters": [], "option": "sdl", "argument": false}, {"parameters": [], "option": "no-quit", "argument": false}, {"parameters": [], "option": "ctrl-grab", "argument": false}, {"parameters": [], "option": "alt-grab", "argument": false}, {"parameters": [], "option": "no-frame", "argument": false}, {"parameters": [], "option": "curses", "argument": false}, {"parameters": [], "option": "nographic", "argument": false}, {"parameters": [], "option": "display", "argument": true}, {"parameters": [], "option": "usbdevice", "argument": true}, {"parameters": [], "option": "usb", "argument": false}, {"parameters": [], "option": "virtfs_synth", "argument": false}, {"parameters": [{"name": "sock_fd", "type": "number"}, {"name": "socket", "type": "string"}, {"name": "readonly", "type": "boolean"}, {"name": "writeout", "type": "string"}, {"name": "security_model", "type": "string"}, {"name": "mount_tag", "type": "string"}, {"name": "path", "type": "string"}, {"name": "fsdriver", "type": "string"}], "option": "virtfs"}, {"parameters": [{"name": "sock_fd", "type": "number"}, {"name": "socket", "type": "string"}, {"name": "readonly", "type": "boolean"}, {"name": "writeout", "type": "string"}, {"name": "security_model", "type": "string"}, {"name": "path", "type": "string"}, {"name": "fsdriver", "type": "string"}], "option": "fsdev"}, {"parameters": [], "option": "hdachs", "argument": true}, {"parameters": [], "option": "snapshot", "argument": false}, {"parameters": [], "option": "pflash", "argument": true}, {"parameters": [], "option": "sd", "argument": true}, {"parameters": [], "option": "mtdblock", "argument": true}, {"parameters": [{"name": "copy-on-read", "help": "copy read data from backing file into image file", "type": "boolean"}, {"name": "werror", "help": "write error action", "type": "string"}, {"name": "rerror", "help": "read error action", "type": "string"}, {"name": "read-only", "help": "open drive file as read-only", "type": "boolean"}, {"name": "file", "help": "file name", "type": "string"}, {"name": "addr", "help": "pci address (virtio only)", "type": "string"}, {"name": "boot", "help": "(deprecated, ignored)", "type": "boolean"}, {"name": "trans", "help": "chs translation (auto, lba, none)", "type": "string"}, {"name": "secs", "help": "number of sectors (ide disk geometry)", "type": "number"}, {"name": "heads", "help": "number of heads (ide disk geometry)", "type": "number"}, {"name": "cyls", "help": "number of cylinders (ide disk geometry)", "type": "number"}, {"name": "if", "help": "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)", "type": "string"}, {"name": "media", "help": "media type (disk, cdrom)", "type": "string"}, {"name": "index", "help": "index number", "type": "number"}, {"name": "unit", "help": "unit number (i.e. lun for scsi)", "type": "number"}, {"name": "bus", "help": "bus number", "type": "number"}, {"name": "throttling.iops-size", "help": "when limiting by iops max size of an I/O in bytes", "type": "number"}, {"name": "throttling.bps-write-max", "help": "total bytes write burst", "type": "number"}, {"name": "throttling.bps-read-max", "help": "total bytes read burst", "type": "number"}, {"name": "throttling.bps-total-max", "help": "total bytes burst", "type": "number"}, {"name": "throttling.iops-write-max", "help": "I/O operations write burst", "type": "number"}, {"name": "throttling.iops-read-max", "help": "I/O operations read burst", "type": "number"}, {"name": "throttling.iops-total-max", "help": "I/O operations burst", "type": "number"}, {"name": "throttling.bps-write", "help": "limit write bytes per second", "type": "number"}, {"name": "throttling.bps-read", "help": "limit read bytes per second", "type": "number"}, {"name": "throttling.bps-total", "help": "limit total bytes per second", "type": "number"}, {"name": "throttling.iops-write", "help": "limit write operations per second", "type": "number"}, {"name": "throttling.iops-read", "help": "limit read operations per second", "type": "number"}, {"name": "throttling.iops-total", "help": "limit total I/O operations per second", "type": "number"}, {"name": "werror", "help": "write error action", "type": "string"}, {"name": "serial", "help": "disk serial number", "type": "string"}, {"name": "format", "help": "disk format (raw, qcow2, ...)", "type": "string"}, {"name": "aio", "help": "host AIO implementation (threads, native)", "type": "string"}, {"name": "cache.no-flush", "help": "ignore any flush requests for the device", "type": "boolean"}, {"name": "cache.direct", "help": "enables use of O_DIRECT (bypass the host page cache)", "type": "boolean"}, {"name": "cache.writeback", "help": "enables writeback mode for any caches", "type": "boolean"}, {"name": "discard", "help": "discard operation (ignore/off, unmap/on)", "type": "string"}, {"name": "snapshot", "help": "enable/disable snapshot mode", "type": "boolean"}], "option": "drive"}, {"parameters": [], "option": "cdrom", "argument": true}, {"parameters": [], "option": "hdd", "argument": true}, {"parameters": [], "option": "hdc", "argument": true}, {"parameters": [], "option": "hdb", "argument": true}, {"parameters": [], "option": "hda", "argument": true}, {"parameters": [], "option": "fdb", "argument": true}, {"parameters": [], "option": "fda", "argument": true}, {"parameters": [], "option": "uuid", "argument": true}, {"parameters": [], "option": "name", "argument": true}, {"parameters": [], "option": "device", "argument": true}, {"parameters": [], "option": "balloon", "argument": true}, {"parameters": [], "option": "soundhw", "argument": true}, {"parameters": [], "option": "audio-help", "argument": false}, {"parameters": [], "option": "k", "argument": true}, {"parameters": [], "option": "mem-prealloc", "argument": false}, {"parameters": [], "option": "mem-path", "argument": true}, {"parameters": [], "option": "m", "argument": true}, {"parameters": [], "option": "boot", "argument": true}, {"parameters": [{"name": "value", "type": "string"}, {"name": "property", "type": "string"}, {"name": "driver", "type": "string"}], "option": "global"}, {"parameters": [], "option": "set", "argument": true}, {"parameters": [{"name": "opaque", "help": "free-form string used to describe fd", "type": "string"}, {"name": "set", "help": "ID of the fd set to add fd to", "type": "number"}, {"name": "fd", "help": "file descriptor of which a duplicate is added to fd set", "type": "number"}], "option": "add-fd"}, {"parameters": [], "option": "numa", "argument": true}, {"parameters": [], "option": "smp", "argument": true}, {"parameters": [], "option": "cpu", "argument": true}, {"parameters": [], "option": "M", "argument": true}, {"parameters": [{"name": "firmware", "help": "firmware image", "type": "string"}, {"name": "usb", "help": "Set on/off to enable/disable usb", "type": "boolean"}, {"name": "mem-merge", "help": "enable/disable memory merge support", "type": "boolean"}, {"name": "dump-guest-core", "help": "Include guest memory in  a core dump", "type": "boolean"}, {"name": "dt_compatible", "help": "Overrides the \"compatible\" property of the dt root node", "type": "string"}, {"name": "phandle_start", "help": "The first phandle ID we may generate dynamically", "type": "number"}, {"name": "dumpdtb", "help": "Dump current dtb to a file and quit", "type": "string"}, {"name": "dtb", "help": "Linux kernel device tree file", "type": "string"}, {"name": "append", "help": "Linux kernel command line", "type": "string"}, {"name": "initrd", "help": "Linux initial ramdisk file", "type": "string"}, {"name": "kernel", "help": "Linux kernel image file", "type": "string"}, {"name": "kvm_shadow_mem", "help": "KVM shadow MMU size", "type": "size"}, {"name": "kernel_irqchip", "help": "use KVM in-kernel irqchip", "type": "boolean"}, {"name": "accel", "help": "accelerator list", "type": "string"}, {"name": "type", "help": "emulated machine", "type": "string"}], "option": "machine"}, {"parameters": [], "option": "version", "argument": false}, {"parameters": [], "option": "help", "argument": false}, {"parameters": [], "option": "h", "argument": false}]}
{"return": {}}
{"timestamp": {"seconds": 1394001181, "microseconds": 494773}, "event": "SHUTDOWN"}
{"timestamp": {"seconds": 1394001181, "microseconds": 495125}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
{"timestamp": {"seconds": 1394001181, "microseconds": 495275}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]