[libvirt] [PATCH] host-validate: Improve CPU flags processing
Instead of relying on substring search, tokenize the input and process each CPU flag separately. This ensures CPU flag detection will continue to work correctly even if we start looking for CPU flags whose name might appear as part of other CPU flags' names. The result of processing is stored in a virBitmap, which means we don't have to parse /proc/cpuinfo in its entirety for each single CPU flag we want to check. Moreover, use of the newly-introduced virHostValidateCPUFlag enumeration ensures we don't go looking for random CPU flags which might actually be simple typos. --- The concern was raised by John in https://www.redhat.com/archives/libvir-list/2016-March/msg01301.html when discussing a new check on the "sie" CPU flag. tools/virt-host-validate-common.c | 50 +++++++++++++++++++++++++++++---------- tools/virt-host-validate-common.h | 13 +++++++++- tools/virt-host-validate-qemu.c | 12 ++++++++-- 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 8ebf73e..57fa332 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -31,7 +31,6 @@ #endif /* HAVE_MNTENT_H */ #include <sys/stat.h> -#include "virutil.h" #include "viralloc.h" #include "virfile.h" #include "virt-host-validate-common.h" @@ -39,6 +38,10 @@ #define VIR_FROM_THIS VIR_FROM_NONE +VIR_ENUM_IMPL(virHostValidateCPUFlag, VIR_HOST_VALIDATE_CPU_FLAG_LAST, + "vmx", + "svm"); + static bool quiet; void virHostMsgSetQuiet(bool quietFlag) @@ -188,29 +191,47 @@ int virHostValidateNamespace(const char *hvname, } -bool virHostValidateHasCPUFlag(const char *name) +virBitmapPtr virHostValidateGetCPUFlags(void) { - FILE *fp = fopen("/proc/cpuinfo", "r"); - bool ret = false; + FILE *fp; + virBitmapPtr flags; - if (!fp) - return false; + if (!(fp = fopen("/proc/cpuinfo", "r"))) + return NULL; + + if (!(flags = virBitmapNewQuiet(VIR_HOST_VALIDATE_CPU_FLAG_LAST))) + return NULL; do { char line[1024]; + char *saveptr; + char *token; if (!fgets(line, sizeof(line), fp)) break; - if (strstr(line, name)) { - ret = true; - break; + if (!(token = strtok_r(line, " \t\n", &saveptr))) + continue; + + /* The line we're interested in is marked either as "flags" or + * as "Features" depending on the architecture, so check both + * prefixes. It's very unlikely that there will be no whitespace + * between the line name and the colon, but handle that as well */ + if (strcmp(token, "flags") && strcmp(token, "flags:") && + strcmp(token, "Features") && strcmp(token, "Features:")) + continue; + + while ((token = strtok_r(NULL, " \t\n", &saveptr))) { + int value; + + if ((value = virHostValidateCPUFlagTypeFromString(token)) >= 0) + ignore_value(virBitmapSetBit(flags, value)); } } while (1); VIR_FORCE_FCLOSE(fp); - return ret; + return flags; } @@ -359,15 +380,20 @@ int virHostValidateCGroupController(const char *hvname, int virHostValidateIOMMU(const char *hvname, virHostValidateLevel level) { + virBitmapPtr flags; struct stat sb; const char *bootarg = NULL; bool isAMD = false, isIntel = false; - if (virHostValidateHasCPUFlag("vmx")) + flags = virHostValidateGetCPUFlags(); + + if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX)) isIntel = true; - else if (virHostValidateHasCPUFlag("svm")) + else if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SVM)) isAMD = true; + virBitmapFree(flags); + virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support")); if (isIntel) { diff --git a/tools/virt-host-validate-common.h b/tools/virt-host-validate-common.h index d4c4759..26e006b 100644 --- a/tools/virt-host-validate-common.h +++ b/tools/virt-host-validate-common.h @@ -23,6 +23,8 @@ # define __VIRT_HOST_VALIDATE_COMMON_H__ # include "internal.h" +# include "virutil.h" +# include "virbitmap.h" typedef enum { VIR_HOST_VALIDATE_FAIL, @@ -32,6 +34,15 @@ typedef enum { VIR_HOST_VALIDATE_LAST, } virHostValidateLevel; +typedef enum { + VIR_HOST_VALIDATE_CPU_FLAG_VMX = 0, + VIR_HOST_VALIDATE_CPU_FLAG_SVM, + + VIR_HOST_VALIDATE_CPU_FLAG_LAST, +} virHostValidateCPUFlag; + +VIR_ENUM_DECL(virHostValidateCPUFlag); + extern void virHostMsgSetQuiet(bool quietFlag); extern void virHostMsgCheck(const char *prefix, @@ -53,7 +64,7 @@ extern int virHostValidateDeviceAccessible(const char *hvname, virHostValidateLevel level, const char *hint); -extern bool virHostValidateHasCPUFlag(const char *name); +extern virBitmapPtr virHostValidateGetCPUFlags(void); extern int virHostValidateLinuxKernel(const char *hvname, int version, diff --git a/tools/virt-host-validate-qemu.c b/tools/virt-host-validate-qemu.c index a9f6c1e..b96b020 100644 --- a/tools/virt-host-validate-qemu.c +++ b/tools/virt-host-validate-qemu.c @@ -24,14 +24,20 @@ #include "virt-host-validate-qemu.h" #include "virt-host-validate-common.h" +#include "virbitmap.h" int virHostValidateQEMU(void) { + virBitmapPtr flags; int ret = 0; virHostMsgCheck("QEMU", "%s", ("for hardware virtualization")); - if (virHostValidateHasCPUFlag("svm") || - virHostValidateHasCPUFlag("vmx")) { + + flags = virHostValidateGetCPUFlags(); + + if (flags && + (virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_SVM) || + virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX))) { virHostMsgPass(); if (virHostValidateDeviceExists("QEMU", "/dev/kvm", VIR_HOST_VALIDATE_FAIL, @@ -48,6 +54,8 @@ int virHostValidateQEMU(void) _("Only emulated CPUs are available, performance will be significantly limited")); } + virBitmapFree(flags); + if (virHostValidateDeviceExists("QEMU", "/dev/vhost-net", VIR_HOST_VALIDATE_WARN, _("Load the 'vhost_net' module to improve performance " -- 2.5.5
On Tue, Mar 29, 2016 at 18:09:44 +0200, Andrea Bolognani wrote:
Instead of relying on substring search, tokenize the input and process each CPU flag separately. This ensures CPU flag detection will continue to work correctly even if we start looking for CPU flags whose name might appear as part of other CPU flags' names.
The result of processing is stored in a virBitmap, which means we don't have to parse /proc/cpuinfo in its entirety for each single CPU flag we want to check.
Moreover, use of the newly-introduced virHostValidateCPUFlag enumeration ensures we don't go looking for random CPU flags which might actually be simple typos. --- The concern was raised by John in
https://www.redhat.com/archives/libvir-list/2016-March/msg01301.html
when discussing a new check on the "sie" CPU flag.
tools/virt-host-validate-common.c | 50 +++++++++++++++++++++++++++++---------- tools/virt-host-validate-common.h | 13 +++++++++- tools/virt-host-validate-qemu.c | 12 ++++++++-- 3 files changed, 60 insertions(+), 15 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 8ebf73e..57fa332 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c
[...]
@@ -188,29 +191,47 @@ int virHostValidateNamespace(const char *hvname, }
-bool virHostValidateHasCPUFlag(const char *name) +virBitmapPtr virHostValidateGetCPUFlags(void) { - FILE *fp = fopen("/proc/cpuinfo", "r"); - bool ret = false; + FILE *fp; + virBitmapPtr flags;
- if (!fp) - return false; + if (!(fp = fopen("/proc/cpuinfo", "r"))) + return NULL; + + if (!(flags = virBitmapNewQuiet(VIR_HOST_VALIDATE_CPU_FLAG_LAST)))
Since you are already using libvirt's utils ...
+ return NULL;
do { char line[1024]; + char *saveptr; + char *token;
if (!fgets(line, sizeof(line), fp)) break;
- if (strstr(line, name)) { - ret = true; - break; + if (!(token = strtok_r(line, " \t\n", &saveptr)))
Why not virStringSplit ...
+ continue; + + /* The line we're interested in is marked either as "flags" or + * as "Features" depending on the architecture, so check both + * prefixes. It's very unlikely that there will be no whitespace + * between the line name and the colon, but handle that as well */ + if (strcmp(token, "flags") && strcmp(token, "flags:") &&
And STREQ? Peter
On Wed, 2016-03-30 at 09:28 +0200, Andrea Bolognani wrote:
On Wed, 2016-03-30 at 08:36 +0200, Peter Krempa wrote:
Since you are already using libvirt's utils ... [...] Why not virStringSplit ... [...] And STREQ? No reason at all, really :)
On second thought, after having actually tried to implement your suggestions... virStringSplit() only supports a single delimiter, while strtok_r() supports an arbitrary number - I'm using three in my case. So AFAICS the options are - call virStringSplit() multiple times, hoping to take all possible combinations into account - enhance virStringSplit() to support multiple delimiters - keep using strtok_r() I'm quite partial to the last option :P What about you? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Wed, Mar 30, 2016 at 18:35:04 +0200, Andrea Bolognani wrote:
On Wed, 2016-03-30 at 09:28 +0200, Andrea Bolognani wrote:
On Wed, 2016-03-30 at 08:36 +0200, Peter Krempa wrote:
Since you are already using libvirt's utils ... [...] Why not virStringSplit ... [...] And STREQ? No reason at all, really :)
On second thought, after having actually tried to implement your suggestions...
virStringSplit() only supports a single delimiter, while strtok_r() supports an arbitrary number - I'm using three in my case.
So AFAICS the options are
- call virStringSplit() multiple times, hoping to take all possible combinations into account
Humm, no that wouldn't make much sense.
- enhance virStringSplit() to support multiple delimiters
virStringSplit actually uses a string as delimiter for some reason so that option would basically result in adding a new function.
- keep using strtok_r()
Okay, it makes sense here :)
I'm quite partial to the last option :P What about you?
Let me re-visit the patch then. Peter
participants (2)
-
Andrea Bolognani -
Peter Krempa