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

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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/