[libvirt] [PATCH REPOST 0/3] Small cleanups and improvements
Rebased on top of current master. Previous attempt: https://www.redhat.com/archives/libvir-list/2016-June/msg02037.html Andrea Bolognani (3): util: hostcpu: Add virHostCPUGetKVMMaxVCPUs() stub util: hostcpu: Drop obsolete compatibility code qemu: capabilities: Make virHostCPUGetKVMMaxVCPUs() errors fatal src/qemu/qemu_capabilities.c | 9 ++++++--- src/util/virhostcpu.c | 25 ++++++++++++------------- 2 files changed, 18 insertions(+), 16 deletions(-) -- 2.7.4
If we don't HAVE_LINUX_KVM_H, we can't query /dev/kvm to discover the limits on the number of vCPUs, so we report an error and return a negative value instead. --- src/util/virhostcpu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 4ff4e72..a33932f 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1299,6 +1299,7 @@ virHostCPUGetThreadsPerSubcore(virArch arch ATTRIBUTE_UNUSED) #endif /* HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) */ +#if HAVE_LINUX_KVM_H int virHostCPUGetKVMMaxVCPUs(void) { @@ -1310,11 +1311,11 @@ virHostCPUGetKVMMaxVCPUs(void) return -1; } -#ifdef KVM_CAP_MAX_VCPUS +# ifdef KVM_CAP_MAX_VCPUS /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */ if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0) goto cleanup; -#endif /* KVM_CAP_MAX_VCPUS */ +# 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. */ @@ -1329,3 +1330,12 @@ virHostCPUGetKVMMaxVCPUs(void) VIR_FORCE_CLOSE(fd); return ret; } +#else +int +virHostCPUGetKVMMaxVCPUs(void) +{ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("KVM is not supported on this platform")); + return -1; +} +#endif /* HAVE_LINUX_KVM_H */ -- 2.7.4
On 08.07.2016 18:04, Andrea Bolognani wrote:
If we don't HAVE_LINUX_KVM_H, we can't query /dev/kvm to discover the limits on the number of vCPUs, so we report an error and return a negative value instead. --- src/util/virhostcpu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 4ff4e72..a33932f 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1299,6 +1299,7 @@ virHostCPUGetThreadsPerSubcore(virArch arch ATTRIBUTE_UNUSED)
#endif /* HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) */
+#if HAVE_LINUX_KVM_H int virHostCPUGetKVMMaxVCPUs(void) { @@ -1310,11 +1311,11 @@ virHostCPUGetKVMMaxVCPUs(void) return -1; }
-#ifdef KVM_CAP_MAX_VCPUS +# ifdef KVM_CAP_MAX_VCPUS /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */ if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0) goto cleanup; -#endif /* KVM_CAP_MAX_VCPUS */ +# 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. */ @@ -1329,3 +1330,12 @@ virHostCPUGetKVMMaxVCPUs(void) VIR_FORCE_CLOSE(fd); return ret; } +#else +int +virHostCPUGetKVMMaxVCPUs(void) +{ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("KVM is not supported on this platform"));
Don't mean to bikeshed, but I'd prefer if this were: virReportSystemError(ENOSYS, ..); I think it's the common patter we use for non-Linux, non-BSD* platform stubs. And if you think of it, it really is a property of a system, not unsupported config on libvirt level. BTW I know that we are not consistent in the error codes. It's one big mess.
+ return -1; +} +#endif /* HAVE_LINUX_KVM_H */
Michal
On Sat, 2016-07-09 at 10:33 +0200, Michal Privoznik wrote:
+virHostCPUGetKVMMaxVCPUs(void) +{ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("KVM is not supported on this platform")); Don't mean to bikeshed, but I'd prefer if this were: virReportSystemError(ENOSYS, ..); I think it's the common patter we use for non-Linux, non-BSD* platform stubs. And if you think of it, it really is a property of a system, not unsupported config on libvirt level. BTW I know that we are not consistent in the error codes. It's one big mess.
Makes sense, and it's indeed a very common pattern throughout libvirt. I changed it. -- Andrea Bolognani / Red Hat / Virtualization
All Linux releases we support (RHEL6 era) include these definitions. --- src/util/virhostcpu.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index a33932f..8a8bda8 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -65,17 +65,6 @@ VIR_LOG_INIT("util.hostcpu"); #define KVM_DEVICE "/dev/kvm" -/* 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 - #if defined(__FreeBSD__) || defined(__APPLE__) static int -- 2.7.4
On 08.07.2016 18:04, Andrea Bolognani wrote:
All Linux releases we support (RHEL6 era) include these definitions. --- src/util/virhostcpu.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index a33932f..8a8bda8 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -65,17 +65,6 @@ VIR_LOG_INIT("util.hostcpu");
#define KVM_DEVICE "/dev/kvm"
-/* 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 -
#if defined(__FreeBSD__) || defined(__APPLE__) static int
ACK. /me just wonders why we even had that code. These defs where since Avi Kivity started merging KVM to kernel. This is 2006 that we're talking here! Michal
On Sat, 2016-07-09 at 10:33 +0200, Michal Privoznik wrote:
On 08.07.2016 18:04, Andrea Bolognani wrote:
All Linux releases we support (RHEL6 era) include these definitions. --- src/util/virhostcpu.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index a33932f..8a8bda8 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -65,17 +65,6 @@ VIR_LOG_INIT("util.hostcpu"); #define KVM_DEVICE "/dev/kvm" -/* 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 - #if defined(__FreeBSD__) || defined(__APPLE__) static int ACK. /me just wonders why we even had that code. These defs where since Avi Kivity started merging KVM to kernel. This is 2006 that we're talking here!
Well, before having a stub for the purpose, those defines were also needed in order to be able to compile on non-Linux platforms :) -- Andrea Bolognani / Red Hat / Virtualization
An error in virHostCPUGetKVMMaxVCPUs() means we've been unable to access /dev/kvm, or we're running on a platform that doesn't support KVM in the first place. If that's the case, we shouldn't ignore the error and report domcapabilities even though we know the user won't be able to start any KVM guest. --- src/qemu/qemu_capabilities.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c8d8a54..3ad71a2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4347,9 +4347,12 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine); if (domCaps->virttype == VIR_DOMAIN_VIRT_KVM) { - int hostmaxvcpus = virHostCPUGetKVMMaxVCPUs(); - if (hostmaxvcpus >= 0) - domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); + int hostmaxvcpus; + + if ((hostmaxvcpus = virHostCPUGetKVMMaxVCPUs()) < 0) + return -1; + + domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); } if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 || -- 2.7.4
On 08.07.2016 18:04, Andrea Bolognani wrote:
Rebased on top of current master.
Previous attempt:
https://www.redhat.com/archives/libvir-list/2016-June/msg02037.html
Andrea Bolognani (3): util: hostcpu: Add virHostCPUGetKVMMaxVCPUs() stub util: hostcpu: Drop obsolete compatibility code qemu: capabilities: Make virHostCPUGetKVMMaxVCPUs() errors fatal
src/qemu/qemu_capabilities.c | 9 ++++++--- src/util/virhostcpu.c | 25 ++++++++++++------------- 2 files changed, 18 insertions(+), 16 deletions(-)
ACK series. Michal
On Sat, 2016-07-09 at 10:33 +0200, Michal Privoznik wrote:
On 08.07.2016 18:04, Andrea Bolognani wrote:
Rebased on top of current master. Previous attempt: https://www.redhat.com/archives/libvir-list/2016-June/msg02037.html Andrea Bolognani (3): util: hostcpu: Add virHostCPUGetKVMMaxVCPUs() stub util: hostcpu: Drop obsolete compatibility code qemu: capabilities: Make virHostCPUGetKVMMaxVCPUs() errors fatal src/qemu/qemu_capabilities.c | 9 ++++++--- src/util/virhostcpu.c | 25 ++++++++++++------------- 2 files changed, 18 insertions(+), 16 deletions(-) ACK series.
I've addressed your comment in patch 1/3 and pushed the series. Thanks for the review! :) -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Michal Privoznik