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@xxxxxxxxxx> 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@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