[Libvir] [patch] virsh setvcpus range check for negative value case
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. Signed-off-by: Atsushi SAKAI <sakaia@jp.fujitsu.com> Thanks Atsushi SAKAI
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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@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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (2)
-
Atsushi SAKAI -
Daniel Veillard