8:17 a.m.
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
8:56 a.m.
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
9:11 a.m.
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
4 a.m.
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
4:18 a.m.
(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
12:11 p.m.
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
4007
days inactive
4019
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa