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

[Qemu-devel] [PATCH v4 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 (eric)
V3: fix typo in commitlog and export qemu_options talbe (eric)
V4: avoid the duplicate static table (eric)

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     | 10 ++++++++++
 util/qemu-config.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
 vl.c               | 15 ---------------
 4 files changed, 57 insertions(+), 26 deletions(-)

-- 
1.8.5.3

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

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Eric Blake <eblake@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 f610101..d2facfd 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 v4 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 macro 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 "argument" to present
if the option takes unspecified arguments.

This patch also fixes options to match their actual command-line
spelling rather than an alternate name associated with the
option table in use by the command.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qapi-schema.json   |  8 ++++++--
 qemu-options.h     | 10 ++++++++++
 util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
 vl.c               | 15 ---------------
 4 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 193e7e4..4ab0d16 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4070,12 +4070,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..8a515e0 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,12 @@ enum {
 #include "qemu-options-wrapper.h"
 };
 
+typedef struct QEMUOption {
+    const char *name;
+    int flags;
+    int index;
+    uint32_t arch_mask;
+} QEMUOption;
+
+extern const QEMUOption qemu_options[];
 #endif
diff --git a/util/qemu-config.c b/util/qemu-config.c
index d2facfd..2f89b92 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -6,6 +6,16 @@
 #include "hw/qdev.h"
 #include "qapi/error.h"
 #include "qmp-commands.h"
+#include "qemu-options.h"
+
+#define HAS_ARG 0x0001
+
+const QEMUOption qemu_options[] = {
+    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
+#define QEMU_OPTIONS_GENERATE_OPTIONS
+#include "qemu-options-wrapper.h"
+    { NULL },
+};
 
 static QemuOptsList *vm_config_groups[32];
 static QemuOptsList *drive_config_groups[4];
@@ -78,6 +88,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 +160,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 685a7a9..6269762 100644
--- a/vl.c
+++ b/vl.c
@@ -111,7 +111,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"
@@ -1992,20 +1991,6 @@ static void help(int 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 v4 2/2] query-command-line-options: query all the options in qemu-options.hx

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> 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 macro 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 "argument" to present
> if the option takes unspecified arguments.
>
> This patch also fixes options to match their actual command-line
> spelling rather than an alternate name associated with the
> option table in use by the command.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json   |  8 ++++++--
>  qemu-options.h     | 10 ++++++++++
>  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  vl.c               | 15 ---------------
>  4 files changed, 54 insertions(+), 23 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 193e7e4..4ab0d16 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4070,12 +4070,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)

Suggest "if false, then the option takes no argument".

I'd call it something like 'unspecified-parameters' to emphasize the
connection with parameters.

>  #
>  # 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..8a515e0 100644
> --- a/qemu-options.h
> +++ b/qemu-options.h
> @@ -25,6 +25,8 @@
>   * THE SOFTWARE.
>   */
>  
> +#include "sysemu/arch_init.h"
> +

Why do you need this header?

If you need it, you should #include within the multiple-inclusion guard!

>  #ifndef _QEMU_OPTIONS_H_
>  #define _QEMU_OPTIONS_H_
>  
> @@ -33,4 +35,12 @@ enum {
>  #include "qemu-options-wrapper.h"
>  };
>  
> +typedef struct QEMUOption {
> +    const char *name;
> +    int flags;
> +    int index;
> +    uint32_t arch_mask;
> +} QEMUOption;
> +
> +extern const QEMUOption qemu_options[];
>  #endif
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index d2facfd..2f89b92 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -6,6 +6,16 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +#define HAS_ARG 0x0001
> +
> +const QEMUOption qemu_options[] = {
> +    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +    { NULL },
> +};
>  
>  static QemuOptsList *vm_config_groups[32];
>  static QemuOptsList *drive_config_groups[4];
> @@ -78,6 +88,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 +160,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);

Style nit: declaration after statement.  Suggest to declare it together
with i above, as "int i, idx;".

> +
> +            if (qemu_options[i].flags) {
> +                info->argument = true;
> +            }

The existing code using flags tests flags & HAS_ARG, see lookup_opt()).
Suggest to stick to that for consistency.

Please move the assigment next to the assignment to info->has_argument,
as shown below.

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

                   info->argument = qemu_options[i].flags & HAS_ARG;

>              }
>              entry = g_malloc0(sizeof(*entry));
>              entry->value = info;
> diff --git a/vl.c b/vl.c
> index 685a7a9..6269762 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -111,7 +111,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"
> @@ -1992,20 +1991,6 @@ static void help(int 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");

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

[Eric Blake]

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

On 03/05/2014 07:36 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 macro 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 "argument" to present
> if the option takes unspecified arguments.

I like Markus' suggestion of naming the new field
'unspecified-parameters' rather than 'argument'.

> 
> This patch also fixes options to match their actual command-line
> spelling rather than an alternate name associated with the
> option table in use by the command.

Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
from "acpi" to "acpitable" to match the command line option?  Same for
vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
qemu_smp_opts from "smp-opts" to "smp"?  Those were the obvious
mismatches I found where the command line was spelled differently than
the vm_config_groups entry.

This is a bug fix patch, so let's shoot to get it into 2.0.

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json   |  8 ++++++--
>  qemu-options.h     | 10 ++++++++++
>  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  vl.c               | 15 ---------------
>  4 files changed, 54 insertions(+), 23 deletions(-)

> 
> +++ b/util/qemu-config.c
> @@ -6,6 +6,16 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +#define HAS_ARG 0x0001

Hmm, we are now duplicating this macro between here and vl.c.  I'd
prefer it gets hoisted into the .h file, so that it doesn't get out of
sync between the two clients.

-- 
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 v4 2/2] query-command-line-options: query all the options in qemu-options.hx

[Amos Kong]

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

On Thu, Mar 06, 2014 at 02:23:15PM -0700, Eric Blake wrote:
> On 03/05/2014 07:36 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 macro 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 "argument" to present
> > if the option takes unspecified arguments.
> 
> I like Markus' suggestion of naming the new field
> 'unspecified-parameters' rather than 'argument'.
> 
> > 
> > This patch also fixes options to match their actual command-line
> > spelling rather than an alternate name associated with the
> > option table in use by the command.
> 
> Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
> from "acpi" to "acpitable" to match the command line option?  Same for
> vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
> qemu_smp_opts from "smp-opts" to "smp"?

Yes, we should.

> Those were the obvious
> mismatches I found where the command line was spelled differently than
> the vm_config_groups entry.
> 
> This is a bug fix patch, so let's shoot to get it into 2.0.
> 
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  qapi-schema.json   |  8 ++++++--
> >  qemu-options.h     | 10 ++++++++++
> >  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
> >  vl.c               | 15 ---------------
> >  4 files changed, 54 insertions(+), 23 deletions(-)
> 
> > 
> > +++ b/util/qemu-config.c
> > @@ -6,6 +6,16 @@
> >  #include "hw/qdev.h"
> >  #include "qapi/error.h"
> >  #include "qmp-commands.h"
> > +#include "qemu-options.h"
> > +
> > +#define HAS_ARG 0x0001
> 
> Hmm, we are now duplicating this macro between here and vl.c.  I'd
> prefer it gets hoisted into the .h file, so that it doesn't get out of
> sync between the two clients.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 



-- 
			Amos.

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

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/05/2014 07:36 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 macro 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 "argument" to present
>> if the option takes unspecified arguments.
>
> I like Markus' suggestion of naming the new field
> 'unspecified-parameters' rather than 'argument'.

Looking again, there are more problems.

1. Non-parameter argument vs. parameter argument taking unspecified
   parameters

   Example: -device takes unspecified parameters.  -cdrom doesn't take
   parameters, it takes a file name.  Yet, the command reports the same
   for both: "parameters": [], "argument": true.

   Looks like we need a tri-state: option takes no argument, QemuOpts
   argument, or other argument.

   parameters is [] unless it's a QemuOpts argument.  Then it lists the
   recognized parameters.

2. Our dear friend -drive is more complicated than you might think

   We special-case it to report the union of drive_config_groups[],
   which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
   qemu_drive_opts.  The latter accepts unspecified parameters.

   I believe qemu_drive_opts is actually not used by the (complex!) code
   parsing the argument of -drive.

   Nevertheless, said code accepts more than just qemu_legacy_drive_opts
   and qemu_common_drive_opts, namely driver-specific parameters.

   Until we define those properly in a schema, I guess the best we can
   do is add one more case: option takes QemuOpts argument, but
   parameters is not exhaustive.

>> This patch also fixes options to match their actual command-line
>> spelling rather than an alternate name associated with the
>> option table in use by the command.
>
> Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
> from "acpi" to "acpitable" to match the command line option?  Same for
> vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
> qemu_smp_opts from "smp-opts" to "smp"?  Those were the obvious
> mismatches I found where the command line was spelled differently than
> the vm_config_groups entry.

Without such a change, the command lies, because it fails to connect the
option to its QemuOptsList.  Example:

    {"parameters": [], "option": "acpitable", "argument": true},

However, the vm_config_groups[].name values are ABI: they're the section
names recognized by -readconfig and produced by -writeconfig.  Thus,
this is an incompatible change.  It's also an improvement of sorts:
things become more consistent.

We could avoid it with a suitable mapping from option name to option
group name.  Simplest way to do that is store only the exceptions from
the rule "the names are the same".

Do we care?

> This is a bug fix patch, so let's shoot to get it into 2.0.

Yes.

>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---
>>  qapi-schema.json   |  8 ++++++--
>>  qemu-options.h     | 10 ++++++++++
>>  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
>>  vl.c               | 15 ---------------
>>  4 files changed, 54 insertions(+), 23 deletions(-)
>
>> 
>> +++ b/util/qemu-config.c
>> @@ -6,6 +6,16 @@
>>  #include "hw/qdev.h"
>>  #include "qapi/error.h"
>>  #include "qmp-commands.h"
>> +#include "qemu-options.h"
>> +
>> +#define HAS_ARG 0x0001
>
> Hmm, we are now duplicating this macro between here and vl.c.  I'd
> prefer it gets hoisted into the .h file, so that it doesn't get out of
> sync between the two clients.

Flag values should be defined next to the flags type or variable, in
this case next to QEMUOption.  The patch hoists QEMUOption vl.c to
qemu-options.h.

It does that because it spreads option handling to another file.
Before, it's all in vl.c.  After, qemu_options[] and
qmp_query_command_line_options() are in qemu-config.c, and lookup_opts()
stays in vl.c.  Not thrilled by that.  Can we keep it in one place?

qemu-config.c isn't about the command line.  I suspect
qmp_query_command_line_options() ended up there just because its
problematic design choice *not* to work on command line options, but on
vm_config_groups[].  This series fixes that design choice.

We can't simply move the new qmp_query_command_line_options() out,
because it still uses vm_config_groups[], which is static.  I take that
as a sign of us not having gotten the interfaces quite right, yet.

I append a quick sketch.  It needs a bit more work to fully separate
qmp_query_command_line_options() and the helpers dealing with
CommandLineParameterInfoList from the QemuOpts code.

Since 2.0 is closing in fast, I won't insist on clean separation now.
We can do that on top.



diff --git a/util/qemu-config.c b/util/qemu-config.c
index 2f89b92..45f48da 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -49,7 +49,7 @@ QemuOptsList *qemu_find_opts(const char *group)
     return ret;
 }
 
-static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc *desc)
+static CommandLineParameterInfoList *get_param_info(const QemuOptDesc *desc)
 {
     CommandLineParameterInfoList *param_list = NULL, *entry;
     CommandLineParameterInfo *info;
@@ -88,17 +88,6 @@ 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)
 {
@@ -134,16 +123,16 @@ static void connect_infolist(CommandLineParameterInfoList *head,
 }
 
 /* access all the local QemuOptsLists for drive option */
-static CommandLineParameterInfoList *get_drive_infolist(void)
+static CommandLineParameterInfoList *get_drive_param_info(void)
 {
     CommandLineParameterInfoList *head = NULL, *cur;
     int i;
 
     for (i = 0; drive_config_groups[i] != NULL; i++) {
         if (!head) {
-            head = get_param_infolist(drive_config_groups[i]->desc);
+            head = get_param_info(drive_config_groups[i]->desc);
         } else {
-            cur = get_param_infolist(drive_config_groups[i]->desc);
+            cur = get_param_info(drive_config_groups[i]->desc);
             connect_infolist(head, cur);
         }
     }
@@ -159,28 +148,25 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
     CommandLineOptionInfoList *conf_list = NULL, *entry;
     CommandLineOptionInfo *info;
     int i;
+    QemuOptsList *list;
 
     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(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 if (idx >= 0) {
-                info->parameters =
-                    get_param_infolist(vm_config_groups[idx]->desc);
+                info->parameters = get_drive_param_info();
+            } else {
+                list = qemu_find_opts_err(qemu_options[i].name, NULL);
+                info->parameters = list ? get_param_info(list->desc) : NULL;
             }
 
             if (!info->parameters) {
                 info->has_argument = true;
+                info->argument = qemu_options[i].flags;
             }
+
             entry = g_malloc0(sizeof(*entry));
             entry->value = info;
             entry->next = conf_list;

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

[Eric Blake]

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

On 03/07/2014 02:54 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/05/2014 07:36 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.
>>>

>    Example: -device takes unspecified parameters.  -cdrom doesn't take
>    parameters, it takes a file name.  Yet, the command reports the same
>    for both: "parameters": [], "argument": true.
> 
>    Looks like we need a tri-state: option takes no argument, QemuOpts
>    argument, or other argument.

I don't buy that.  '-cdrom filename' could easily be re-written [in a
future qemu version] to use QemuOpts with an implied parameter name
(we've done that elsewhere, such as for '-machine').  In other words, I
think we could make it become shorthand for '-cdrom file=filename', at
which point the QemuOpts spelling is available and would now show up as
"parameters":[{"name":"file"...}].  Thus, in converting -cdrom to
QemuOpts, we can still maintain command-line back-compat, while making
the query-command-line-options output more featureful.  In other words,
_for now_ it takes unspecified parameters, and the fact that it is only
a single parameter in the form 'filename' rather than a more typical
parameter 'file=filename' is not a show-stopper.

So your idea of a tri-state (QemuOpts, no argument, or other argument)
doesn't add anything - any option that takes "other argument" could be
converted to take QemuOpts, and from the command line, we can't tell the
difference from whether something was implemented by QemuOpts, only by
whether we have introspection on what the argument consists of.

Meanwhile, it DOES point out that our use of implicit argument in
QemuOpts ought to be exposed to the introspection mechanism, for
introspection to be fully descriptive.  That is, maybe we should modify
our introspection to add a new 'implied-name':

##
# @CommandLineParameterInfo:
#
...
# @implied-name: #optional, if present and true, the parameter can be
#                specified as '-option value' instead of the preferred
#                spelling of '-option name=value' (since 2.0)
# Since 1.5
{ 'type': 'CommandLineParameterInfo',
  'data': { 'name': 'str',
            'type': 'CommandLineParameterType',
            '*help': 'str', '*implied-name': 'bool' } }

> 
>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
>    recognized parameters.

This part is still true.  When parameters[] is non-empty, it is a
QemuOpts and we know all recognized parameters (well, more precisely,
the subset of QemuOpts that were explicitly called out - given your
point 2 about the mess of -drive); when it is empty, then all we know is
whether the argument is a boolean or takes unspecified arguments (where
the conversion of those unknown arguments to QemuOpts will be what
finally lets us introspect the format of those unknown arguments).

> 
> 2. Our dear friend -drive is more complicated than you might think
> 
>    We special-case it to report the union of drive_config_groups[],
>    which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
>    qemu_drive_opts.  The latter accepts unspecified parameters.
> 
>    I believe qemu_drive_opts is actually not used by the (complex!) code
>    parsing the argument of -drive.
> 
>    Nevertheless, said code accepts more than just qemu_legacy_drive_opts
>    and qemu_common_drive_opts, namely driver-specific parameters.
> 
>    Until we define those properly in a schema, I guess the best we can
>    do is add one more case: option takes QemuOpts argument, but
>    parameters is not exhaustive.

We already know 'query-command-line-options' is not a full introspection
yet.  So far, libvirt has managed to get by on partial information (in
fact, the whole hack for special-casing -drive to merge multiple lists
together was precisely to avoid a regression with at least providing the
partial information that libvirt was actually using).  Documenting that
QemuOpts information may be incomplete may be nice, but shouldn't hold
up the initial purpose of this patch which is to document non-QemuOpts
options.  And knowing that an option takes unspecified arguments is
still better than not knowing about the option at all.

> 
>>> This patch also fixes options to match their actual command-line
>>> spelling rather than an alternate name associated with the
>>> option table in use by the command.
>>
>> Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
>> from "acpi" to "acpitable" to match the command line option?  Same for
>> vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
>> qemu_smp_opts from "smp-opts" to "smp"?  Those were the obvious
>> mismatches I found where the command line was spelled differently than
>> the vm_config_groups entry.
> 
> Without such a change, the command lies, because it fails to connect the
> option to its QemuOptsList.  Example:
> 
>     {"parameters": [], "option": "acpitable", "argument": true},
> 
> However, the vm_config_groups[].name values are ABI: they're the section
> names recognized by -readconfig and produced by -writeconfig.  Thus,
> this is an incompatible change.  It's also an improvement of sorts:
> things become more consistent.

Ouch.  I did not realize they were ABI.  'query-command-line-options'
should expose the command line spelling, but maybe that argues that we
need to enhance our QAPI introspection to make it easier to document the
special cases:

##
# @CommandLineOptionInfo:
...
# @config-name: #optional if present, the command line spelling differs
#               from the name used by -readconfig (since 2.0)
# Since 1.5
##
{ 'type': 'CommandLineOptionInfo',
  'data': { 'option': 'str', '*config-name':'str',
            'parameters': ['CommandLineParameterInfo'] } }

and where we would expose:

{"parameters": [], "option": "acpitable", "config-name": "acpi",
"argument": true},

or even combining my above suggestions:

{"option":"M", "parameters":[], "config-name":"machine",
 "argument": true},
{"option":"machine", "parameters":[
  {"name": "firmware", "help": "firmware image", "type": "string"},
  {"name": "type", "implied-name": true, "help": "emulated machine",
"type": "string"}, ...]},

to make it a bit more obvious that '-M str' and '-machine str' are both
shorthands for the preferred '-machine type=str', and that the same
effect is reached via a config file that has a [machine] section.

> 
> We could avoid it with a suitable mapping from option name to option
> group name.  Simplest way to do that is store only the exceptions from
> the rule "the names are the same".
>

Yes.  We've identified at least 3 exceptions now (acpitable, boot, smp),
and exposing those exceptions in the introspection is a good idea, to
make us quit adding new ones.

> Do we care?
> 
>> This is a bug fix patch, so let's shoot to get it into 2.0.
> 
> Yes.

How much work are we able to do before hard freeze? How much work are we
willing to accept as bug fix after hard freeze?

-- 
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 v4 2/2] query-command-line-options: query all the options in qemu-options.hx

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/07/2014 02:54 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 03/05/2014 07:36 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.
>>>>
>
>>    Example: -device takes unspecified parameters.  -cdrom doesn't take
>>    parameters, it takes a file name.  Yet, the command reports the same
>>    for both: "parameters": [], "argument": true.
>> 
>>    Looks like we need a tri-state: option takes no argument, QemuOpts
>>    argument, or other argument.
>
> I don't buy that.  '-cdrom filename' could easily be re-written [in a
> future qemu version] to use QemuOpts with an implied parameter name
> (we've done that elsewhere, such as for '-machine').  In other words, I
> think we could make it become shorthand for '-cdrom file=filename', at
> which point the QemuOpts spelling is available and would now show up as
> "parameters":[{"name":"file"...}].  Thus, in converting -cdrom to
> QemuOpts, we can still maintain command-line back-compat, while making
> the query-command-line-options output more featureful.  In other words,
> _for now_ it takes unspecified parameters, and the fact that it is only
> a single parameter in the form 'filename' rather than a more typical
> parameter 'file=filename' is not a show-stopper.

Incompatible change for funny filenames: -cdrom you,break=me.

Besides breaking funny filenames, we'd also buy ourselves some stupid
-readconfig / -writeconfig trouble.  Let me explain.

-cdrom F is effectively sugar for "-drive media=cdrom,index=2,file=FF",
where FF is F with comma doubled.

-writeconfig writes out desugared QemuOpts.  Therefore, "-cdrom r7.iso"
gets written as

    [drive]
      media = "cdrom"
      index = "2"
      file = "r7.iso"

which -readconfig can read.

If we convert -cdrom to QemuOpts, it gets written out like this:

    [cdrom]
       file = "r7.iso"

If we continue to desugar it, it'll *also* get written out as before.
Either we *delete* the sugared QemuOpts to avoid duplication, or we
*stop* desugaring.  The latter breaks -readconfig of existing
configuration files, and complicates the code reading configuration from
QemuOpts.

I don't think any of the old non-QemuOpts options that have become sugar
for newer, more flexible QemuOpts options should be converted to
QemuOpts.

> So your idea of a tri-state (QemuOpts, no argument, or other argument)
> doesn't add anything - any option that takes "other argument" could be
> converted to take QemuOpts, and from the command line, we can't tell the
> difference from whether something was implemented by QemuOpts, only by
> whether we have introspection on what the argument consists of.

I doubt we can convert all existing options to QemuOpts without breaking
backward compatibility and complicating the code.

> Meanwhile, it DOES point out that our use of implicit argument in
> QemuOpts ought to be exposed to the introspection mechanism, for
> introspection to be fully descriptive.  That is, maybe we should modify
> our introspection to add a new 'implied-name':
>
> ##
> # @CommandLineParameterInfo:
> #
> ...
> # @implied-name: #optional, if present and true, the parameter can be
> #                specified as '-option value' instead of the preferred
> #                spelling of '-option name=value' (since 2.0)
> # Since 1.5
> { 'type': 'CommandLineParameterInfo',
>   'data': { 'name': 'str',
>             'type': 'CommandLineParameterType',
>             '*help': 'str', '*implied-name': 'bool' } }

The only use for implied-name I can think of is interpreting a user's
command line.  Is that a real use case?

>> 
>>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
>>    recognized parameters.
>
> This part is still true.  When parameters[] is non-empty, it is a
> QemuOpts and we know all recognized parameters (well, more precisely,
> the subset of QemuOpts that were explicitly called out - given your
> point 2 about the mess of -drive); when it is empty, then all we know is
> whether the argument is a boolean or takes unspecified arguments (where
> the conversion of those unknown arguments to QemuOpts will be what
> finally lets us introspect the format of those unknown arguments).

QemuOpts argument with only unspecified parameters is not the same as
non-QemuOpts argument.  I don't think conflating the two is useful.

>> 2. Our dear friend -drive is more complicated than you might think
>> 
>>    We special-case it to report the union of drive_config_groups[],
>>    which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
>>    qemu_drive_opts.  The latter accepts unspecified parameters.
>> 
>>    I believe qemu_drive_opts is actually not used by the (complex!) code
>>    parsing the argument of -drive.
>> 
>>    Nevertheless, said code accepts more than just qemu_legacy_drive_opts
>>    and qemu_common_drive_opts, namely driver-specific parameters.
>> 
>>    Until we define those properly in a schema, I guess the best we can
>>    do is add one more case: option takes QemuOpts argument, but
>>    parameters is not exhaustive.
>
> We already know 'query-command-line-options' is not a full introspection
> yet.  So far, libvirt has managed to get by on partial information (in
> fact, the whole hack for special-casing -drive to merge multiple lists
> together was precisely to avoid a regression with at least providing the
> partial information that libvirt was actually using).  Documenting that
> QemuOpts information may be incomplete may be nice, but shouldn't hold
> up the initial purpose of this patch which is to document non-QemuOpts
> options.  And knowing that an option takes unspecified arguments is
> still better than not knowing about the option at all.

If all we want is a quick fix for "I can't see whether -frobnicate is
supported", then let's add a command to dump qemu_options[], and leave
query-command-line-options broken as designed.

But if we want proper command line introspection, then let's do it
properly: no quick hacks, no half-truths.

I can't get contents right and do backward compatibility acrobatics at
the same time.  I need to come up with the data to convey first, and a
way to shoehorn it into the existing command second.  *If* we choose to
shoehorn rather than deprecate & replace.

>>>> This patch also fixes options to match their actual command-line
>>>> spelling rather than an alternate name associated with the
>>>> option table in use by the command.
>>>
>>> Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
>>> from "acpi" to "acpitable" to match the command line option?  Same for
>>> vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
>>> qemu_smp_opts from "smp-opts" to "smp"?  Those were the obvious
>>> mismatches I found where the command line was spelled differently than
>>> the vm_config_groups entry.
>> 
>> Without such a change, the command lies, because it fails to connect the
>> option to its QemuOptsList.  Example:
>> 
>>     {"parameters": [], "option": "acpitable", "argument": true},
>> 
>> However, the vm_config_groups[].name values are ABI: they're the section
>> names recognized by -readconfig and produced by -writeconfig.  Thus,
>> this is an incompatible change.  It's also an improvement of sorts:
>> things become more consistent.
>
> Ouch.  I did not realize they were ABI.  'query-command-line-options'
> should expose the command line spelling, but maybe that argues that we
> need to enhance our QAPI introspection to make it easier to document the
> special cases:
>
> ##
> # @CommandLineOptionInfo:
> ...
> # @config-name: #optional if present, the command line spelling differs
> #               from the name used by -readconfig (since 2.0)
> # Since 1.5
> ##
> { 'type': 'CommandLineOptionInfo',
>   'data': { 'option': 'str', '*config-name':'str',
>             'parameters': ['CommandLineParameterInfo'] } }
>
> and where we would expose:
>
> {"parameters": [], "option": "acpitable", "config-name": "acpi",
> "argument": true},
>
> or even combining my above suggestions:
>
> {"option":"M", "parameters":[], "config-name":"machine",
>  "argument": true},
> {"option":"machine", "parameters":[
>   {"name": "firmware", "help": "firmware image", "type": "string"},
>   {"name": "type", "implied-name": true, "help": "emulated machine",
> "type": "string"}, ...]},
>
> to make it a bit more obvious that '-M str' and '-machine str' are both
> shorthands for the preferred '-machine type=str', and that the same
> effect is reached via a config file that has a [machine] section.

Use case for the introspection into the desugaring of -M?

Can't cover less trivial desugarings, like -cdrom.

We got more sugar than a jelly doughnut with radioactive pink frosting!

>> We could avoid it with a suitable mapping from option name to option
>> group name.  Simplest way to do that is store only the exceptions from
>> the rule "the names are the same".
>>
>
> Yes.  We've identified at least 3 exceptions now (acpitable, boot, smp),
> and exposing those exceptions in the introspection is a good idea, to
> make us quit adding new ones.

It'll make us quit adding new ones only if we can come up with a test
that breaks when we add new ones :)

>> Do we care?
>> 
>>> This is a bug fix patch, so let's shoot to get it into 2.0.
>> 
>> Yes.
>
> How much work are we able to do before hard freeze? How much work are we
> willing to accept as bug fix after hard freeze?

I don't know.

Is better command line introspection in 2.0 worth the risk that comes
with softening up the hard freeze?

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

[Eric Blake]

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

On 03/11/2014 03:04 AM, Markus Armbruster wrote:

>>      '-cdrom filename' could easily be re-written [in a
>> future qemu version] to use QemuOpts with an implied parameter name
>> (we've done that elsewhere, such as for '-machine').

> 
> Incompatible change for funny filenames: -cdrom you,break=me.

Oh.  And probably tangentially related to the bug I just reported where
'-drive id=a,file=funny,,id=1' works, but swapping to '-drive
file=funny,,id=1,id=a' fails.

> 
> Besides breaking funny filenames, we'd also buy ourselves some stupid
> -readconfig / -writeconfig trouble.  Let me explain.
> 
> -cdrom F is effectively sugar for "-drive media=cdrom,index=2,file=FF",
> where FF is F with comma doubled.
> 
> -writeconfig writes out desugared QemuOpts.  Therefore, "-cdrom r7.iso"
> gets written as
> 
>     [drive]
>       media = "cdrom"
>       index = "2"
>       file = "r7.iso"
> 
> which -readconfig can read.

So it sounds like what we REALLY want is a way to tell, for every
command line option, what that command will convert to in config
notation.  'drive' is output as-is, because it is (currently) a
first-class option, while 'cdrom' is sugar.  Mentioning that '-cdrom' is
valid on the command line is good (especially since if we DO have
introspection on what sugar is available, we can remove the sugar in the
future and libvirt can still query whether it should use the sugar form
or the preferred form when controlling a new qemu); but also mentioning
that '-cdrom' is sugar and what the preferred form is would also be good
(libvirt can use the information to use preferred forms instead of sugar).

> 
> If we convert -cdrom to QemuOpts, it gets written out like this:
> 
>     [cdrom]
>        file = "r7.iso"
> 
> If we continue to desugar it, it'll *also* get written out as before.
> Either we *delete* the sugared QemuOpts to avoid duplication, or we
> *stop* desugaring.  The latter breaks -readconfig of existing
> configuration files, and complicates the code reading configuration from
> QemuOpts.
> 
> I don't think any of the old non-QemuOpts options that have become sugar
> for newer, more flexible QemuOpts options should be converted to
> QemuOpts.

Fair enough.  So that means that for every option that exists as sugar
for some other option, the introspection should include enough details
as to give the end user a hint as to what the sugar will expand to.

> 
>> So your idea of a tri-state (QemuOpts, no argument, or other argument)
>> doesn't add anything - any option that takes "other argument" could be
>> converted to take QemuOpts, and from the command line, we can't tell the
>> difference from whether something was implemented by QemuOpts, only by
>> whether we have introspection on what the argument consists of.
> 
> I doubt we can convert all existing options to QemuOpts without breaking
> backward compatibility and complicating the code.

Point taken.

>> We already know 'query-command-line-options' is not a full introspection
>> yet.  So far, libvirt has managed to get by on partial information (in
>> fact, the whole hack for special-casing -drive to merge multiple lists
>> together was precisely to avoid a regression with at least providing the
>> partial information that libvirt was actually using).  Documenting that
>> QemuOpts information may be incomplete may be nice, but shouldn't hold
>> up the initial purpose of this patch which is to document non-QemuOpts
>> options.  And knowing that an option takes unspecified arguments is
>> still better than not knowing about the option at all.
> 
> If all we want is a quick fix for "I can't see whether -frobnicate is
> supported", then let's add a command to dump qemu_options[], and leave
> query-command-line-options broken as designed.
> 
> But if we want proper command line introspection, then let's do it
> properly: no quick hacks, no half-truths.

The current 'query-command-line-options' is misnamed, it is more like
'query-config-options'.  Maybe we want that functionality - but if so,
we should name it correctly.  Meanwhile, libvirt DOES want a way to
query whether -frobnicate is supported, but as we already lack it, not
having it in 2.0 won't make a difference, so I agree with delaying this
series until 2.1 when we can think more about getting it right rather
than fast.

>> {"option":"M", "parameters":[], "config-name":"machine",
>>  "argument": true},
>> {"option":"machine", "parameters":[
>>   {"name": "firmware", "help": "firmware image", "type": "string"},
>>   {"name": "type", "implied-name": true, "help": "emulated machine",
>> "type": "string"}, ...]},
>>
>> to make it a bit more obvious that '-M str' and '-machine str' are both
>> shorthands for the preferred '-machine type=str', and that the same
>> effect is reached via a config file that has a [machine] section.
> 
> Use case for the introspection into the desugaring of -M?
> 
> Can't cover less trivial desugarings, like -cdrom.

Use case for any desugaring is to learn what the preferred form is, and
to know that a preferred form exists (in case there are other existing
command-line arguments that we convert in the future by adding a new
preferred QemuOpts form).  But if we express desugaring at all, then we
need to fully express it - so we need a schema that shows exactly how
cdrom is converted into drive (including both the implied media=cdrom
and index=2 arguments, as well as the named file=FF conversion).

>>
>> Yes.  We've identified at least 3 exceptions now (acpitable, boot, smp),
>> and exposing those exceptions in the introspection is a good idea, to
>> make us quit adding new ones.
> 
> It'll make us quit adding new ones only if we can come up with a test
> that breaks when we add new ones :)

Too true.

>>> Do we care?
>>>
>>>> This is a bug fix patch, so let's shoot to get it into 2.0.
>>>
>>> Yes.

So far, detecting boolean options like -enable-fips has never worked, so
that is out of scope for 2.0.  Likewise, the bug between the command
line -acpitable and the config file [acpi] not having matching names has
been buggy since the query-command-line-options was introduced and
hasn't been tripping up libvirt yet, so again rushing a fix may not be
the best.

> 
> Is better command line introspection in 2.0 worth the risk that comes
> with softening up the hard freeze?

At this point, probably not.

-- 
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 v4 2/2] query-command-line-options: query all the options in qemu-options.hx

[Amos Kong]

On Tue, Mar 11, 2014 at 10:04:56AM +0100, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 03/07/2014 02:54 AM, Markus Armbruster wrote:
> >> Eric Blake <eblake@redhat.com> writes:
> >> 
> >>> On 03/05/2014 07:36 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.
> >>>>
> >
> >>    Example: -device takes unspecified parameters.  -cdrom doesn't take
> >>    parameters, it takes a file name.  Yet, the command reports the same
> >>    for both: "parameters": [], "argument": true.
> >> 
> >>    Looks like we need a tri-state: option takes no argument, QemuOpts
> >>    argument, or other argument.
> >
> > I don't buy that.  '-cdrom filename' could easily be re-written [in a
> > future qemu version] to use QemuOpts with an implied parameter name
> > (we've done that elsewhere, such as for '-machine').  In other words, I
> > think we could make it become shorthand for '-cdrom file=filename', at
> > which point the QemuOpts spelling is available and would now show up as
> > "parameters":[{"name":"file"...}].  Thus, in converting -cdrom to
> > QemuOpts, we can still maintain command-line back-compat, while making
> > the query-command-line-options output more featureful.  In other words,
> > _for now_ it takes unspecified parameters, and the fact that it is only
> > a single parameter in the form 'filename' rather than a more typical
> > parameter 'file=filename' is not a show-stopper.
> 
> Incompatible change for funny filenames: -cdrom you,break=me.
> 
> Besides breaking funny filenames, we'd also buy ourselves some stupid
> -readconfig / -writeconfig trouble.  Let me explain.
> 
> -cdrom F is effectively sugar for "-drive media=cdrom,index=2,file=FF",
> where FF is F with comma doubled.
> 
> -writeconfig writes out desugared QemuOpts.  Therefore, "-cdrom r7.iso"
> gets written as
> 
>     [drive]
>       media = "cdrom"
>       index = "2"
>       file = "r7.iso"
> 
> which -readconfig can read.
> 
> If we convert -cdrom to QemuOpts, it gets written out like this:
> 
>     [cdrom]
>        file = "r7.iso"
> 
> If we continue to desugar it, it'll *also* get written out as before.
> Either we *delete* the sugared QemuOpts to avoid duplication, or we
> *stop* desugaring.  The latter breaks -readconfig of existing
> configuration files, and complicates the code reading configuration from
> QemuOpts.
> 
> I don't think any of the old non-QemuOpts options that have become sugar
> for newer, more flexible QemuOpts options should be converted to
> QemuOpts.
> 
> > So your idea of a tri-state (QemuOpts, no argument, or other argument)
> > doesn't add anything - any option that takes "other argument" could be
> > converted to take QemuOpts, and from the command line, we can't tell the
> > difference from whether something was implemented by QemuOpts, only by
> > whether we have introspection on what the argument consists of.
> 
> I doubt we can convert all existing options to QemuOpts without breaking
> backward compatibility and complicating the code.
> 
> > Meanwhile, it DOES point out that our use of implicit argument in
> > QemuOpts ought to be exposed to the introspection mechanism, for
> > introspection to be fully descriptive.  That is, maybe we should modify
> > our introspection to add a new 'implied-name':
> >
> > ##
> > # @CommandLineParameterInfo:
> > #
> > ...
> > # @implied-name: #optional, if present and true, the parameter can be
> > #                specified as '-option value' instead of the preferred
> > #                spelling of '-option name=value' (since 2.0)
> > # Since 1.5
> > { 'type': 'CommandLineParameterInfo',
> >   'data': { 'name': 'str',
> >             'type': 'CommandLineParameterType',
> >             '*help': 'str', '*implied-name': 'bool' } }
 
How can we get this information? it's not good to rely on the help message.

And the parameters [] only have content when the option have a non-NULL desc
table, so we always just return a NULL parameters list, the 'implied-name'
information will be lost.

I thinks Markus's suggestion is fine, we can use tri-state (no-arg,
unsuecified-para-arg, no-para-arg).


Thanks, Amos

> The only use for implied-name I can think of is interpreting a user's
> command line.  Is that a real use case?

 
> >> 
> >>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
> >>    recognized parameters.
> >
> > This part is still true.  When parameters[] is non-empty, it is a
> > QemuOpts and we know all recognized parameters (well, more precisely,
> > the subset of QemuOpts that were explicitly called out - given your
> > point 2 about the mess of -drive); when it is empty, then all we know is
> > whether the argument is a boolean or takes unspecified arguments (where
> > the conversion of those unknown arguments to QemuOpts will be what
> > finally lets us introspect the format of those unknown arguments).
> 
> QemuOpts argument with only unspecified parameters is not the same as
> non-QemuOpts argument.  I don't think conflating the two is useful.
> 
> >> 2. Our dear friend -drive is more complicated than you might think
> >> 
> >>    We special-case it to report the union of drive_config_groups[],
> >>    which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
> >>    qemu_drive_opts.  The latter accepts unspecified parameters.
> >> 
> >>    I believe qemu_drive_opts is actually not used by the (complex!) code
> >>    parsing the argument of -drive.
> >> 
> >>    Nevertheless, said code accepts more than just qemu_legacy_drive_opts
> >>    and qemu_common_drive_opts, namely driver-specific parameters.
> >> 
> >>    Until we define those properly in a schema, I guess the best we can
> >>    do is add one more case: option takes QemuOpts argument, but
> >>    parameters is not exhaustive.
> >
> > We already know 'query-command-line-options' is not a full introspection
> > yet.  So far, libvirt has managed to get by on partial information (in
> > fact, the whole hack for special-casing -drive to merge multiple lists
> > together was precisely to avoid a regression with at least providing the
> > partial information that libvirt was actually using).  Documenting that
> > QemuOpts information may be incomplete may be nice, but shouldn't hold
> > up the initial purpose of this patch which is to document non-QemuOpts
> > options.  And knowing that an option takes unspecified arguments is
> > still better than not knowing about the option at all.
> 
> If all we want is a quick fix for "I can't see whether -frobnicate is
> supported", then let's add a command to dump qemu_options[], and leave
> query-command-line-options broken as designed.
> 
> But if we want proper command line introspection, then let's do it
> properly: no quick hacks, no half-truths.
> 
> I can't get contents right and do backward compatibility acrobatics at
> the same time.  I need to come up with the data to convey first, and a
> way to shoehorn it into the existing command second.  *If* we choose to
> shoehorn rather than deprecate & replace.
> 
> >>>> This patch also fixes options to match their actual command-line
> >>>> spelling rather than an alternate name associated with the
> >>>> option table in use by the command.
> >>>
> >>> Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
> >>> from "acpi" to "acpitable" to match the command line option?  Same for
> >>> vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
> >>> qemu_smp_opts from "smp-opts" to "smp"?  Those were the obvious
> >>> mismatches I found where the command line was spelled differently than
> >>> the vm_config_groups entry.
> >> 
> >> Without such a change, the command lies, because it fails to connect the
> >> option to its QemuOptsList.  Example:
> >> 
> >>     {"parameters": [], "option": "acpitable", "argument": true},
> >> 
> >> However, the vm_config_groups[].name values are ABI: they're the section
> >> names recognized by -readconfig and produced by -writeconfig.  Thus,
> >> this is an incompatible change.  It's also an improvement of sorts:
> >> things become more consistent.
> >
> > Ouch.  I did not realize they were ABI.  'query-command-line-options'
> > should expose the command line spelling, but maybe that argues that we
> > need to enhance our QAPI introspection to make it easier to document the
> > special cases:
> >
> > ##
> > # @CommandLineOptionInfo:
> > ...
> > # @config-name: #optional if present, the command line spelling differs
> > #               from the name used by -readconfig (since 2.0)
> > # Since 1.5
> > ##
> > { 'type': 'CommandLineOptionInfo',
> >   'data': { 'option': 'str', '*config-name':'str',
> >             'parameters': ['CommandLineParameterInfo'] } }
> >
> > and where we would expose:
> >
> > {"parameters": [], "option": "acpitable", "config-name": "acpi",
> > "argument": true},
> >
> > or even combining my above suggestions:
> >
> > {"option":"M", "parameters":[], "config-name":"machine",
> >  "argument": true},
> > {"option":"machine", "parameters":[
> >   {"name": "firmware", "help": "firmware image", "type": "string"},
> >   {"name": "type", "implied-name": true, "help": "emulated machine",
> > "type": "string"}, ...]},
> >
> > to make it a bit more obvious that '-M str' and '-machine str' are both
> > shorthands for the preferred '-machine type=str', and that the same
> > effect is reached via a config file that has a [machine] section.
> 
> Use case for the introspection into the desugaring of -M?
> 
> Can't cover less trivial desugarings, like -cdrom.
> 
> We got more sugar than a jelly doughnut with radioactive pink frosting!
> 
> >> We could avoid it with a suitable mapping from option name to option
> >> group name.  Simplest way to do that is store only the exceptions from
> >> the rule "the names are the same".
> >>
> >
> > Yes.  We've identified at least 3 exceptions now (acpitable, boot, smp),
> > and exposing those exceptions in the introspection is a good idea, to
> > make us quit adding new ones.
> 
> It'll make us quit adding new ones only if we can come up with a test
> that breaks when we add new ones :)
> 
> >> Do we care?
> >> 
> >>> This is a bug fix patch, so let's shoot to get it into 2.0.
> >> 
> >> Yes.
> >
> > How much work are we able to do before hard freeze? How much work are we
> > willing to accept as bug fix after hard freeze?
> 
> I don't know.
> 
> Is better command line introspection in 2.0 worth the risk that comes
> with softening up the hard freeze?

-- 
			Amos.

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

[Amos Kong]

On Thu, Mar 20, 2014 at 10:12:43PM +0800, Amos Kong wrote:
> On Tue, Mar 11, 2014 at 10:04:56AM +0100, Markus Armbruster wrote:
> > Eric Blake <eblake@redhat.com> writes:
> > 
> > > On 03/07/2014 02:54 AM, Markus Armbruster wrote:
> > >> Eric Blake <eblake@redhat.com> writes:
> > >> 
> > >>> On 03/05/2014 07:36 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.
> > >>>>
> > >
> > >>    Example: -device takes unspecified parameters.  -cdrom doesn't take
> > >>    parameters, it takes a file name.  Yet, the command reports the same
> > >>    for both: "parameters": [], "argument": true.
> > >> 
> > >>    Looks like we need a tri-state: option takes no argument, QemuOpts
> > >>    argument, or other argument.
> > >
> > > I don't buy that.  '-cdrom filename' could easily be re-written [in a
> > > future qemu version] to use QemuOpts with an implied parameter name
> > > (we've done that elsewhere, such as for '-machine').  In other words, I
> > > think we could make it become shorthand for '-cdrom file=filename', at
> > > which point the QemuOpts spelling is available and would now show up as
> > > "parameters":[{"name":"file"...}].  Thus, in converting -cdrom to
> > > QemuOpts, we can still maintain command-line back-compat, while making
> > > the query-command-line-options output more featureful.  In other words,
> > > _for now_ it takes unspecified parameters, and the fact that it is only
> > > a single parameter in the form 'filename' rather than a more typical
> > > parameter 'file=filename' is not a show-stopper.
> > 
> > Incompatible change for funny filenames: -cdrom you,break=me.
> > 
> > Besides breaking funny filenames, we'd also buy ourselves some stupid
> > -readconfig / -writeconfig trouble.  Let me explain.
> > 
> > -cdrom F is effectively sugar for "-drive media=cdrom,index=2,file=FF",
> > where FF is F with comma doubled.
> > 
> > -writeconfig writes out desugared QemuOpts.  Therefore, "-cdrom r7.iso"
> > gets written as
> > 
> >     [drive]
> >       media = "cdrom"
> >       index = "2"
> >       file = "r7.iso"
> > 
> > which -readconfig can read.
> > 
> > If we convert -cdrom to QemuOpts, it gets written out like this:
> > 
> >     [cdrom]
> >        file = "r7.iso"
> > 
> > If we continue to desugar it, it'll *also* get written out as before.
> > Either we *delete* the sugared QemuOpts to avoid duplication, or we
> > *stop* desugaring.  The latter breaks -readconfig of existing
> > configuration files, and complicates the code reading configuration from
> > QemuOpts.
> > 
> > I don't think any of the old non-QemuOpts options that have become sugar
> > for newer, more flexible QemuOpts options should be converted to
> > QemuOpts.
> > 
> > > So your idea of a tri-state (QemuOpts, no argument, or other argument)
> > > doesn't add anything - any option that takes "other argument" could be
> > > converted to take QemuOpts, and from the command line, we can't tell the
> > > difference from whether something was implemented by QemuOpts, only by
> > > whether we have introspection on what the argument consists of.
> > 
> > I doubt we can convert all existing options to QemuOpts without breaking
> > backward compatibility and complicating the code.
> > 
> > > Meanwhile, it DOES point out that our use of implicit argument in
> > > QemuOpts ought to be exposed to the introspection mechanism, for
> > > introspection to be fully descriptive.  That is, maybe we should modify
> > > our introspection to add a new 'implied-name':
> > >
> > > ##
> > > # @CommandLineParameterInfo:
> > > #
> > > ...
> > > # @implied-name: #optional, if present and true, the parameter can be
> > > #                specified as '-option value' instead of the preferred
> > > #                spelling of '-option name=value' (since 2.0)
> > > # Since 1.5
> > > { 'type': 'CommandLineParameterInfo',
> > >   'data': { 'name': 'str',
> > >             'type': 'CommandLineParameterType',
> > >             '*help': 'str', '*implied-name': 'bool' } }
  
reply-myself

> How can we get this information? it's not good to rely on the help message.
> 
> And the parameters [] only have content when the option have a non-NULL desc
> table, so we always just return a NULL parameters list, the 'implied-name'
> information will be lost.

I'm wrong, 'implied-name' is attribute of parameter, it's stored in
QemuOptsList.implied_opt_name.

I will add this filed in V6.
 
> I thinks Markus's suggestion is fine, we can use tri-state (no-arg,
> unsuecified-para-arg, no-para-arg).

Thanks, Amos
 
> > The only use for implied-name I can think of is interpreting a user's
> > command line.  Is that a real use case?
> 
>  
> > >> 
> > >>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
> > >>    recognized parameters.
> > >
> > > This part is still true.  When parameters[] is non-empty, it is a
> > > QemuOpts and we know all recognized parameters (well, more precisely,
> > > the subset of QemuOpts that were explicitly called out - given your
> > > point 2 about the mess of -drive); when it is empty, then all we know is
> > > whether the argument is a boolean or takes unspecified arguments (where
> > > the conversion of those unknown arguments to QemuOpts will be what
> > > finally lets us introspect the format of those unknown arguments).
> > 
> > QemuOpts argument with only unspecified parameters is not the same as
> > non-QemuOpts argument.  I don't think conflating the two is useful.

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

[Amos Kong]

On Fri, Mar 07, 2014 at 10:54:09AM +0100, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 03/05/2014 07:36 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 macro 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 "argument" to present
> >> if the option takes unspecified arguments.
> >
> > I like Markus' suggestion of naming the new field
> > 'unspecified-parameters' rather than 'argument'.
 
Hi Markus,

> Looking again, there are more problems.
> 
> 1. Non-parameter argument vs. parameter argument taking unspecified
>    parameters
> 
>    Example: -device takes unspecified parameters.  -cdrom doesn't take
>    parameters, it takes a file name.  Yet, the command reports the same
>    for both: "parameters": [], "argument": true.
> 
>    Looks like we need a tri-state: option takes no argument, QemuOpts
>    argument, or other argument.

'-cdrom' is the 'other argument' == 'Non-parameter argument'?

We can use a enum state.

|  { 'enum': 'ArgumentStateType',
|    'data': ['no-argument', 'unspecified-parameters-argument',
|             'non-parameter-argument']
|  }
|  
|  { 'type': 'CommandLineOptionInfo',
|    'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
|              '*argument-state': 'ArgumentStateType' } }
 
>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
>    recognized parameters.

How about balloon? we should treat as 'taking unspecified parameters'?

    "-balloon none   disable balloon device\n"
    "-balloon virtio[,addr=str]\n"

I think we can only check this from help message in qemu-options.hx,
is it a stable/acceptable method?

We introduce query-command-line-options command to avoid libvirt to
check qemu commandline information by scanning qemu's help message,
it's not strict & stable.
 
> 2. Our dear friend -drive is more complicated than you might think
> 
>    We special-case it to report the union of drive_config_groups[],
>    which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
>    qemu_drive_opts.  The latter accepts unspecified parameters.

I'm confused here. But there is only one option (-drive), we should
return merged desc table (legacy + common).

Desc table of qemu_drive_opts is NULL, then -drive can also take
unspecified parameters?
 
>    I believe qemu_drive_opts is actually not used by the (complex!) code
>    parsing the argument of -drive.
> 
>    Nevertheless, said code accepts more than just qemu_legacy_drive_opts
>    and qemu_common_drive_opts, namely driver-specific parameters.
> 
>    Until we define those properly in a schema, I guess the best we can
>    do is add one more case: option takes QemuOpts argument, but
>    parameters is not exhaustive.


Thanks, Amos
 
> >> This patch also fixes options to match their actual command-line
> >> spelling rather than an alternate name associated with the
> >> option table in use by the command.
> >
> > Should we independently patch hw/acpi/core.c to rename qemu_acpi_opts
> > from "acpi" to "acpitable" to match the command line option?  Same for
> > vl.c and qemu_boot_opts from "boot-opts" to "boot"?  Same for vl.c and
> > qemu_smp_opts from "smp-opts" to "smp"?  Those were the obvious
> > mismatches I found where the command line was spelled differently than
> > the vm_config_groups entry.
> 
> Without such a change, the command lies, because it fails to connect the
> option to its QemuOptsList.  Example:
> 
>     {"parameters": [], "option": "acpitable", "argument": true},
> 
> However, the vm_config_groups[].name values are ABI: they're the section
> names recognized by -readconfig and produced by -writeconfig.  Thus,
> this is an incompatible change.  It's also an improvement of sorts:
> things become more consistent.
> 
> We could avoid it with a suitable mapping from option name to option
> group name.  Simplest way to do that is store only the exceptions from
> the rule "the names are the same".
> 
> Do we care?
> 
> > This is a bug fix patch, so let's shoot to get it into 2.0.
> 
> Yes.
> 
> >> Signed-off-by: Amos Kong <akong@redhat.com>
> >> ---
> >>  qapi-schema.json   |  8 ++++++--
> >>  qemu-options.h     | 10 ++++++++++
> >>  util/qemu-config.c | 44 ++++++++++++++++++++++++++++++++++++++------
> >>  vl.c               | 15 ---------------
> >>  4 files changed, 54 insertions(+), 23 deletions(-)
> >
> >> 
> >> +++ b/util/qemu-config.c
> >> @@ -6,6 +6,16 @@
> >>  #include "hw/qdev.h"
> >>  #include "qapi/error.h"
> >>  #include "qmp-commands.h"
> >> +#include "qemu-options.h"
> >> +
> >> +#define HAS_ARG 0x0001
> >
> > Hmm, we are now duplicating this macro between here and vl.c.  I'd
> > prefer it gets hoisted into the .h file, so that it doesn't get out of
> > sync between the two clients.
> 
> Flag values should be defined next to the flags type or variable, in
> this case next to QEMUOption.  The patch hoists QEMUOption vl.c to
> qemu-options.h.
> 
> It does that because it spreads option handling to another file.
> Before, it's all in vl.c.  After, qemu_options[] and
> qmp_query_command_line_options() are in qemu-config.c, and lookup_opts()
> stays in vl.c.  Not thrilled by that.  Can we keep it in one place?
> 
> qemu-config.c isn't about the command line.  I suspect
> qmp_query_command_line_options() ended up there just because its
> problematic design choice *not* to work on command line options, but on
> vm_config_groups[].  This series fixes that design choice.
> 
> We can't simply move the new qmp_query_command_line_options() out,
> because it still uses vm_config_groups[], which is static.  I take that
> as a sign of us not having gotten the interfaces quite right, yet.
> 
> I append a quick sketch.  It needs a bit more work to fully separate
> qmp_query_command_line_options() and the helpers dealing with
> CommandLineParameterInfoList from the QemuOpts code.
> 
> Since 2.0 is closing in fast, I won't insist on clean separation now.
> We can do that on top.
> 
> 
> 
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 2f89b92..45f48da 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -49,7 +49,7 @@ QemuOptsList *qemu_find_opts(const char *group)
>      return ret;
>  }
>  
> -static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc *desc)
> +static CommandLineParameterInfoList *get_param_info(const QemuOptDesc *desc)
>  {
>      CommandLineParameterInfoList *param_list = NULL, *entry;
>      CommandLineParameterInfo *info;
> @@ -88,17 +88,6 @@ 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)
>  {
> @@ -134,16 +123,16 @@ static void connect_infolist(CommandLineParameterInfoList *head,
>  }
>  
>  /* access all the local QemuOptsLists for drive option */
> -static CommandLineParameterInfoList *get_drive_infolist(void)
> +static CommandLineParameterInfoList *get_drive_param_info(void)
>  {
>      CommandLineParameterInfoList *head = NULL, *cur;
>      int i;
>  
>      for (i = 0; drive_config_groups[i] != NULL; i++) {
>          if (!head) {
> -            head = get_param_infolist(drive_config_groups[i]->desc);
> +            head = get_param_info(drive_config_groups[i]->desc);
>          } else {
> -            cur = get_param_infolist(drive_config_groups[i]->desc);
> +            cur = get_param_info(drive_config_groups[i]->desc);
>              connect_infolist(head, cur);
>          }
>      }
> @@ -159,28 +148,25 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>      CommandLineOptionInfoList *conf_list = NULL, *entry;
>      CommandLineOptionInfo *info;
>      int i;
> +    QemuOptsList *list;
>  
>      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(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 if (idx >= 0) {
> -                info->parameters =
> -                    get_param_infolist(vm_config_groups[idx]->desc);
> +                info->parameters = get_drive_param_info();
> +            } else {
> +                list = qemu_find_opts_err(qemu_options[i].name, NULL);
> +                info->parameters = list ? get_param_info(list->desc) : NULL;
>              }
>  
>              if (!info->parameters) {
>                  info->has_argument = true;
> +                info->argument = qemu_options[i].flags;
>              }
> +
>              entry = g_malloc0(sizeof(*entry));
>              entry->value = info;
>              entry->next = conf_list;

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

[Amos Kong]

On Thu, Mar 20, 2014 at 10:03:12PM +0800, Amos Kong wrote:
> On Fri, Mar 07, 2014 at 10:54:09AM +0100, Markus Armbruster wrote:
> > Eric Blake <eblake@redhat.com> writes:
> > 
> > > On 03/05/2014 07:36 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 macro 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 "argument" to present
> > >> if the option takes unspecified arguments.
> > >
> > > I like Markus' suggestion of naming the new field
> > > 'unspecified-parameters' rather than 'argument'.
>  
> Hi Markus,
> 
> > Looking again, there are more problems.
> > 
> > 1. Non-parameter argument vs. parameter argument taking unspecified
> >    parameters
> > 
> >    Example: -device takes unspecified parameters.  -cdrom doesn't take
> >    parameters, it takes a file name.  Yet, the command reports the same
> >    for both: "parameters": [], "argument": true.
> > 
> >    Looks like we need a tri-state: option takes no argument, QemuOpts
> >    argument, or other argument.
> 
> '-cdrom' is the 'other argument' == 'Non-parameter argument'?
> 
> We can use a enum state.
> 
> |  { 'enum': 'ArgumentStateType',
> |    'data': ['no-argument', 'unspecified-parameters-argument',
> |             'non-parameter-argument']
> |  }
> |  
> |  { 'type': 'CommandLineOptionInfo',
> |    'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
> |              '*argument-state': 'ArgumentStateType' } }

> >    parameters is [] unless it's a QemuOpts argument.  Then it lists the
> >    recognized parameters.
> 
> How about balloon? we should treat as 'taking unspecified parameters'?
> 
>     "-balloon none   disable balloon device\n"
>     "-balloon virtio[,addr=str]\n"

I think we should treat as 'unspecified parameters'
 
> I think we can only check this from help message in qemu-options.hx,
> is it a stable/acceptable method?
> 
> We introduce query-command-line-options command to avoid libvirt to
> check qemu commandline information by scanning qemu's help message,
> it's not strict & stable.


[not a completed patch] 

diff --git a/qemu-options.h b/qemu-options.h
index 4024487..b08a6dc 100644
--- a/qemu-options.h
+++ b/qemu-options.h
@@ -41,6 +41,7 @@ typedef struct QEMUOption {
     const char *name;
     int flags;
     int index;
+    const char *help;
     uint32_t arch_mask;
 } QEMUOption;
 
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 5bc3c31..e7758e3 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -9,7 +9,8 @@
 #include "qemu-options.h"
 
 const QEMUOption qemu_options[] = {
-    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
+    { "h", 0, QEMU_OPTION_h, "-h or -help     display this help and exit\n",
+      QEMU_ARCH_ALL },
 #define QEMU_OPTIONS_GENERATE_OPTIONS
 #include "qemu-options-wrapper.h"
     { NULL },
@@ -147,6 +148,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_
     CommandLineOptionInfo *info;
     QemuOptsList *list;
     int i;
+    const char *p;
 
     for (i = 0; qemu_options[i].name; i++) {
         if (!has_option || !strcmp(option, qemu_options[i].name)) {
@@ -161,8 +163,23 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has
             }
 
             if (!info->parameters) {
-                info->has_unspecified_parameters = true;
-                info->unspecified_parameters = qemu_options[i].flags & HAS_ARG;
+                info->has_argument_state = true;
+
+                if (!qemu_options[i].flags & HAS_ARG) {
+                    info->argument_state = ARGUMENT_STATE_TYPE_NO_ARGUMENT;
+                } else {
+                    info->argument_state =
+                        ARGUMENT_STATE_TYPE_NO_PARAMETER_ARGUMENT;
+                }
+
+               p = qemu_options[i].help;
+                while (*p) {
+                    if (*p != ' ' && *(p + 1) == '[') {

                              ^^^^^
I know the string matching in while loop is too crude ;-) we can make it strict.

+                        info->argument_state =
+                            ARGUMENT_STATE_TYPE_UNSPECIFIED_PARAMETERS_ARGUMENT;
+                    }
+                    p += 1;
+                }
             }
 
             entry = g_malloc0(sizeof(*entry));



===================== query command output ============

{
    "return": [
        {
            "parameters": [
                {
                    "name": "timestamp", 
                    "type": "boolean"
                }
            ], 
            "option": "msg"
        }, 
        {
            "parameters": [
            ], 
            "option": "object", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "tdf", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-kvm-irqchip", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-kvm-pit", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-kvm-pit-reinjection", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-kvm", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "enable-fips", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "qtest-log", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "qtest", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "file", 
                    "type": "string"
                }, 
                {
                    "name": "events", 
                    "type": "string"
                }
            ], 
            "option": "trace"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-user-config", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "nodefconfig", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "writeconfig", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "readconfig", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "enable", 
                    "type": "boolean"
                }
            ], 
            "option": "sandbox"
        }, 
        {
            "parameters": [
            ], 
            "option": "old-param", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "semihosting", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "prom-env", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "runas", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "chroot", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "nodefaults", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "incoming", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "tb-size", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "show-cursor", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "virtioconsole", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "echr", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "watchdog-action", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "watchdog", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "icount", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "driftfix", 
                    "type": "string"
                }, 
                {
                    "name": "clock", 
                    "type": "string"
                }, 
                {
                    "name": "base", 
                    "type": "string"
                }
            ], 
            "option": "rtc"
        }, 
        {
            "parameters": [
            ], 
            "option": "startdate", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "localtime", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "clock", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "romfile", 
                    "type": "string"
                }, 
                {
                    "name": "bootindex", 
                    "type": "number"
                }
            ], 
            "option": "option-rom"
        }, 
        {
            "parameters": [
            ], 
            "option": "daemonize", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "loadvm", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-shutdown", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-reboot", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "xen-attach", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "xen-create", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "xen-domid", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "enable-kvm", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "bios", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "L", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "D", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "d", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "s", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "gdb", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "mlock", 
                    "type": "boolean"
                }
            ], 
            "option": "realtime"
        }, 
        {
            "parameters": [
            ], 
            "option": "S", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "singlestep", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "pidfile", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "debugcon", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "pretty", 
                    "type": "boolean"
                }, 
                {
                    "name": "default", 
                    "type": "boolean"
                }, 
                {
                    "name": "chardev", 
                    "type": "string"
                }, 
                {
                    "name": "mode", 
                    "type": "string"
                }
            ], 
            "option": "mon"
        }, 
        {
            "parameters": [
            ], 
            "option": "qmp", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "monitor", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "parallel", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "serial", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "dtb", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "initrd", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "append", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "kernel", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "bt", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "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-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "net", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "smb", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "redir", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "bootp", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "tftp", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "smbios", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "acpitable", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-hpet", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-acpi", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-fd-bootchk", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "rtc-td-hack", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "win2k-hack", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "vnc", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "g", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "full-screen", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "vga", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "rotate", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "portrait", 
            "argument-state": "no-argument"
        }, 
        {
            "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-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-quit", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "ctrl-grab", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "alt-grab", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "no-frame", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "curses", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "nographic", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "display", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "usbdevice", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "usb", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "virtfs_synth", 
            "argument-state": "no-argument"
        }, 
        {
            "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-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "snapshot", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "pflash", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "sd", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "mtdblock", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "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-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "hdd", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "hdc", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "hdb", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "hda", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "fdb", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "fda", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "uuid", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "debug-threads", 
                    "help": "When enabled, name the individual threads; defaults off.\nNOTE: The thread names are for debugging and not a\nstable API.", 
                    "type": "boolean"
                }, 
                {
                    "name": "process", 
                    "help": "Sets the name of the QEMU process, as shown in top etc", 
                    "type": "string"
                }, 
                {
                    "name": "guest", 
                    "help": "Sets the name of the guest.\nThis name will be displayed in the SDL window caption.\nThe name will also be used for the VNC server", 
                    "type": "string"
                }
            ], 
            "option": "name"
        }, 
        {
            "parameters": [
            ], 
            "option": "device", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "balloon", 
            "argument-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "soundhw", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "audio-help", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "k", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "mem-prealloc", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "mem-path", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "m", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "strict", 
                    "type": "boolean"
                }, 
                {
                    "name": "reboot-timeout", 
                    "type": "string"
                }, 
                {
                    "name": "splash-time", 
                    "type": "string"
                }, 
                {
                    "name": "splash", 
                    "type": "string"
                }, 
                {
                    "name": "menu", 
                    "type": "boolean"
                }, 
                {
                    "name": "once", 
                    "type": "string"
                }, 
                {
                    "name": "order", 
                    "type": "string"
                }
            ], 
            "option": "boot"
        }, 
        {
            "parameters": [
                {
                    "name": "value", 
                    "type": "string"
                }, 
                {
                    "name": "property", 
                    "type": "string"
                }, 
                {
                    "name": "driver", 
                    "type": "string"
                }
            ], 
            "option": "global"
        }, 
        {
            "parameters": [
            ], 
            "option": "set", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "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-state": "unspecified-parameters-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "maxcpus", 
                    "type": "number"
                }, 
                {
                    "name": "threads", 
                    "type": "number"
                }, 
                {
                    "name": "cores", 
                    "type": "number"
                }, 
                {
                    "name": "sockets", 
                    "type": "number"
                }, 
                {
                    "name": "cpus", 
                    "type": "number"
                }
            ], 
            "option": "smp"
        }, 
        {
            "parameters": [
            ], 
            "option": "cpu", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "M", 
            "argument-state": "no-parameter-argument"
        }, 
        {
            "parameters": [
                {
                    "name": "kvm-type", 
                    "help": "Specifies the KVM virtualization mode (HV, PR)", 
                    "type": "string"
                }, 
                {
                    "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-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "help", 
            "argument-state": "no-argument"
        }, 
        {
            "parameters": [
            ], 
            "option": "h", 
            "argument-state": "no-argument"
        }
    ]
}

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> On Fri, Mar 07, 2014 at 10:54:09AM +0100, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 03/05/2014 07:36 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 macro 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 "argument" to present
>> >> if the option takes unspecified arguments.
>> >
>> > I like Markus' suggestion of naming the new field
>> > 'unspecified-parameters' rather than 'argument'.
>  
> Hi Markus,
>
>> Looking again, there are more problems.
>> 
>> 1. Non-parameter argument vs. parameter argument taking unspecified
>>    parameters
>> 
>>    Example: -device takes unspecified parameters.  -cdrom doesn't take
>>    parameters, it takes a file name.  Yet, the command reports the same
>>    for both: "parameters": [], "argument": true.
>> 
>>    Looks like we need a tri-state: option takes no argument, QemuOpts
>>    argument, or other argument.
>
> '-cdrom' is the 'other argument' == 'Non-parameter argument'?

Let me clarify my terminology:

* A "parameter argument" is an option argument of the form KEY=VALUE,...
  Since parameter arguments should always be parsed with QemuOpts[*], I
  use the term "QemuOpts argument" interchangeably.

* A "non-parameter argument" or "other argument" is an option argument
  that doesn't use this form.

Does that answer your question?

> We can use a enum state.

I'm not sure I got that.

> |  { 'enum': 'ArgumentStateType',
> |    'data': ['no-argument', 'unspecified-parameters-argument',
> |             'non-parameter-argument']
> |  }
> |  
> |  { 'type': 'CommandLineOptionInfo',
> |    'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
> |              '*argument-state': 'ArgumentStateType' } }
>  
>>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
>>    recognized parameters.
>
> How about balloon? we should treat as 'taking unspecified parameters'?
>
>     "-balloon none   disable balloon device\n"
>     "-balloon virtio[,addr=str]\n"
>
> I think we can only check this from help message in qemu-options.hx,
> is it a stable/acceptable method?

-balloon is yet another sugar option:

* "-balloon none" does nothing.  It could suppress a default balloon
  device, if such a thing existed.

* "-balloon virtio,KEY=VALUE..." is sugar for "-device
  virtio-balloon,KEY=VALUE...".  Keys other than "addr" are
  undocumented.

The actual option argument parsing is ad hoc, not QemuOpts.

I sure hope something like this would not pass review today.

My advice to tools introspecting the command line is to avoid sugared
options, unless the desugaring encapsulates something they don't want to
know.

> We introduce query-command-line-options command to avoid libvirt to
> check qemu commandline information by scanning qemu's help message,
> it's not strict & stable.
>  
>> 2. Our dear friend -drive is more complicated than you might think
>> 
>>    We special-case it to report the union of drive_config_groups[],
>>    which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
>>    qemu_drive_opts.  The latter accepts unspecified parameters.
>
> I'm confused here. But there is only one option (-drive), we should
> return merged desc table (legacy + common).
>
> Desc table of qemu_drive_opts is NULL, then -drive can also take
> unspecified parameters?

Yes: driver-specific parameters.

-drive takes currently takes unspecified parameters (the driver-specific
parameters) in addition to a number of specified parameters (the common
and legacy parameters).

>>    I believe qemu_drive_opts is actually not used by the (complex!) code
>>    parsing the argument of -drive.
>> 
>>    Nevertheless, said code accepts more than just qemu_legacy_drive_opts
>>    and qemu_common_drive_opts, namely driver-specific parameters.
>> 
>>    Until we define those properly in a schema, I guess the best we can
>>    do is add one more case: option takes QemuOpts argument, but
>>    parameters is not exhaustive.
>
>
> Thanks, Amos


[*] Leftovers still parsed by other means, if any, should be converted
to QemuOpts.

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

[Amos Kong]

On Wed, Mar 26, 2014 at 02:15:18PM +0100, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
> > On Fri, Mar 07, 2014 at 10:54:09AM +0100, Markus Armbruster wrote:
> >> Eric Blake <eblake@redhat.com> writes:
> >> 
> >> > On 03/05/2014 07:36 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 macro 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 "argument" to present
> >> >> if the option takes unspecified arguments.
> >> >
> >> > I like Markus' suggestion of naming the new field
> >> > 'unspecified-parameters' rather than 'argument'.
> >  
> > Hi Markus,
> >
> >> Looking again, there are more problems.
> >> 
> >> 1. Non-parameter argument vs. parameter argument taking unspecified
> >>    parameters
> >> 
> >>    Example: -device takes unspecified parameters.  -cdrom doesn't take
> >>    parameters, it takes a file name.  Yet, the command reports the same
> >>    for both: "parameters": [], "argument": true.
> >> 
> >>    Looks like we need a tri-state: option takes no argument, QemuOpts
> >>    argument, or other argument.
> >
> > '-cdrom' is the 'other argument' == 'Non-parameter argument'?
> 
> Let me clarify my terminology:
> 
> * A "parameter argument" is an option argument of the form KEY=VALUE,...
>   Since parameter arguments should always be parsed with QemuOpts[*], I
>   use the term "QemuOpts argument" interchangeably.
> 
> * A "non-parameter argument" or "other argument" is an option argument
>   that doesn't use this form.
> 
> Does that answer your question?

Got it, thanks.
 
> > We can use a enum state.
> 
> I'm not sure I got that.
> 
> > |  { 'enum': 'ArgumentStateType',
> > |    'data': ['no-argument', 'unspecified-parameters-argument',
> > |             'non-parameter-argument']
> > |  }


        {'enum': 'ArgumentStateType',
         'data': ['no-argument', 'qemuopts-argument', 'non-param-argument']
        }

     no-argument:         -enable-kvm
     qemuopts-argument:   -boot order=c,menu=on
     non-param-argument:  -cdrom file


     I don't know if it's the tri-state you suggested in previous reply.

> > |  
> > |  { 'type': 'CommandLineOptionInfo',
> > |    'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
> > |              '*argument-state': 'ArgumentStateType' } }
> >  
> >>    parameters is [] unless it's a QemuOpts argument.  Then it lists the
> >>    recognized parameters.
> >
> > How about balloon? we should treat as 'taking unspecified parameters'?
> >
> >     "-balloon none   disable balloon device\n"
> >     "-balloon virtio[,addr=str]\n"
> >
> > I think we can only check this from help message in qemu-options.hx,
> > is it a stable/acceptable method?
> 
> -balloon is yet another sugar option:
> 
> * "-balloon none" does nothing.  It could suppress a default balloon
>   device, if such a thing existed.
> 
> * "-balloon virtio,KEY=VALUE..." is sugar for "-device
>   virtio-balloon,KEY=VALUE...".  Keys other than "addr" are
>   undocumented.
> 
> The actual option argument parsing is ad hoc, not QemuOpts.
> 
> I sure hope something like this would not pass review today.
> 
> My advice to tools introspecting the command line is to avoid sugared
> options, unless the desugaring encapsulates something they don't want to
> know.
> 
> > We introduce query-command-line-options command to avoid libvirt to
> > check qemu commandline information by scanning qemu's help message,
> > it's not strict & stable.
> >  
> >> 2. Our dear friend -drive is more complicated than you might think
> >> 
> >>    We special-case it to report the union of drive_config_groups[],
> >>    which contains qemu_legacy_drive_opts, qemu_common_drive_opts and
> >>    qemu_drive_opts.  The latter accepts unspecified parameters.
> >
> > I'm confused here. But there is only one option (-drive), we should
> > return merged desc table (legacy + common).
> >
> > Desc table of qemu_drive_opts is NULL, then -drive can also take
> > unspecified parameters?
> 
> Yes: driver-specific parameters.
> 
> -drive takes currently takes unspecified parameters (the driver-specific
> parameters) in addition to a number of specified parameters (the common
> and legacy parameters).
> 
> >>    I believe qemu_drive_opts is actually not used by the (complex!) code
> >>    parsing the argument of -drive.
> >> 
> >>    Nevertheless, said code accepts more than just qemu_legacy_drive_opts
> >>    and qemu_common_drive_opts, namely driver-specific parameters.
> >> 
> >>    Until we define those properly in a schema, I guess the best we can
> >>    do is add one more case: option takes QemuOpts argument, but
> >>    parameters is not exhaustive.
> >
> >
> > Thanks, Amos
> 
> 
> [*] Leftovers still parsed by other means, if any, should be converted
> to QemuOpts.

-- 
			Amos.

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> On Wed, Mar 26, 2014 at 02:15:18PM +0100, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
>> 
>> > On Fri, Mar 07, 2014 at 10:54:09AM +0100, Markus Armbruster wrote:
>> >> Eric Blake <eblake@redhat.com> writes:
>> >> 
>> >> > On 03/05/2014 07:36 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 macro 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 "argument" to present
>> >> >> if the option takes unspecified arguments.
>> >> >
>> >> > I like Markus' suggestion of naming the new field
>> >> > 'unspecified-parameters' rather than 'argument'.
>> >  
>> > Hi Markus,
>> >
>> >> Looking again, there are more problems.
>> >> 
>> >> 1. Non-parameter argument vs. parameter argument taking unspecified
>> >>    parameters
>> >> 
>> >>    Example: -device takes unspecified parameters.  -cdrom doesn't take
>> >>    parameters, it takes a file name.  Yet, the command reports the same
>> >>    for both: "parameters": [], "argument": true.
>> >> 
>> >>    Looks like we need a tri-state: option takes no argument, QemuOpts
>> >>    argument, or other argument.
>> >
>> > '-cdrom' is the 'other argument' == 'Non-parameter argument'?
>> 
>> Let me clarify my terminology:
>> 
>> * A "parameter argument" is an option argument of the form KEY=VALUE,...
>>   Since parameter arguments should always be parsed with QemuOpts[*], I
>>   use the term "QemuOpts argument" interchangeably.
>> 
>> * A "non-parameter argument" or "other argument" is an option argument
>>   that doesn't use this form.
>> 
>> Does that answer your question?
>
> Got it, thanks.
>  
>> > We can use a enum state.
>> 
>> I'm not sure I got that.
>> 
>> > |  { 'enum': 'ArgumentStateType',
>> > |    'data': ['no-argument', 'unspecified-parameters-argument',
>> > |             'non-parameter-argument']
>> > |  }
>
>
>         {'enum': 'ArgumentStateType',
>          'data': ['no-argument', 'qemuopts-argument', 'non-param-argument']
>         }
>
>      no-argument:         -enable-kvm
>      qemuopts-argument:   -boot order=c,menu=on
>      non-param-argument:  -cdrom file
>
>
>      I don't know if it's the tri-state you suggested in previous reply.

It is.

Maybe

    { 'enum': 'OptionArgumentKind',
      'data': ['none, 'QemuOpts, 'other'] }

The type name makes clear it's about *option* argument, and avoids
connotation with schema types or C types.  The enum value names are
short and to the point.

[...]

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> 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 macro 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 "argument" to present
> if the option takes unspecified arguments.
>
> This patch also fixes options to match their actual command-line
> spelling rather than an alternate name associated with the
> option table in use by the command.
[...]
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index d2facfd..2f89b92 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -6,6 +6,16 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +#define HAS_ARG 0x0001
> +
> +const QEMUOption qemu_options[] = {
> +    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +    { NULL },
> +};
>  
>  static QemuOptsList *vm_config_groups[32];
>  static QemuOptsList *drive_config_groups[4];
> @@ -78,6 +88,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)
>  {

Please separate functions by an empty line.

Actually, please drop get_group_index() and use existing
qemu_find_opts_err(name, NULL).

[...]