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

[Qemu-devel] [PATCH v5 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)
V5: rename new field, other fix (markus)

Thanks for your review!

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   |  9 +++++++--
 qemu-options.h     | 12 ++++++++++++
 util/qemu-config.c | 49 +++++++++++++++++++++++++++++++++++++++----------
 vl.c               | 19 ++-----------------
 4 files changed, 60 insertions(+), 29 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v5 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 v5 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
parameters, and all options which have no parameter 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 parameter will also be returned (eg: -enable-fips)

Some options that have parameters have a NULL desc list, some
options don't have parameters, and "parameters" is mandatory
in the past. So we add a new field "unspecified-parameters" to
present if the option takes unspecified parameters.

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   |  9 +++++++--
 qemu-options.h     | 12 ++++++++++++
 util/qemu-config.c | 43 ++++++++++++++++++++++++++++++++++++-------
 vl.c               | 19 ++-----------------
 4 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 193e7e4..fb7ca1b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4070,12 +4070,17 @@
 #
 # @option: option name
 #
-# @parameters: an array of @CommandLineParameterInfo
+# @parameters: array of @CommandLineParameterInfo, possibly empty
+# @unspecified-parameters: @optional present if the @parameters array is empty.
+#                          If true, then the option takes unspecified
+#                          parameters, if false, then the option takes no
+#                          parameter (since 2.0)
 #
 # Since 1.5
 ##
 { 'type': 'CommandLineOptionInfo',
-  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
+  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
+            '*unspecified-parameters': 'bool' } }
 
 ##
 # @query-command-line-options:
diff --git a/qemu-options.h b/qemu-options.h
index 89a009e..4024487 100644
--- a/qemu-options.h
+++ b/qemu-options.h
@@ -28,9 +28,21 @@
 #ifndef _QEMU_OPTIONS_H_
 #define _QEMU_OPTIONS_H_
 
+#include "sysemu/arch_init.h"
+
 enum {
 #define QEMU_OPTIONS_GENERATE_ENUM
 #include "qemu-options-wrapper.h"
 };
 
+#define HAS_ARG 0x0001
+
+typedef struct QEMUOption {
+    const char *name;
+    int flags;
+    int index;
+    uint32_t arch_mask;
+} QEMUOption;
+
+extern const QEMUOption qemu_options[];
 #endif
diff --git a/util/qemu-config.c b/util/qemu-config.c
index d2facfd..ea8a419 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -6,6 +6,14 @@
 #include "hw/qdev.h"
 #include "qapi/error.h"
 #include "qmp-commands.h"
+#include "qemu-options.h"
+
+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 +86,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)
 {
@@ -137,17 +156,25 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
 {
     CommandLineOptionInfoList *conf_list = NULL, *entry;
     CommandLineOptionInfo *info;
-    int i;
+    int i, idx;
 
-    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);
+
+            idx = get_group_index(qemu_options[i].name);
+
+            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_unspecified_parameters = true;
+                info->unspecified_parameters = qemu_options[i].flags & HAS_ARG;
             }
             entry = g_malloc0(sizeof(*entry));
             entry->value = info;
@@ -163,6 +190,8 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
     return conf_list;
 }
 
+#undef HAS_ARG
+
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
 {
     return find_list(vm_config_groups, group, errp);
diff --git a/vl.c b/vl.c
index 685a7a9..a967b17 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"
@@ -1990,22 +1989,6 @@ static void help(int exitcode)
     exit(exitcode);
 }
 
-#define HAS_ARG 0x0001
-
-typedef struct QEMUOption {
-    const char *name;
-    int flags;
-    int index;
-    uint32_t arch_mask;
-} QEMUOption;
-
-static const QEMUOption qemu_options[] = {
-    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
-#define QEMU_OPTIONS_GENERATE_OPTIONS
-#include "qemu-options-wrapper.h"
-    { NULL },
-};
-
 static bool vga_available(void)
 {
     return object_class_by_name("VGA") || object_class_by_name("isa-vga");
@@ -2721,6 +2704,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
     return popt;
 }
 
+#undef HAS_ARG
+
 static gpointer malloc_and_trace(gsize n_bytes)
 {
     void *ptr = malloc(n_bytes);
-- 
1.8.5.3

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

[Eric Blake]

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

On 03/06/2014 11:09 PM, Amos Kong wrote:
> vm_config_groups[] only contains part of the options which have
> parameters, and all options which have no parameter 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 parameter will also be returned (eg: -enable-fips)
> 
> Some options that have parameters have a NULL desc list, some
> options don't have parameters, and "parameters" is mandatory
> in the past. So we add a new field "unspecified-parameters" to
> present if the option takes unspecified parameters.
> 
> 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   |  9 +++++++--
>  qemu-options.h     | 12 ++++++++++++
>  util/qemu-config.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  vl.c               | 19 ++-----------------
>  4 files changed, 57 insertions(+), 26 deletions(-)

Based on the thread on v4, it sounds like this design is still not
finalized, and won't make 2.0.  It sounds like once we start exposing
all options, it would also be nice to show which options are sugar for
other spellings.

> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 193e7e4..fb7ca1b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4070,12 +4070,17 @@
>  #
>  # @option: option name
>  #
> -# @parameters: an array of @CommandLineParameterInfo
> +# @parameters: array of @CommandLineParameterInfo, possibly empty
> +# @unspecified-parameters: @optional present if the @parameters array is empty.

Blank lines between the two parameters, for consistency.

> +#                          If true, then the option takes unspecified
> +#                          parameters, if false, then the option takes no
> +#                          parameter (since 2.0)

So this will need to be changed to '(since 2.1)'.

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

[Amos Kong]

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

On Tue, Mar 11, 2014 at 06:46:10PM -0600, Eric Blake wrote:
> On 03/06/2014 11:09 PM, Amos Kong wrote:
> > vm_config_groups[] only contains part of the options which have
> > parameters, and all options which have no parameter 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 parameter will also be returned (eg: -enable-fips)
> > 
> > Some options that have parameters have a NULL desc list, some
> > options don't have parameters, and "parameters" is mandatory
> > in the past. So we add a new field "unspecified-parameters" to
> > present if the option takes unspecified parameters.
> > 
> > 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   |  9 +++++++--
> >  qemu-options.h     | 12 ++++++++++++
> >  util/qemu-config.c | 43 ++++++++++++++++++++++++++++++++++++-------
> >  vl.c               | 19 ++-----------------
> >  4 files changed, 57 insertions(+), 26 deletions(-)
 
> Based on the thread on v4, it sounds like this design is still not
> finalized, and won't make 2.0.  It sounds like once we start exposing
> all options, it would also be nice to show which options are sugar for
> other spellings.

It's not good to check if one option is sugar for other spellings from
help message. Does libvirt really need this information?

As I mentioned in V4 thread, we can add a tri-state:

+# @ArgumentStateType:
+#
+# Possible types for argument state.
+#
+# @no-argument: the option takes no argument
+#
+# @argument-with-parameters: the option takes argument with
+#                            unspecified parameters
+#
+# @argument-without-parameters: the option takes argument without
+#                               unspecified parameters
+#
+# Since 2.1
+##
+{ 'enum': 'ArgumentStateType',
+  'data': ['no-argument', 'argument-with-parameters',
+           'argument-without-parameters']
+}

I only implement the code by checking help message

  -option_name xxx,[xx=xx]\n

However, I will send a V6 later, let discuss based on it.


Thanks, Amos

> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 193e7e4..fb7ca1b 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4070,12 +4070,17 @@
> >  #
> >  # @option: option name
> >  #
> > -# @parameters: an array of @CommandLineParameterInfo
> > +# @parameters: array of @CommandLineParameterInfo, possibly empty
> > +# @unspecified-parameters: @optional present if the @parameters array is empty.
> 
> Blank lines between the two parameters, for consistency.
> 
> > +#                          If true, then the option takes unspecified
> > +#                          parameters, if false, then the option takes no
> > +#                          parameter (since 2.0)
> 
> So this will need to be changed to '(since 2.1)'.
> 
-- 
			Amos.

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