12:53 p.m.
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
12:53 p.m.
New subject: [libvirt] [PATCH 1/5] qmp: rename query_option_descs() to get_param_infolist()
Signed-off-by: Amos Kong <akong(a)redhat.com>
---
util/qemu-config.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 9298f55..d624d92 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -39,7 +39,7 @@ QemuOptsList *qemu_find_opts(const char *group)
return ret;
}
-static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
+static CommandLineParameterInfoList *get_param_infolist(const QemuOptDesc *desc)
{
CommandLineParameterInfoList *param_list = NULL, *entry;
CommandLineParameterInfo *info;
@@ -120,9 +120,9 @@ static CommandLineParameterInfoList *get_drive_infolist(void)
for (i = 0; drive_config_groups[i] != NULL; i++) {
if (!head) {
- head = query_option_descs(drive_config_groups[i]->desc);
+ head = get_param_infolist(drive_config_groups[i]->desc);
} else {
- cur = query_option_descs(drive_config_groups[i]->desc);
+ cur = get_param_infolist(drive_config_groups[i]->desc);
connect_infolist(head, cur);
}
}
@@ -147,7 +147,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool
has_option,
info->parameters = get_drive_infolist();
} else {
info->parameters =
- query_option_descs(vm_config_groups[i]->desc);
+ get_param_infolist(vm_config_groups[i]->desc);
}
entry = g_malloc0(sizeof(*entry));
entry->value = info;
--
1.8.4.2
5:33 a.m.
New subject: [libvirt] [PATCH 1/5] qmp: rename query_option_descs() to get_param_infolist()
On 01/27/2014 08:53 PM, Amos Kong wrote:
Signed-off-by: Amos Kong <akong(a)redhat.com>
---
util/qemu-config.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:53 p.m.
New subject: [libvirt] [PATCH 2/5] introduce two marcos to dump the options info
We will use the marocs to generate two tables, which contain
the option name and argument information.
Signed-off-by: Amos Kong <akong(a)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
7:55 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH 2/5] introduce two marcos to dump the options info
Amos Kong <akong(a)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(a)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.
5:37 a.m.
New subject: [libvirt] [PATCH 2/5] introduce two marcos to dump the options info
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(a)redhat.com>
---
qemu-options-wrapper.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
Reviewed-by: Eric Blake <eblake(a)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
12:53 p.m.
New subject: [libvirt] [PATCH 3/5] query-command-line-options: query all the options in qemu-options.hx
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(a)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
9:03 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH 3/5] query-command-line-options: query all the options in qemu-options.hx
Amos Kong <akong(a)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(a)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;
5:26 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH 3/5] query-command-line-options: query all the options in qemu-options.hx
On Tue, Feb 11, 2014 at 01:03:05PM +0100, Markus Armbruster wrote:
Amos Kong <akong(a)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(a)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.
5:39 a.m.
New subject: [libvirt] [PATCH 3/5] query-command-line-options: query all the options in qemu-options.hx
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(a)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
12:53 p.m.
New subject: [libvirt] [PATCH 4/5] introduce QEMU_OPTIONS_GENERATE_HELPMSG
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(a)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
5:41 a.m.
New subject: [libvirt] [PATCH 4/5] introduce QEMU_OPTIONS_GENERATE_HELPMSG
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(a)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
12:53 p.m.
New subject: [libvirt] [PATCH 5/5] query-command-line-options: return help message for legacy options
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(a)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
9:19 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options
[Note cc: Eric]
Amos Kong <akong(a)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(a)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));
5:48 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options
On 02/11/2014 05:19 AM, Markus Armbruster wrote:
[Note cc: Eric]
Amos Kong <akong(a)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
5:49 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH 5/5] query-command-line-options: return help message for legacy options
On Tue, Feb 11, 2014 at 01:19:16PM +0100, Markus Armbruster wrote:
[Note cc: Eric]
Hi Markus,
Amos Kong <akong(a)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(a)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.
6 a.m.
New subject: [libvirt] [PATCH 5/5] query-command-line-options: return help message for legacy options
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(a)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
5:56 p.m.
New subject: [libvirt] [PATCH 5/5] query-command-line-options: return help message for legacy options
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(a)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.
5:26 a.m.
On Tue, 28 Jan 2014 11:53:45 +0800
Amos Kong <akong(a)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.
4064
days inactive
4084
days old
19 comments
4 participants
participants (4)
-
Amos Kong -
Eric Blake -
Luiz Capitulino -
Markus Armbruster