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

[Daniel Veillard]

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/