[libvirt] [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu number
If usr pass a vcpu which exceed guest maxvcpu number, virsh client will only output an header like this: # virsh vcpupin test3 1000 VCPU: CPU Affinity ---------------------------------- After this patch: # virsh vcpupin test3 1000 error: vcpu 1000 is out of range of cpu count 2 Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 27d62e9..681fc1a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6497,6 +6497,11 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + if (got_vcpu && vcpu >= ncpus) { + vshError(ctl, _("vcpu %d is out of range of cpu count %d"), vcpu, ncpus); + goto cleanup; + } + cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumap = vshMalloc(ctl, ncpus * cpumaplen); if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap, -- 1.8.3.1
On 06/28/2015 10:10 PM, Luyao Huang wrote:
If usr pass a vcpu which exceed guest maxvcpu number, virsh client will only output an header like this:
# virsh vcpupin test3 1000 VCPU: CPU Affinity ----------------------------------
After this patch:
# virsh vcpupin test3 1000 error: vcpu 1000 is out of range of cpu count 2
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 5 +++++ 1 file changed, 5 insertions(+)
Seemed odd that this check wasn't there - so I did some digging... Pavel's commit id '81dd81e' removed a check that seems to be what is desired in this path (or was there before his change): if (vcpu >= info.nrVirtCpu) { vshError(ctl, "%s", _("vcpupin: vCPU index out of range.")); goto cleanup; return false; } As part of this commit, you'll note there was a test change in "tests/vcpupin": # An out-of-range vCPU number deserves a diagnostic, too. $abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: vcpupin: vCPU index out of range. +error: invalid argument: requested vcpu is higher than allocated vcpus Searching on their error message lands one in test_driver.c/ testDomainPinVcpu(). So something specific for a set path, but the path you're hitting is the get path. FWIW: If a similar test was run on my system I get: # virsh vcpupin $dom 1000 0,1 error: invalid argument: vcpu 1000 is out of range of live cpu count 2 # So, if I understand everything that was done - then while your change is mostly correct, I think you could perhaps message the error similar to the vshCPUCountCollect failure (see the attached patch) Before I make that change for you - hopefully Pavel can take a look as well to be sure I haven't missed something. With any luck we this could be addressed before the 1.2.17 release, but if not since it's been a regression since 1.2.13 and no one's noticed, then another release probably won't hurt. John
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 27d62e9..681fc1a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6497,6 +6497,11 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ if (got_vcpu && vcpu >= ncpus) { + vshError(ctl, _("vcpu %d is out of range of cpu count %d"), vcpu, ncpus); + goto cleanup; + } + cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumap = vshMalloc(ctl, ncpus * cpumaplen); if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
On 07/02/2015 04:29 AM, John Ferlan wrote:
On 06/28/2015 10:10 PM, Luyao Huang wrote:
If usr pass a vcpu which exceed guest maxvcpu number, virsh client will only output an header like this:
# virsh vcpupin test3 1000 VCPU: CPU Affinity ----------------------------------
After this patch:
# virsh vcpupin test3 1000 error: vcpu 1000 is out of range of cpu count 2
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 5 +++++ 1 file changed, 5 insertions(+)
Seemed odd that this check wasn't there - so I did some digging...
Pavel's commit id '81dd81e' removed a check that seems to be what is desired in this path (or was there before his change):
if (vcpu >= info.nrVirtCpu) { vshError(ctl, "%s", _("vcpupin: vCPU index out of range.")); goto cleanup; return false; }
As part of this commit, you'll note there was a test change in "tests/vcpupin":
# An out-of-range vCPU number deserves a diagnostic, too. $abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: vcpupin: vCPU index out of range. +error: invalid argument: requested vcpu is higher than allocated vcpus
Searching on their error message lands one in test_driver.c/ testDomainPinVcpu(). So something specific for a set path, but the path you're hitting is the get path.
Yes, i noticed this and checked if need introduce a test or change the old test, but i found test driver not support get vcpupin.
FWIW: If a similar test was run on my system I get: # virsh vcpupin $dom 1000 0,1 error: invalid argument: vcpu 1000 is out of range of live cpu count 2
#
So, if I understand everything that was done - then while your change is mostly correct, I think you could perhaps message the error similar to the vshCPUCountCollect failure (see the attached patch)
I saw the attached patch, but there is some problem about check the flag (actually i had a try with check flags and output a better error before). If check flags like vshCPUCountCollect failure, there will be a problem when do not pass flag or just pass current flag to vcpupin, we will get error like this (pass a too big vcpu number): # virsh list;virsh vcpupin rhel7.0 1000 --current Id Name State ---------------------------------------------------- 3 rhel7.0 running error: vcpu 1000 is out of range of persistent cpu count 4 In this case, we output "persistent" instead of "live", this is because vshCPUCountCollect() cannot return certain flags (although there is a description say "Returns the count of vCPUs for a domain and certain flags"). So we need more check for current flags, maybe like this : diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 27d62e9..334fd3a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + if (got_vcpu && vcpu >= ncpus) { + if (flags & VIR_DOMAIN_AFFECT_LIVE || + (flags & VIR_DOMAIN_AFFECT_CURRENT && virDomainIsActive(dom) == 1)) + vshError(ctl, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, ncpus); + else + vshError(ctl, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, ncpus); + goto cleanup; + } + cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumap = vshMalloc(ctl, ncpus * cpumaplen); if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
Before I make that change for you - hopefully Pavel can take a look as well to be sure I haven't missed something.
With any luck we this could be addressed before the 1.2.17 release, but if not since it's been a regression since 1.2.13 and no one's noticed, then another release probably won't hurt.
Right, if we can fix it in 1.2.17, it will be better :) Thanks a lot for your help and review.
John
Luyao
On Thu, Jul 02, 2015 at 10:44:36AM +0800, lhuang wrote:
On 07/02/2015 04:29 AM, John Ferlan wrote:
On 06/28/2015 10:10 PM, Luyao Huang wrote:
If usr pass a vcpu which exceed guest maxvcpu number, virsh client will only output an header like this:
# virsh vcpupin test3 1000 VCPU: CPU Affinity ----------------------------------
After this patch:
# virsh vcpupin test3 1000 error: vcpu 1000 is out of range of cpu count 2
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- tools/virsh-domain.c | 5 +++++ 1 file changed, 5 insertions(+)
Seemed odd that this check wasn't there - so I did some digging...
Pavel's commit id '81dd81e' removed a check that seems to be what is desired in this path (or was there before his change):
if (vcpu >= info.nrVirtCpu) { vshError(ctl, "%s", _("vcpupin: vCPU index out of range.")); goto cleanup; return false; }
Looking at the patches, this was removed accidentally.
As part of this commit, you'll note there was a test change in "tests/vcpupin":
# An out-of-range vCPU number deserves a diagnostic, too. $abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: vcpupin: vCPU index out of range. +error: invalid argument: requested vcpu is higher than allocated vcpus
This error change is only while setting vcpu pinning.
Searching on their error message lands one in test_driver.c/ testDomainPinVcpu(). So something specific for a set path, but the path you're hitting is the get path.
Yes, i noticed this and checked if need introduce a test or change the old test, but i found test driver not support get vcpupin.
FWIW: If a similar test was run on my system I get: # virsh vcpupin $dom 1000 0,1 error: invalid argument: vcpu 1000 is out of range of live cpu count 2
#
So, if I understand everything that was done - then while your change is mostly correct, I think you could perhaps message the error similar to the vshCPUCountCollect failure (see the attached patch)
I saw the attached patch, but there is some problem about check the flag (actually i had a try with check flags and output a better error before).
If check flags like vshCPUCountCollect failure, there will be a problem when do not pass flag or just pass current flag to vcpupin, we will get error like this (pass a too big vcpu number):
# virsh list;virsh vcpupin rhel7.0 1000 --current Id Name State ---------------------------------------------------- 3 rhel7.0 running
error: vcpu 1000 is out of range of persistent cpu count 4
In this case, we output "persistent" instead of "live", this is because vshCPUCountCollect() cannot return certain flags (although there is a description say "Returns the count of vCPUs for a domain and certain flags"). So we need more check for current flags, maybe like this :
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 27d62e9..334fd3a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ if (got_vcpu && vcpu >= ncpus) { + if (flags & VIR_DOMAIN_AFFECT_LIVE || + (flags & VIR_DOMAIN_AFFECT_CURRENT && virDomainIsActive(dom) == 1)) + vshError(ctl, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, ncpus); + else + vshError(ctl, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, ncpus); + goto cleanup; + } + cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumap = vshMalloc(ctl, ncpus * cpumaplen); if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
This modification is much better and correspond to the error messages while setting the vcpu pinning.
Before I make that change for you - hopefully Pavel can take a look as well to be sure I haven't missed something.
With any luck we this could be addressed before the 1.2.17 release, but if not since it's been a regression since 1.2.13 and no one's noticed, then another release probably won't hurt.
Right, if we can fix it in 1.2.17, it will be better :)
Thanks a lot for your help and review.
ACK to the patch than John updated and proposed.
John
Luyao
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/02/2015 05:46 AM, Pavel Hrdina wrote: ...
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 27d62e9..334fd3a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ if (got_vcpu && vcpu >= ncpus) { + if (flags & VIR_DOMAIN_AFFECT_LIVE || + (flags & VIR_DOMAIN_AFFECT_CURRENT && virDomainIsActive(dom) == 1)) + vshError(ctl, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, ncpus); + else + vshError(ctl, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, ncpus); + goto cleanup; + } + cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumap = vshMalloc(ctl, ncpus * cpumaplen); if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
This modification is much better and correspond to the error messages while setting the vcpu pinning.
I just pushed this now - it'd need a bz for a backport (ahem) since 1.2.17 was cut before the push... commit 848ab685f74afae102e265108518095942ecb293 Author: Luyao Huang <lhuang@redhat.com> Date: Mon Jun 29 10:10:15 2015 +0800 virsh: report error if vcpu number exceed the guest maxvcpu number John
Before I make that change for you - hopefully Pavel can take a look as well to be sure I haven't missed something.
With any luck we this could be addressed before the 1.2.17 release, but if not since it's been a regression since 1.2.13 and no one's noticed, then another release probably won't hurt.
Right, if we can fix it in 1.2.17, it will be better :)
Thanks a lot for your help and review.
ACK to the patch than John updated and proposed.
On 07/02/2015 06:28 PM, John Ferlan wrote:
On 07/02/2015 05:46 AM, Pavel Hrdina wrote:
...
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 27d62e9..334fd3a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6497,6 +6497,19 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+ if (got_vcpu && vcpu >= ncpus) { + if (flags & VIR_DOMAIN_AFFECT_LIVE || + (flags & VIR_DOMAIN_AFFECT_CURRENT && virDomainIsActive(dom) == 1)) + vshError(ctl, + _("vcpu %d is out of range of live cpu count %d"), + vcpu, ncpus); + else + vshError(ctl, + _("vcpu %d is out of range of persistent cpu count %d"), + vcpu, ncpus); + goto cleanup; + } + cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumap = vshMalloc(ctl, ncpus * cpumaplen); if ((ncpus = virDomainGetVcpuPinInfo(dom, ncpus, cpumap,
This modification is much better and correspond to the error messages while setting the vcpu pinning.
I just pushed this now - it'd need a bz for a backport (ahem) since 1.2.17 was cut before the push...
commit 848ab685f74afae102e265108518095942ecb293 Author: Luyao Huang <lhuang@redhat.com> Date: Mon Jun 29 10:10:15 2015 +0800
virsh: report error if vcpu number exceed the guest maxvcpu number
Okay, i will help to find a bz for this patch. thanks a lot for your help and review, Pavel and John :)
John
Luyao
Before I make that change for you - hopefully Pavel can take a look as well to be sure I haven't missed something.
With any luck we this could be addressed before the 1.2.17 release, but if not since it's been a regression since 1.2.13 and no one's noticed, then another release probably won't hurt. Right, if we can fix it in 1.2.17, it will be better :)
Thanks a lot for your help and review.
ACK to the patch than John updated and proposed.
participants (4)
-
John Ferlan -
lhuang -
Luyao Huang
-
Pavel Hrdina