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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list