[libvirt] [PATCH 2/4] Rename and move kvmGetMaxVCPUs to utils and extend it

[Andrea Bolognani]

On Wed, 2016-06-15 at 09:56 +0000, Shivaprasad G Bhat wrote:
> This function needs to be used at two different places and make it global
> now. Also, extend it to return the NR_CPUs when needed.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> ---
>  src/libvirt_private.syms |    1 +
>  src/qemu/qemu_driver.c   |   52 +---------------------------------------------
>  src/util/virhostcpu.c    |   37 +++++++++++++++++++++++++++++++++
>  src/util/virhostcpu.h    |    7 ++++++
>  4 files changed, 46 insertions(+), 51 deletions(-)

This should really be two patches: one moving the function
to its new location while marking it as a private exported
symbol, and the other one chaging its signature and
behavior.

Actually, make it three patches - see below.

> -/* add definitions missing in older linux/kvm.h */
> -#ifndef KVMIO
> -# define KVMIO 0xAE
> -#endif
> -#ifndef KVM_CHECK_EXTENSION
> -# define KVM_CHECK_EXTENSION       _IO(KVMIO,   0x03)
> -#endif
> -#ifndef KVM_CAP_NR_VCPUS
> -# define KVM_CAP_NR_VCPUS 9       /* returns max vcpus per vm */
> -#endif

We no longer support any distribution old enough not to have
these definitions, so you can just drop them (as a separate
patch).

> +int
> +virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag)
> +{
> +    int fd;
> +    int ret;
> +
> +    if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) {
> +        virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE);
> +        return -1;
> +    }
> +
> +#ifdef KVM_CAP_MAX_VCPUS
> +    /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */
> +    if (flag & VIR_HOSTCPU_KVM_MAXVCPUS &&
> +        (ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0)
> +        goto cleanup;
> +#endif /* KVM_CAP_MAX_VCPUS */
> +
> +    /* as a fallback get KVM_CAP_NR_VCPUS (the recommended maximum number of
> +     * vcpus). Note that on most machines this is set to 160. */
> +    if ((flag & VIR_HOSTCPU_KVM_MAXVCPUS || flag & VIR_HOSTCPU_KVM_NR_VCPUS) &&
> +        (ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS)) > 0)
> +                 goto cleanup;

You dropped a blank line here, and the "goto" is not indented
properly

> +    /* if KVM_CAP_NR_VCPUS doesn't exist either, kernel documentation states
> +     * that 4 should be used as the maximum number of cpus */
> +    ret = 4;

[...]

> +typedef enum {
> +    VIR_HOSTCPU_KVM_MAXVCPUS   = (1 << 0),
> +    VIR_HOSTCPU_KVM_NR_VCPUS = (1 << 1),
> +} virHostCPUKVMWrapperFlags;
> +
> +int virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag);
> 

The naming for the function, and especially for the enum and
its values, is IMHO kinda awkward after the change... We could
have something like

  virHostCPUGetKVMMaxVCPUs()
  virHostCPUGetKVMRecommendedVCPUs()

instead. The attached patch, which applies cleanly on top of
your series, shows what I have in mind :)

-- 
Andrea Bolognani
Software Engineer - Virtualization Team
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Introduce-virHostCPUGetKVM-Max-Recommended-VCPU.patch
Type: text/x-patch
Size: 5260 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160622/822d3eaf/attachment-0001.bin>