[libvirt] [PATCH 0/5] fix query-command-line-options
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
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
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
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
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.
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
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
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;
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.
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
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
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
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
[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));
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
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.
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
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.
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.
participants (4)
-
Amos Kong -
Eric Blake -
Luiz Capitulino -
Markus Armbruster