[libvirt] [PATCH 4/4] Check the kvm host cpu max limits in virConnectGetDomainCapabilities()

[Andrea Bolognani]

On Wed, 2016-06-15 at 09:58 +0000, Shivaprasad G Bhat wrote:
>      <dl>
>        <dt><code>vcpu</code></dt>
> -      <dd>The maximum number of supported virtual CPUs</dd>
> +      <dd>The maximum number of supported virtual CPUs. The suggested attribute if present, gives the recommended
> maximum vcpus for the KVM host.</dd>
>      </dl>

Rewrite as

  Information about the number of virtual CPUs that can be
  assigned to a guest. The <code>max</code> attribute contains
  the maximum number of virtual CPUs; the
  <code>suggested</code> attribute, if present, contains the
  recommended number of virtual CPUs.

making sure lines are at most 80 columns long.

I would prefer to use "recommended" instead of "suggested" for
the attribute name, and for all related variables, as the
relevant comment in linux/kvm.h reads

  /* returns recommended max vcpus per vm */

and "to recommend" is generally stronger than "to suggest", ie.
you should think twice before disregarding a recommendation.

> -    if (caps->maxvcpus)
> -        virBufferAsprintf(buf, "<vcpu max='%d'/>\n", caps->maxvcpus);
> -
> +    if (caps->maxvcpus) {
> +        if (!caps->suggestedvcpus)
> +            virBufferAsprintf(buf, "<vcpu max='%d'/>\n", caps->maxvcpus);
> +        else
> +            virBufferAsprintf(buf, "<vcpu max='%d' suggested='%d'/>\n",
> +                          caps->maxvcpus, caps->suggestedvcpus);
> +    }

You could change this to

  if (caps->maxvcpus) {
      virBufferAsprintf(buf, "<vcpu max='%d'", caps->maxvcpus);
      if (caps->suggestedvcpus)
          virBufferAsprintf(buf, " suggested='%d'", caps->suggestedvcpus);
      virBufferAddLit(buf, "/>\n");
  }

but it's up to you, really.

>  virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
>                            virQEMUCapsPtr qemuCaps,
>                            virFirmwarePtr *firmwares,
> -                          size_t nfirmwares)
> +                          size_t nfirmwares,
> +                          virDomainVirtType virttype)
>  {
>      virDomainCapsOSPtr os = &domCaps->os;
>      virDomainCapsDeviceDiskPtr disk = &domCaps->disk;
>      virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev;
>      virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics;
>      virDomainCapsDeviceVideoPtr video = &domCaps->video;
> -    int maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
> +    int hostmaxvcpus = 0;
> 
> -    domCaps->maxvcpus = maxvcpus;
> +    domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
> +    if (virttype == VIR_DOMAIN_VIRT_KVM) {
> +        hostmaxvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS);

You can move the definition of the 'hostmaxvcpus' variable
in here, it's not used in the outer scope anyway.

You should also perform some error checking here:
virHostCPUGetKVMVCPUs() can return a negative number, for
example if /dev/kvm can't be accessed.

If that happens, I'm not sure whether it would be better
to simply ignore the failure and just report the QEMU
limits, or if we should abort virQEMUCapsFillDomainCaps()
altogether. Probably the former.

Everything else looks good.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team