Re: [PATCH v2 2/3] qemu: check the kvm host cpu max limits in virConnectGetDomainCapabilities

[Andrea Bolognani <abologna@xxxxxxxxxx>]

On Fri, 2016-06-24 at 20:34 +0530, Shivaprasad G Bhat wrote:
> The qemu limit and host limit both should be considered for
> the domain vcpu max limits.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxxxxxxx>
> ---
>  src/qemu/qemu_capabilities.c |   11 ++++++++---
>  src/qemu/qemu_capabilities.h |    3 ++-
>  src/qemu/qemu_driver.c       |    2 +-
>  tests/domaincapstest.c       |    3 ++-
>  4 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 01466fc..ff5ad19 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -38,6 +38,7 @@
>  #include "virbitmap.h"
>  #include "virnodesuspend.h"
>  #include "virnuma.h"
> +#include "virhostcpu.h"
>  #include "qemu_monitor.h"
>  #include "virstring.h"
>  #include "qemu_hostdev.h"
> @@ -4336,16 +4337,20 @@ int
>  virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
>                            virQEMUCapsPtr qemuCaps,
>                            virFirmwarePtr *firmwares,
> -                          size_t nfirmwares)
> +                          size_t nfirmwares,
> +                          virDomainVirtType virttype)

I missed this the first time around, but passing virttype
here is kinda pointless as it can be retrieved from
domCaps->virttype. However, that would cause the test
suite to depend on the host state, /dev/kvm in particular).

So ACK to this approach for now, but there should be a
follow-up patch that updates domaincapstest to access a
mocked /dev/kvm and gets rid of this extra parameter.

>  {
>      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);
>  
> -    domCaps->maxvcpus = maxvcpus;
> +    domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);

Line's too long, should have been split into two.

> +    if (virttype == VIR_DOMAIN_VIRT_KVM) {
> +        int hostmaxvcpus = virHostCPUGetKVMMaxVCPUs();
> +        domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus);
> +    }

I mentioned during the first round of reviews that the
value returned by virHostCPUGetKVMMaxVCPUs() should be
checked, because it could be <0 if /dev/kvm could not
be accessed.

ACK, I've squashed in the following trivial diff and
pushed the patch.


diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ff5ad19..28d5321 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4346,10 +4346,12 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
     virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics;
     virDomainCapsDeviceVideoPtr video = &domCaps->video;
 
-    domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
+    domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps,
+                                                     domCaps->machine);
     if (virttype == VIR_DOMAIN_VIRT_KVM) {
         int hostmaxvcpus = virHostCPUGetKVMMaxVCPUs();
-        domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus);
+        if (hostmaxvcpus >= 0)
+            domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus);
     }
 
     if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 ||

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list