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(a)redhat.com&gt; --- 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,

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(a)redhat.com&gt; >> --- >> 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 -- libvir-list mailing list libvir-list(a)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(a)redhat.com&gt; 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. >