Re: [PATCH] virsh: report error if vcpu number exceed the guest maxvcpu number

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> >> ---
> >>   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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]