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

[Cole Robinson]

On 03/19/2011 08:50 PM, Satoru SATOH wrote:
> 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):

Yes, just was informed on friday that --check-cpu was still broken. Thanks for
the patch, applied now:

http://git.fedorahosted.org/git?p=python-virtinst.git;a=commit;h=42fef47ef31f5daf230ed63f22b185da696bc35d

And a test case:

http://git.fedorahosted.org/git?p=python-virtinst.git;a=commit;h=1e214b7ba07cf1285ee5627d1cd3179c368a967d

Thanks,
Cole