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

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

[Amos Kong]

This patchset fixed some issues of query-command-line-options:
  * some new options haven't arguments can't be queried. (eg: -enable-fips)
  * some legcy options have arguments can't be queried. (eg: -vnc display)

More discussion:
 http://marc.info/?l=qemu-devel&m=139081830416684&w=2

Amos Kong (5):
  qmp: rename query_option_descs() to get_param_infolist()
  introduce two marcos to dump the options info
  query-command-line-options: query all the options in qemu-options.hx
  introduce QEMU_OPTIONS_GENERATE_HELPMSG
  query-command-line-options: return help message for legacy options

 qapi-schema.json       |  5 ++++-
 qemu-options-wrapper.h | 27 +++++++++++++++++++++++++++
 util/qemu-config.c     | 50 +++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 72 insertions(+), 10 deletions(-)

-- 
1.8.4.2

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

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
---
 util/qemu-config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 9298f55..d624d92 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -39,7 +39,7 @@ QemuOptsList *qemu_find_opts(const char *group)
     return ret;
 }
 
-static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
+static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc *desc)
 {
     CommandLineParameterInfoList *param_list = NULL, *entry;
     CommandLineParameterInfo *info;
@@ -120,9 +120,9 @@ static CommandLineParameterInfoList *get_drive_infolist(void)
 
     for (i = 0; drive_config_groups[i] != NULL; i++) {
         if (!head) {
-            head = query_option_descs(drive_config_groups[i]->desc);
+            head = get_param_infolist(drive_config_groups[i]->desc);
         } else {
-            cur = query_option_descs(drive_config_groups[i]->desc);
+            cur = get_param_infolist(drive_config_groups[i]->desc);
             connect_infolist(head, cur);
         }
     }
@@ -147,7 +147,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
                 info->parameters = get_drive_infolist();
             } else {
                 info->parameters =
-                    query_option_descs(vm_config_groups[i]->desc);
+                    get_param_infolist(vm_config_groups[i]->desc);
             }
             entry = g_malloc0(sizeof(*entry));
             entry->value = info;
-- 
1.8.4.2

[Qemu-devel] [PATCH 2/5] introduce two marcos to dump the options info

[Amos Kong]

We will use the marocs to generate two tables, which contain
the option name and argument information.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qemu-options-wrapper.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qemu-options-wrapper.h b/qemu-options-wrapper.h
index 13bfea0..c36a9ee 100644
--- a/qemu-options-wrapper.h
+++ b/qemu-options-wrapper.h
@@ -18,6 +18,22 @@
 
 #define DEFHEADING(text) ARCHHEADING(text, QEMU_ARCH_ALL)
 
+#elif defined(QEMU_OPTIONS_GENERATE_NAME)
+
+#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)    \
+    option,
+
+#define DEFHEADING(text)
+#define ARCHHEADING(text, arch_mask)
+
+#elif defined(QEMU_OPTIONS_GENERATE_HASARG)
+
+#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)    \
+    stringify(opt_arg),
+
+#define DEFHEADING(text)
+#define ARCHHEADING(text, arch_mask)
+
 #elif defined(QEMU_OPTIONS_GENERATE_OPTIONS)
 
 #define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)     \
@@ -38,4 +54,6 @@
 
 #undef QEMU_OPTIONS_GENERATE_ENUM
 #undef QEMU_OPTIONS_GENERATE_HELP
+#undef QEMU_OPTIONS_GENERATE_NAME
+#undef QEMU_OPTIONS_GENERATE_HASPARAM
 #undef QEMU_OPTIONS_GENERATE_OPTIONS
-- 
1.8.4.2

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

[Amos Kong]

vm_config_groups[] contain the options which have parameter, but some
legcy options haven't been added to vm_config_groups[].

All the options can be found in qemu-options.hx, this patch used two
new marcos to generate two tables, we can check if the option name is
valid and if the option has arguments.

This patch also try to visit all the options in option_names[], then
we won't lost the legacy options that weren't added to vm_config_groups[].
The options have no arguments will also be returned (eg: -enable-fips)

Signed-off-by: Amos Kong <akong@redhat.com>
---
 util/qemu-config.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index d624d92..de233d8 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -78,6 +78,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,16 +150,34 @@ 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)) {
+    char const *option_names[] = {
+#define QEMU_OPTIONS_GENERATE_NAME
+#include "qemu-options-wrapper.h"
+    };
+
+    char const *option_hasargs[] = {
+#define QEMU_OPTIONS_GENERATE_HASARG
+#include "qemu-options-wrapper.h"
+    };
+
+    for (i = 0; i < sizeof(option_names) / sizeof(char *); i++) {
+        if (!has_option || !strcmp(option, option_names[i])) {
             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(option_names[i]);
+
+            int idx = get_group_index(option_names[i]);
+
+            if (!strcmp("HAS_ARG", option_hasargs[i])) {
+                info->has_parameters = true;
+            }
+
+            if (!strcmp("drive", option_names[i])) {
                 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);
             }
+
             entry = g_malloc0(sizeof(*entry));
             entry->value = info;
             entry->next = conf_list;
-- 
1.8.4.2

[Qemu-devel] [PATCH 4/5] introduce QEMU_OPTIONS_GENERATE_HELPMSG

[Amos Kong]

This patch introduced a new maroc, it will be used to dump the help
messages of all the options.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qemu-options-wrapper.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/qemu-options-wrapper.h b/qemu-options-wrapper.h
index c36a9ee..bb0ac87 100644
--- a/qemu-options-wrapper.h
+++ b/qemu-options-wrapper.h
@@ -34,6 +34,14 @@
 #define DEFHEADING(text)
 #define ARCHHEADING(text, arch_mask)
 
+#elif defined(QEMU_OPTIONS_GENERATE_HELPMSG)
+
+#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)    \
+    stringify(opt_help),
+
+#define DEFHEADING(text)
+#define ARCHHEADING(text, arch_mask)
+
 #elif defined(QEMU_OPTIONS_GENERATE_OPTIONS)
 
 #define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)     \
@@ -56,4 +64,5 @@
 #undef QEMU_OPTIONS_GENERATE_HELP
 #undef QEMU_OPTIONS_GENERATE_NAME
 #undef QEMU_OPTIONS_GENERATE_HASPARAM
+#undef QEMU_OPTIONS_GENERATE_HELPMSG
 #undef QEMU_OPTIONS_GENERATE_OPTIONS
-- 
1.8.4.2

[Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options

[Amos Kong]

Some legacy options that have arguments weren't added to
vm_config_groups[], so query-command-line-options returns a
NULL parameters infolist. This patch try to return help message
for this kind of legacy options.

Example:
 {
     "helpmsg": "\"-vnc display    start a VNC server on display\\n\"",
     "parameters": [
     ],
     "option": "vnc"
 },

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qapi-schema.json   | 5 ++++-
 util/qemu-config.c | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 05ced9d..b3e6f46 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3943,13 +3943,16 @@
 # Details about a command line option, including its list of parameter details
 #
 # @option: option name
+# @helpmsg: help message for legacy options
 #
 # @parameters: an array of @CommandLineParameterInfo
 #
 # Since 1.5
 ##
 { 'type': 'CommandLineOptionInfo',
-  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
+  'data': { 'option': 'str',
+            '*parameters': ['CommandLineParameterInfo'],
+            '*helpmsg': 'str' } }
 
 ##
 # @query-command-line-options:
diff --git a/util/qemu-config.c b/util/qemu-config.c
index de233d8..a2def03 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -176,6 +176,9 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
             } else if (idx >= 0) {
                 info->parameters =
                     get_param_infolist(vm_config_groups[idx]->desc);
+            } else if (info->has_parameters) {
+                info->has_helpmsg = true;
+                info->helpmsg = g_strdup(option_helpmsgs[i]);
             }
 
             entry = g_malloc0(sizeof(*entry));
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH 0/5] fix query-command-line-options

[Luiz Capitulino]

On Tue, 28 Jan 2014 11:53:45 +0800
Amos Kong <akong@redhat.com> wrote:

> This patchset fixed some issues of query-command-line-options:
>   * some new options haven't arguments can't be queried. (eg: -enable-fips)
>   * some legcy options have arguments can't be queried. (eg: -vnc display)

Markus, you're more aware of query-command-line-options' than I am. Can
you please review this series?

Thanks.

Re: [Qemu-devel] [PATCH 2/5] introduce two marcos to dump the options info

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> We will use the marocs to generate two tables, which contain
> the option name and argument information.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qemu-options-wrapper.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/qemu-options-wrapper.h b/qemu-options-wrapper.h
> index 13bfea0..c36a9ee 100644
> --- a/qemu-options-wrapper.h
> +++ b/qemu-options-wrapper.h
> @@ -18,6 +18,22 @@
>  
>  #define DEFHEADING(text) ARCHHEADING(text, QEMU_ARCH_ALL)
>  
> +#elif defined(QEMU_OPTIONS_GENERATE_NAME)
> +
> +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)    \
> +    option,
> +
> +#define DEFHEADING(text)
> +#define ARCHHEADING(text, arch_mask)
> +
> +#elif defined(QEMU_OPTIONS_GENERATE_HASARG)
> +
> +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)    \
> +    stringify(opt_arg),
> +
> +#define DEFHEADING(text)
> +#define ARCHHEADING(text, arch_mask)
> +
>  #elif defined(QEMU_OPTIONS_GENERATE_OPTIONS)
>  
>  #define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)     \
> @@ -38,4 +54,6 @@
>  
>  #undef QEMU_OPTIONS_GENERATE_ENUM
>  #undef QEMU_OPTIONS_GENERATE_HELP
> +#undef QEMU_OPTIONS_GENERATE_NAME
> +#undef QEMU_OPTIONS_GENERATE_HASPARAM
>  #undef QEMU_OPTIONS_GENERATE_OPTIONS

Personally, I find this indirection through qemu-options-wrapper.h
silly.  The ultimate user could just as well define the necessary DEF
macros itself, instead of telling a wrapper what it wants defined by
defining yet another macro.

Not your fault, and I'm not demanding you change it.

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> vm_config_groups[] contain the options which have parameter, but some
> legcy options haven't been added to vm_config_groups[].
>
> All the options can be found in qemu-options.hx, this patch used two
> new marcos to generate two tables, we can check if the option name is
> valid and if the option has arguments.
>
> This patch also try to visit all the options in option_names[], then
> we won't lost the legacy options that weren't added to vm_config_groups[].
> The options have no arguments will also be returned (eg: -enable-fips)
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  util/qemu-config.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index d624d92..de233d8 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -78,6 +78,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,16 +150,34 @@ 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)) {
> +    char const *option_names[] = {
> +#define QEMU_OPTIONS_GENERATE_NAME
> +#include "qemu-options-wrapper.h"
> +    };
> +
> +    char const *option_hasargs[] = {
> +#define QEMU_OPTIONS_GENERATE_HASARG
> +#include "qemu-options-wrapper.h"
> +    };

Both tables are technically redundant.  The same information is already
in vl.c's qemu_options[].  That one also includes -h, which the tables
here miss.

Duplicating tables can be okay, but I suspect using the existing one
would be simpler.  Have you tried?

> +
> +    for (i = 0; i < sizeof(option_names) / sizeof(char *); i++) {
> +        if (!has_option || !strcmp(option, option_names[i])) {
>              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(option_names[i]);
> +
> +            int idx = get_group_index(option_names[i]);

Variable declaration follows statement.  Please declare int idx at the
beginning of a block.  I'd declare it right at the beginning, together
with int i.

> +
> +            if (!strcmp("HAS_ARG", option_hasargs[i])) {
> +                info->has_parameters = true;
> +            }

qemu/util/qemu-config.c:171:21: error: ‘CommandLineOptionInfo’ has no member named ‘has_parameters’

Did you forget to include a schema change?

Awfully roundabout way to test whether the option takes an argument.
Here's the other part, from PATCH 2/5:

  +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)    \
  +    stringify(opt_arg),

Please do it more like vl.c:

   #define HAS_ARG true
       bool option_has_arg[] = {
   #define QEMU_OPTIONS_GENERATE_HASARG
       #include "qemu-options-wrapper.h"
   }

Then you can simply test option_has_arg[i].

Or reuse vl.c's table.

> +
> +            if (!strcmp("drive", option_names[i])) {
>                  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);
>              }
> +
>              entry = g_malloc0(sizeof(*entry));
>              entry->value = info;
>              entry->next = conf_list;

Re: [Qemu-devel] [PATCH 4/5] introduce QEMU_OPTIONS_GENERATE_HELPMSG

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> This patch introduced a new maroc, it will be used to dump the help
> messages of all the options.

"macro".  Also misspelled in PATCH 2/5.

Re: [Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options

[Markus Armbruster]

[Note cc: Eric]

Amos Kong <akong@redhat.com> writes:

> Some legacy options that have arguments weren't added to
> vm_config_groups[], so query-command-line-options returns a
> NULL parameters infolist. This patch try to return help message
> for this kind of legacy options.
>
> Example:
>  {
>      "helpmsg": "\"-vnc display    start a VNC server on display\\n\"",
>      "parameters": [
>      ],
>      "option": "vnc"
>  },

Do we have prospective users for this feature?

> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json   | 5 ++++-
>  util/qemu-config.c | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..b3e6f46 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3943,13 +3943,16 @@
>  # Details about a command line option, including its list of parameter details
>  #
>  # @option: option name
> +# @helpmsg: help message for legacy options
>  #
>  # @parameters: an array of @CommandLineParameterInfo
>  #
>  # Since 1.5
>  ##
>  { 'type': 'CommandLineOptionInfo',
> -  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
> +  'data': { 'option': 'str',
> +            '*parameters': ['CommandLineParameterInfo'],
> +            '*helpmsg': 'str' } }
>  
>  ##
>  # @query-command-line-options:

Aha, here's the schema change missing in PATCH 3/5.

The schema needs to cover these kinds of options:

1. No argument

   { 'option': OPT-NAME }

2. QemuOpts argument

2a. with known parameters (QemuOptsList member desc[] not empty)

   { 'option': OPT-NAME,
     'parameters': [ { 'name': PAR-NAME, 'type': PAR-TYPE }, ... ] }

2b. with unknown parameters (desc[] empty)

   { 'option': OPT-NAME, parameters: [] }

3. Other argument

   { 'option': OPT-NAME, ? }

This patch adds 3.  We need to decide what we want there.

Any particular reason why we have to invent something new, and can't
simply fold it into 2b?

Note that I completely left out help strings, both on the option and on
the parameter level.  Both for brevity of presentation, and because I'd
like to see a use before we add them.

> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index de233d8..a2def03 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -176,6 +176,9 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>              } else if (idx >= 0) {
>                  info->parameters =
>                      get_param_infolist(vm_config_groups[idx]->desc);
> +            } else if (info->has_parameters) {
> +                info->has_helpmsg = true;
> +                info->helpmsg = g_strdup(option_helpmsgs[i]);
>              }
>  
>              entry = g_malloc0(sizeof(*entry));

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

[Eric Blake]

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

On 01/27/2014 08:53 PM, Amos Kong wrote:
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  util/qemu-config.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 2/5] introduce two marcos to dump the options info

[Eric Blake]

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

On 01/27/2014 08:53 PM, Amos Kong wrote:

s/marcos/macros/ in the subject line

> We will use the marocs to generate two tables, which contain

s/marocs/macros/

(wow, two different ways to mis-spell the word)

> the option name and argument information.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qemu-options-wrapper.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

although I'd probably squash this with the patch that actually defines
QEMU_OPTIONS_GENERATE_{NAME,HASARG}, as this patch is useless on its own.

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

[Eric Blake]

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

On 01/27/2014 08:53 PM, Amos Kong wrote:
> vm_config_groups[] contain the options which have parameter, but some
> legcy options haven't been added to vm_config_groups[].

s/legcy/legacy/

> 
> All the options can be found in qemu-options.hx, this patch used two
> new marcos to generate two tables, we can check if the option name is

s/marcos/macros/

> valid and if the option has arguments.
> 
> This patch also try to visit all the options in option_names[], then
> we won't lost the legacy options that weren't added to vm_config_groups[].

s/lost/lose/

> The options have no arguments will also be returned (eg: -enable-fips)

s/options/options that/

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  util/qemu-config.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 

-- 
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 4/5] introduce QEMU_OPTIONS_GENERATE_HELPMSG

[Eric Blake]

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

On 01/27/2014 08:53 PM, Amos Kong wrote:
> This patch introduced a new maroc, it will be used to dump the help
> messages of all the options.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qemu-options-wrapper.h | 9 +++++++++
>  1 file changed, 9 insertions(+)

Again, this patch is useless on its own, I'd squash it with the first
client that defines QEMU_OPTIONS_GENERATE_HELPMSG.

-- 
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 5/5] query-command-line-options: return help message for legacy options

[Eric Blake]

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

On 02/11/2014 05:19 AM, Markus Armbruster wrote:
> [Note cc: Eric]
> 
> Amos Kong <akong@redhat.com> writes:
> 
>> Some legacy options that have arguments weren't added to
>> vm_config_groups[], so query-command-line-options returns a
>> NULL parameters infolist. This patch try to return help message
>> for this kind of legacy options.
>>
>> Example:
>>  {
>>      "helpmsg": "\"-vnc display    start a VNC server on display\\n\"",
>>      "parameters": [
>>      ],
>>      "option": "vnc"
>>  },
> 
> Do we have prospective users for this feature?

Libvirt probably won't care about helpmsg other than the fact that it
gets logged as part of the QMP reply, and the log is more legible if
human-readable text is included.  I don't care if you add it or omit it;
the REAL change here is...

>>  ##
>>  { 'type': 'CommandLineOptionInfo',
>> -  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
>> +  'data': { 'option': 'str',
>> +            '*parameters': ['CommandLineParameterInfo'],

changing parameters from mandatory to optional, and omitting it when
supplying information on an arg-less option.  And that is what libvirt
wants, for learning about things like -enable-fips.

>> +            '*helpmsg': 'str' } }
>>  
>>  ##
>>  # @query-command-line-options:
> 
> Aha, here's the schema change missing in PATCH 3/5.

Indeed; please resubmit the series so that every patch builds on its own.

> 
> The schema needs to cover these kinds of options:
> 
> 1. No argument
> 
>    { 'option': OPT-NAME }

This is newly allowed by your schema change.

> 
> 2. QemuOpts argument
> 
> 2a. with known parameters (QemuOptsList member desc[] not empty)
> 
>    { 'option': OPT-NAME,
>      'parameters': [ { 'name': PAR-NAME, 'type': PAR-TYPE }, ... ] }

Existing before your patch.

> 
> 2b. with unknown parameters (desc[] empty)
> 
>    { 'option': OPT-NAME, parameters: [] }

Existing before your patch.

> 
> 3. Other argument
> 
>    { 'option': OPT-NAME, ? }
> 
> This patch adds 3.  We need to decide what we want there.
> 
> Any particular reason why we have to invent something new, and can't
> simply fold it into 2b?

Or even into 1?  That is, the presence of 'parameters' is a witness of
whether a flag is boolean (-enable-fips) or takes arguments (-device);
then the length of the 'parameters' array is a witness of whether we
know all option arguments (modern code) or have unknown parameters
(older options that have not yet been converted to modern form).

> 
> Note that I completely left out help strings, both on the option and on
> the parameter level.  Both for brevity of presentation, and because I'd
> like to see a use before we add them.

Libvirt won't be hurt if you don't present help strings.

-- 
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 5/5] query-command-line-options: return help message for legacy options

[Eric Blake]

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

On 01/27/2014 08:53 PM, Amos Kong wrote:
> Some legacy options that have arguments weren't added to
> vm_config_groups[], so query-command-line-options returns a
> NULL parameters infolist. This patch try to return help message
> for this kind of legacy options.
> 
> Example:
>  {
>      "helpmsg": "\"-vnc display    start a VNC server on display\\n\"",
>      "parameters": [
>      ],
>      "option": "vnc"
>  },
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json   | 5 ++++-
>  util/qemu-config.c | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..b3e6f46 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3943,13 +3943,16 @@
>  # Details about a command line option, including its list of parameter details
>  #
>  # @option: option name
> +# @helpmsg: help message for legacy options

Missing "#optional" and "(since 2.0)" designations

>  #
>  # @parameters: an array of @CommandLineParameterInfo

Might be worth documenting "#optional since 2.0" (we don't yet have good
precedent for documenting when a formerly mandatory field became optional).

Groan.  This is an output struct.  On input structs, changing a
mandatory field to optional is safe - old callers will always supply the
field.  But on output structs, changing a mandatory field to optional is
backwards-incompatible.  Old callers may be blindly expecting the field,
and crash when it is not present.

Your approach needs to be modified.

>  #
>  # Since 1.5
>  ##
>  { 'type': 'CommandLineOptionInfo',
> -  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
> +  'data': { 'option': 'str',
> +            '*parameters': ['CommandLineParameterInfo'],
> +            '*helpmsg': 'str' } }

I suggest:


# @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)

{ 'type': 'CommandLineOptionInfo',
'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
          '*argument': 'bool' } }

used as:
[
  { "option":"enable-fips", "parameters":[], "argument":false },
  { "option":"smbios", "parameters":[], "argument":true },
  { "option":"iscsi", "paramters":[ ... ] },
  ...
]

which adequately differentiates between -iscsi taking arguments (but
where we haven't yet hooked it in to introspect those arguments) vs.
-enable-fips being boolean.

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

[Amos Kong]

On Tue, Feb 11, 2014 at 01:03:05PM +0100, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
> > vm_config_groups[] contain the options which have parameter, but some
> > legcy options haven't been added to vm_config_groups[].
> >
> > All the options can be found in qemu-options.hx, this patch used two
> > new marcos to generate two tables, we can check if the option name is
> > valid and if the option has arguments.
> >
> > This patch also try to visit all the options in option_names[], then
> > we won't lost the legacy options that weren't added to vm_config_groups[].
> > The options have no arguments will also be returned (eg: -enable-fips)
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  util/qemu-config.c | 41 +++++++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/util/qemu-config.c b/util/qemu-config.c
> > index d624d92..de233d8 100644
> > --- a/util/qemu-config.c
> > +++ b/util/qemu-config.c
> > @@ -78,6 +78,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,16 +150,34 @@ 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)) {
> > +    char const *option_names[] = {
> > +#define QEMU_OPTIONS_GENERATE_NAME
> > +#include "qemu-options-wrapper.h"
> > +    };
> > +
> > +    char const *option_hasargs[] = {
> > +#define QEMU_OPTIONS_GENERATE_HASARG
> > +#include "qemu-options-wrapper.h"
> > +    };
> 
> Both tables are technically redundant.  The same information is already
> in vl.c's qemu_options[].  That one also includes -h, which the tables
> here miss.
> 
> Duplicating tables can be okay, but I suspect using the existing one
> would be simpler.  Have you tried?

Right, it's redundant work.
 
> > +
> > +    for (i = 0; i < sizeof(option_names) / sizeof(char *); i++) {
> > +        if (!has_option || !strcmp(option, option_names[i])) {
> >              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(option_names[i]);
> > +
> > +            int idx = get_group_index(option_names[i]);
> 
> Variable declaration follows statement.  Please declare int idx at the
> beginning of a block.  I'd declare it right at the beginning, together
> with int i.
 
Ok

> > +
> > +            if (!strcmp("HAS_ARG", option_hasargs[i])) {
> > +                info->has_parameters = true;
> > +            }
> 
> qemu/util/qemu-config.c:171:21: error: ‘CommandLineOptionInfo’ has no member named ‘has_parameters’
> 
> Did you forget to include a schema change?

Schema change was wrongly included to patch 5/5.
 
> Awfully roundabout way to test whether the option takes an argument.
> Here's the other part, from PATCH 2/5:
> 
>   +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)    \
>   +    stringify(opt_arg),
> 
> Please do it more like vl.c:
> 
>    #define HAS_ARG true
>        bool option_has_arg[] = {
>    #define QEMU_OPTIONS_GENERATE_HASARG
>        #include "qemu-options-wrapper.h"
>    }

I touched an error in the past, so convert the has_arg to string.

| In file included from util/qemu-config.c:160:0:
| ./qemu-options.def: In function ‘qmp_query_command_line_options’:
| ./qemu-options.def:10:16: error: ‘HAS_ARG’ undeclared (first use in this function)
|  DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
|                 ^
| ./qemu-options-wrapper.h:32:5: note: in definition of macro ‘DEF’
|      opt_arg,
|      ^
| ./qemu-options.def:10:16: note: each undeclared identifier is reported only once for each function it appears in
|  DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
|                 ^
| ./qemu-options-wrapper.h:32:5: note: in definition of macro ‘DEF’
|      opt_arg,
|      ^

This problem can be solved by defining HAS_ARG in qemu-config.c as in vl.c
+ #define HAS_ARG 0x0001
 
> Then you can simply test option_has_arg[i].
> 
> Or reuse vl.c's table.

OK.
 
> > +
> > +            if (!strcmp("drive", option_names[i])) {
> >                  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);
> >              }
> > +
> >              entry = g_malloc0(sizeof(*entry));
> >              entry->value = info;
> >              entry->next = conf_list;

-- 
			Amos.

Re: [Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options

[Amos Kong]

On Tue, Feb 11, 2014 at 01:19:16PM +0100, Markus Armbruster wrote:
> [Note cc: Eric]
 
Hi Markus,

> Amos Kong <akong@redhat.com> writes:
> 
> > Some legacy options that have arguments weren't added to
> > vm_config_groups[], so query-command-line-options returns a
> > NULL parameters infolist. This patch try to return help message
> > for this kind of legacy options.
> >
> > Example:
> >  {
> >      "helpmsg": "\"-vnc display    start a VNC server on display\\n\"",
> >      "parameters": [
> >      ],
> >      "option": "vnc"
> >  },
> 
> Do we have prospective users for this feature?

I had posted a RFC mail to discuss about "fix/redo query-command-line-options",
this patch is a solution for legacy options that have not arguments.

Current QEMU returns NULL list for this kind of options, this patch
tries to return option name and help message to the libvirt. (it's
better)

You can find some examples in the end.
 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  qapi-schema.json   | 5 ++++-
> >  util/qemu-config.c | 3 +++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 05ced9d..b3e6f46 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3943,13 +3943,16 @@
> >  # Details about a command line option, including its list of parameter details
> >  #
> >  # @option: option name
> > +# @helpmsg: help message for legacy options
> >  #
> >  # @parameters: an array of @CommandLineParameterInfo
> >  #
> >  # Since 1.5
> >  ##
> >  { 'type': 'CommandLineOptionInfo',
> > -  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
> > +  'data': { 'option': 'str',
> > +            '*parameters': ['CommandLineParameterInfo'],
> > +            '*helpmsg': 'str' } }
> >  
> >  ##
> >  # @query-command-line-options:
> 
> Aha, here's the schema change missing in PATCH 3/5.

Yeah

> The schema needs to cover these kinds of options:
> 
> 1. No argument
> 
>    { 'option': OPT-NAME }
> 
> 2. QemuOpts argument
> 
> 2a. with known parameters (QemuOptsList member desc[] not empty)
> 
>    { 'option': OPT-NAME,
>      'parameters': [ { 'name': PAR-NAME, 'type': PAR-TYPE }, ... ] }
> 
> 2b. with unknown parameters (desc[] empty)
> 
>    { 'option': OPT-NAME, parameters: [] }
> 
> 3. Other argument
> 
>    { 'option': OPT-NAME, ? }
> 
> This patch adds 3.  We need to decide what we want there.
> 
> Any particular reason why we have to invent something new, and can't
> simply fold it into 2b?

I thinks it's ok, it's just the output format, then the 'parameters'
isn't optional.
 
> Note that I completely left out help strings, both on the option and on
> the parameter level.  Both for brevity of presentation, and because I'd
> like to see a use before we add them.

Something was _wrong_ in this patch, I want to additionally output
helpmsg only for this case:
  If option has parameter, and we didn't convert the option to use
  QemuOpts (with good desc table), then help message is helpful.
  This only effects some legacy options.

|-set group.id.arg=value
|                set <arg> parameter for item <id> of type <group>
|                i.e. -set drive.$id.file=/path/to/image
|-qtest-log /dev/null
|-qtest 
|-writeconfig <file>
|              read/write config file
|....
|....

 
> > diff --git a/util/qemu-config.c b/util/qemu-config.c
> > index de233d8..a2def03 100644
> > --- a/util/qemu-config.c
> > +++ b/util/qemu-config.c
> > @@ -176,6 +176,9 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
> >              } else if (idx >= 0) {
> >                  info->parameters =
> >                      get_param_infolist(vm_config_groups[idx]->desc);
> > +            } else if (info->has_parameters) {
> > +                info->has_helpmsg = true;
> > +                info->helpmsg = g_strdup(option_helpmsgs[i]);
> >              }
> >  
> >              entry = g_malloc0(sizeof(*entry));

-- 
			Amos.

Re: [Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options

[Amos Kong]

On Wed, Feb 12, 2014 at 02:00:51PM -0700, Eric Blake wrote:
> On 01/27/2014 08:53 PM, Amos Kong wrote:
> > Some legacy options that have arguments weren't added to
> > vm_config_groups[], so query-command-line-options returns a
> > NULL parameters infolist. This patch try to return help message
> > for this kind of legacy options.
> > 
> > Example:
> >  {
> >      "helpmsg": "\"-vnc display    start a VNC server on display\\n\"",
> >      "parameters": [
> >      ],
> >      "option": "vnc"
> >  },
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  qapi-schema.json   | 5 ++++-
> >  util/qemu-config.c | 3 +++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 05ced9d..b3e6f46 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3943,13 +3943,16 @@
> >  # Details about a command line option, including its list of parameter details
> >  #
> >  # @option: option name
> > +# @helpmsg: help message for legacy options
> 
> Missing "#optional" and "(since 2.0)" designations
> 
> >  #
> >  # @parameters: an array of @CommandLineParameterInfo
> 
> Might be worth documenting "#optional since 2.0" (we don't yet have good
> precedent for documenting when a formerly mandatory field became optional).
> 
> Groan.  This is an output struct.  On input structs, changing a
> mandatory field to optional is safe - old callers will always supply the
> field.  But on output structs, changing a mandatory field to optional is
> backwards-incompatible.  Old callers may be blindly expecting the field,
> and crash when it is not present.
> 
> Your approach needs to be modified.
> 
> >  #
> >  # Since 1.5
> >  ##
> >  { 'type': 'CommandLineOptionInfo',
> > -  'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } }
> > +  'data': { 'option': 'str',
> > +            '*parameters': ['CommandLineParameterInfo'],
> > +            '*helpmsg': 'str' } }
> 
> I suggest:
> 
> 
> # @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)
> 
> { 'type': 'CommandLineOptionInfo',
> 'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'],
>           '*argument': 'bool' } }
> 
> used as:
> [
>   { "option":"enable-fips", "parameters":[], "argument":false },
>   { "option":"smbios", "parameters":[], "argument":true },
>   { "option":"iscsi", "paramters":[ ... ] },
>   ...
> ]
 
It's ok to use a split "argument" to indicate if option has
parameters. and the parameters will not be optional, it will
be NULL list in the case of no parameters.

Thanks.

> which adequately differentiates between -iscsi taking arguments (but
> where we haven't yet hooked it in to introspect those arguments) vs.
> -enable-fips being boolean.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


-- 
			Amos.