[virt-tools-list] [PATCH] python-virtinst: type inconsistencies in 'vcpus'

[Satoru SATOH]

Hi,

On Wed, Feb 09, 2011 at 12:40:18PM -0500, Cole Robinson wrote:
(snip)
> Thanks for the detailed report. FYI in the future, running virt-install with
> the --debug flag should print the full backtrace, rather than needing to use gdb.

It's nice!

> As you say, in latest virt-install, --vcpus is now a string rather than a
> plain int. This is deliberate to facilitate new functionality like specifying
> maxvcpus and socket/core/thread topology. So the correct fix is to juss tweak
> --check-cpu string format.
> 
> I've fixed this upstream now:
> 
> http://hg.fedorahosted.org/hg/python-virtinst/rev/cdfd4ebd2233

Thanks for the explanation.


I looked into this further and guess one more fix is needed because the
variable 'vcpus' in parse_vcpu_option() and get_vcpus() looks set to
None sometimes.


Signed-off-by: Satoru SATOH <satoru.satoh at gmail.com>

---
 virtinst/cli.py |   36 ++++++++++++++++++++++++------------
 1 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/virtinst/cli.py b/virtinst/cli.py
index 6fd8e38..9f9e0b8 100644
--- a/virtinst/cli.py
+++ b/virtinst/cli.py
@@ -695,13 +695,17 @@ def _build_set_param(inst, opts):
 def parse_vcpu_option(guest, optstring, default_vcpus):
     """
     Helper to parse --vcpu string
+
+    @param  guest: virtinst.Guest instance (object)
+    @param  optstring: value of the option '--vcpus' (str)
+    @param  default_vcpus: ? (it should be None at present.)
     """
     vcpus, opts = parse_optstr(optstring, remove_first=True)
     vcpus = vcpus or default_vcpus
 
     set_param = _build_set_param(guest, opts)
     set_cpu_param = _build_set_param(guest.cpu, opts)
-    has_vcpus = ("vcpus" in opts or vcpus)
+    has_vcpus = ("vcpus" in opts or (vcpus is not None))
 
     set_param("vcpus", "vcpus", vcpus)
     set_param("maxvcpus", "maxvcpus")
@@ -718,21 +722,29 @@ def parse_vcpu_option(guest, optstring, default_vcpus):
 
 
 def get_vcpus(vcpus, check_cpu, guest, image_vcpus=None):
-    if vcpus is None and image_vcpus is not None:
-        vcpus = int(image_vcpus)
+    """
+    @param vcpus: value of the option '--vcpus' (str or None)
+    @param check_cpu: Whether to check that the number virtual cpus requested
+                      does not exceed physical CPUs (bool)
+    @param guest: virtinst.Guest instance (object)
+    @param image_vcpus: ? (It's not used currently and should be None.)
+    """
+    if vcpus is None:
+        if image_vcpus is not None:
+            vcpus = image_vcpus
+        else:
+            vcpus = ""
 
     parse_vcpu_option(guest, vcpus, image_vcpus)
 
-    conn = guest.conn
     if check_cpu:
-        vcpucount = str(guest.vcpus)
-
-        hostinfo = conn.getInfo()
-        cpu_num = hostinfo[4] * hostinfo[5] * hostinfo[6] * hostinfo[7]
-        if not vcpus <= cpu_num:
-            msg = _("You have asked for more virtual CPUs (%s) than there "
-                    "are physical CPUs (%s) on the host. This will work, "
-                    "but performance will be poor. ") % (vcpucount, cpu_num)
+        hostinfo = guest.conn.getInfo()
+        pcpus = hostinfo[4] * hostinfo[5] * hostinfo[6] * hostinfo[7]
+
+        if guest.vcpus > pcpus:
+            msg = _("You have asked for more virtual CPUs (%d) than there "
+                    "are physical CPUs (%d) on the host. This will work, "
+                    "but performance will be poor. ") % (guest.vcpus, pcpus)
             askmsg = _("Are you sure? (yes or no)")
 
             if not prompt_for_yes_or_no(msg, askmsg):
-- 
1.7.4