[libvirt] [PATCH] virsh-domain: Mark --live and --config mutually exclusive in vcpucount
The 'vcpucount' command is a getter command for the vCPUu count. When one or more of the filtering flags are specified the command returns the value only for the selected combination. In this case the --live and --config combination isn't valid. This however didn't cause errors as the combination of flags was rejected by the libvirt API but then the fallback code kicked in and requested the count in a way where the clash of the flags didn't matter. Mark the flag combination mutually exclusive so that user's aren't confused. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1024245 --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 59e3d8d..60abd3d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5331,6 +5331,7 @@ cmdVcpucount(vshControl *ctl, const vshCmd *cmd) if (!maximum && !active && current) current = false; + VSH_EXCLUSIVE_OPTIONS_VAR(live, config) VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); VSH_EXCLUSIVE_OPTIONS_VAR(active, maximum); -- 1.8.4.2
On 10/30/2013 07:17 AM, Peter Krempa wrote:
The 'vcpucount' command is a getter command for the vCPUu count. When one or more of the filtering flags are specified the command returns the value only for the selected combination. In this case the --live and --config combination isn't valid. This however didn't cause errors as the combination of flags was rejected by the libvirt API but then the fallback code kicked in and requested the count in a way where the clash of the flags didn't matter.
I seem to recall specifically allowing '--live --config' as a way of requesting multiple values in one command call (similar to how vcpucount with no flags grabs every possible value). I need to think about this more, and double check historical behavior, to make sure we aren't adding a regression. Please hold off until after 1.1.4 for this one (and ping me if I haven't responded with more details in a week, since I'm currently busy on another patch). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/30/13 14:56, Eric Blake wrote:
On 10/30/2013 07:17 AM, Peter Krempa wrote:
The 'vcpucount' command is a getter command for the vCPUu count. When one or more of the filtering flags are specified the command returns the value only for the selected combination. In this case the --live and --config combination isn't valid. This however didn't cause errors as the combination of flags was rejected by the libvirt API but then the fallback code kicked in and requested the count in a way where the clash of the flags didn't matter.
I seem to recall specifically allowing '--live --config' as a way of requesting multiple values in one command call (similar to how vcpucount with no flags grabs every possible value). I need to think about this more, and double check historical behavior, to make sure we aren't adding a regression. Please hold off until after 1.1.4 for this one (and ping me if I haven't responded with more details in a week, since I'm currently busy on another patch).
If you look closely at the code, you'll see that there's no option to request multiple values other than not specifying any option when a table of the cpus is printed. Also the library code explicitly forbids the combination of flags: int virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) { ... /* At most one of these two flags should be set. */ if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { virReportInvalidArg(flags, _("flags 'affect live' and 'affect config' in %s " "are mutually exclusive"), __FUNCTION__); goto error; } Peter
On 10/30/13 15:11, Peter Krempa wrote:
On 10/30/13 14:56, Eric Blake wrote:
On 10/30/2013 07:17 AM, Peter Krempa wrote:
The 'vcpucount' command is a getter command for the vCPUu count. When one or more of the filtering flags are specified the command returns the value only for the selected combination. In this case the --live and --config combination isn't valid. This however didn't cause errors as the combination of flags was rejected by the libvirt API but then the fallback code kicked in and requested the count in a way where the clash of the flags didn't matter.
I seem to recall specifically allowing '--live --config' as a way of requesting multiple values in one command call (similar to how vcpucount with no flags grabs every possible value). I need to think about this more, and double check historical behavior, to make sure we aren't adding a regression. Please hold off until after 1.1.4 for this one (and ping me if I haven't responded with more details in a week, since I'm currently busy on another patch).
Any thoughts now that the release is out?
If you look closely at the code, you'll see that there's no option to request multiple values other than not specifying any option when a table of the cpus is printed.
Also the library code explicitly forbids the combination of flags:
int virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) {
...
/* At most one of these two flags should be set. */ if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { virReportInvalidArg(flags, _("flags 'affect live' and 'affect config' in %s " "are mutually exclusive"), __FUNCTION__); goto error; }
Peter
Peter
(cc'd Eric as a gentle ping) On 11/04/13 11:00, Peter Krempa wrote:
On 10/30/13 15:11, Peter Krempa wrote:
On 10/30/13 14:56, Eric Blake wrote:
On 10/30/2013 07:17 AM, Peter Krempa wrote:
The 'vcpucount' command is a getter command for the vCPUu count. When one or more of the filtering flags are specified the command returns the value only for the selected combination. In this case the --live and --config combination isn't valid. This however didn't cause errors as the combination of flags was rejected by the libvirt API but then the fallback code kicked in and requested the count in a way where the clash of the flags didn't matter.
I seem to recall specifically allowing '--live --config' as a way of requesting multiple values in one command call (similar to how vcpucount with no flags grabs every possible value). I need to think about this more, and double check historical behavior, to make sure we aren't adding a regression. Please hold off until after 1.1.4 for this one (and ping me if I haven't responded with more details in a week, since I'm currently busy on another patch).
Any thoughts now that the release is out?
If you look closely at the code, you'll see that there's no option to request multiple values other than not specifying any option when a table of the cpus is printed.
Also the library code explicitly forbids the combination of flags:
int virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) {
...
/* At most one of these two flags should be set. */ if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { virReportInvalidArg(flags, _("flags 'affect live' and 'affect config' in %s " "are mutually exclusive"), __FUNCTION__); goto error; }
Peter
Thanks. Peter
On 11/07/2013 03:18 AM, Peter Krempa wrote:
(cc'd Eric as a gentle ping)
Thanks.
I seem to recall specifically allowing '--live --config' as a way of requesting multiple values in one command call (similar to how vcpucount with no flags grabs every possible value). I need to think about this more, and double check historical behavior, to make sure we aren't adding a regression. Please hold off until after 1.1.4 for this one (and ping me if I haven't responded with more details in a week, since I'm currently busy on another patch).
Any thoughts now that the release is out?
Let's look at the logic in an older version: git show v0.9.0:tools/virsh.c cmdVcpucount supported only --maximum, --current, --config, and --live, with mutual exclusion for maximum/current, config/live, and then a check that either no flags were present or else exactly 2 flags were present. (Weird, but that's what it was). Thus, the valid call modes were: vcpucount $dom => show four lines for all 4 combinations (with an error message in place of a line for settings not possible, such as the config lines on a transient domain) vcpucount $dom --maximum --config => show one line vcpucount $dom --maximum --live => show one line vcpucount $dom --current --config => show one line vcpucount $dom --current --live => show one line all other combinations => error 0.9.4 changed things by adding --active and repurposing --config so that it allowed back-compat use while also being more consistent with other --live/--config/--current three-way choices.
If you look closely at the code, you'll see that there's no option to request multiple values other than not specifying any option when a table of the cpus is printed.
Indeed. This is still true even in the 1.1.4 release, even where we added --active and repurposed --current. Okay, I've now played with your patch installed, and verified that the pre-0.9.4 back-compat options that are supposed to work still do. We no longer reject attempts to use just one flag (vcpucount test --live is now shorthand for vcpucount test --live --active); but that's not the fault of this patch. So it looks like all you really do is reject a three-option combination, which matches our documentation that you either omit all flags to get all lines printed, or specify at most 2 flags to print a specific line. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa