| Message ID | 1390881230-14033-6-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
[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.
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.
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));
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 >
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));
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(-)