[Libvir] [patch] virsh setvcpus range check for negative value case

[Atsushi SAKAI]

Hi, Daniel

　Thank you for your reviewing.
I agree your fixes.

Also I agree this issue should be handled by hypervisor.
But for Xen, if # of vcpus are out of range, 
XEN_DOMCTL_setvcpu_context return the -EINVAL.
So the inactive domain cannot boot.

For this circumstances,
it is better to handle # of vcpus error by libvirt.

c.f.
Then I go to Next Bug fixes.

Thanks
Atsushi SAKAI



Daniel Veillard <veillard at redhat.com> wrote:

> On Wed, Aug 15, 2007 at 05:01:04PM +0900, Atsushi SAKAI wrote:
> > Hi,
> > 
> > This patch adds virsh setvcpus range check for negative value case.
> > 
> > for example 
> > to the inactive domain 
> > virsh setvcpus -1
> > sets vcpus=4294967295
> > And cannot boot the inactive domain.
> 
>   I would rather change the test 
> 
>    if (!count) {
> 
> to
> 
>    if (count <= 0) {
> 
> rather than use the unsigned cast to catch it.
> 
> There is 2 things to note:
> - virDomainSetVcpus actually do a check but since the argument is an 
>   unsigned int we have a problem
>         if (nvcpus < 1) {
> 	    virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
> 	    return (-1);
> 	}
>   I would be tempted to do an (internal ?)
>         #define MAX_VCPUS 4096
>   and change that check to 
>         if ((nvcpus < 1) || (nvcpus > MAX_VCPUS)) {
>   to guard at the API against unreasonnable values.
> 
> - There is actually a bug a few lines down in virsh, when checking for the
>   maximum number of CPUs for the domain:
>     maxcpu = virDomainGetMaxVcpus(dom);
>     if (!maxcpu) {
>   as -1 is the error values for the call. so the test there really ought to be 
>     if (maxcpu <= 0)
>   one could argue that 0 should be the error value returned by
>   virDomainGetMaxVcpus but since it's defined as -1 in the API, the test
>   must be fixed.
> 
>  I have made the 2 changes to virsh but not the one to virDomainSetVcpus
> where it could be argued it's the hypervisor responsability to check the
> given value. Opinions ?
> 
>   Thanks for raising the problem !
> 
> Daniel
> 
> -- 
> Red Hat Virtualization group http://redhat.com/virtualization/
> Daniel Veillard      | virtualization library  http://libvirt.org/
> veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
> http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/