From: Bjoern Walk <bwalk@linux.vnet.ibm.com>
All different architectures use the same copy-pasted code to parse
processor frequency information from /proc/cpuinfo. Let's extract that
code into a function to avoid repetition.
We now also tolerate if the parsing of /proc/cpuinfo is not successful
and just report a warning instead of bailing out and abandoning the rest
of the CPU information.
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
---
Changes from Bjoern's original version:
* rename virHostCPUParseFrequencyLine() to
virHostCPUParseFrequencyString().
src/util/virhostcpu.c | 136 +++++++++++++++++++++-----------------------------
1 file changed, 57 insertions(+), 79 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index c485a9721..3091a92c0 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -508,6 +508,61 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
return ret;
}
+static int
+virHostCPUParseFrequencyString(const char *str,
+ unsigned int *mhz)
+{
+ char *p;
+ unsigned int ui;
+
+ while (*str && c_isspace(*str))
+ str++;
+
+ if (*str != ':' || !str[1]) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("parsing cpu MHz from cpuinfo"));
+ return -1;
+ }
+
+ if (virStrToLong_ui(str + 1, &p, 10, &ui) == 0 &&
+ /* Accept trailing fractional part. */
+ (*p == '\0' || *p == '.' || c_isspace(*p)))
+ *mhz = ui;
+
+ return 0;
+}
+
+static int
+virHostCPUParseFrequency(FILE *cpuinfo,
+ virArch arch,
+ unsigned int *mhz)
+{
+ const char *prefix = NULL;
+ char line[1024];
+
+ if (ARCH_IS_X86(arch))
+ prefix = "cpu MHz";
+ else if (ARCH_IS_PPC(arch))
+ prefix = "clock";
+ else if (ARCH_IS_ARM(arch))
+ prefix = "BogoMIPS";
+
+ if (!prefix) {
+ VIR_WARN("Parser for /proc/cpuinfo needs to be adapted for your architecture");
+ return 1;
+ }
+
+ while (fgets(line, sizeof(line), cpuinfo) != NULL) {
+ if (!STRPREFIX(line, prefix))
+ continue;
+
+ if (virHostCPUParseFrequencyString(line + strlen(prefix), mhz) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
int
virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
virArch arch,
@@ -520,7 +575,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
{
virBitmapPtr present_cpus_map = NULL;
virBitmapPtr online_cpus_map = NULL;
- char line[1024];
DIR *nodedir = NULL;
struct dirent *nodedirent = NULL;
int nodecpus, nodecores, nodesockets, nodethreads, offline = 0;
@@ -535,84 +589,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
*cpus = *nodes = *sockets = *cores = *threads = 0;
/* Start with parsing CPU clock speed from /proc/cpuinfo */
- while (fgets(line, sizeof(line), cpuinfo) != NULL) {
- if (ARCH_IS_X86(arch)) {
- char *buf = line;
- if (STRPREFIX(buf, "cpu MHz")) {
- char *p;
- unsigned int ui;
-
- buf += 7;
- while (*buf && c_isspace(*buf))
- buf++;
-
- if (*buf != ':' || !buf[1]) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("parsing cpu MHz from cpuinfo"));
- goto cleanup;
- }
-
- if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0 &&
- /* Accept trailing fractional part. */
- (*p == '\0' || *p == '.' || c_isspace(*p)))
- *mhz = ui;
- }
- } else if (ARCH_IS_PPC(arch)) {
- char *buf = line;
- if (STRPREFIX(buf, "clock")) {
- char *p;
- unsigned int ui;
-
- buf += 5;
- while (*buf && c_isspace(*buf))
- buf++;
-
- if (*buf != ':' || !buf[1]) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("parsing cpu MHz from cpuinfo"));
- goto cleanup;
- }
-
- if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0 &&
- /* Accept trailing fractional part. */
- (*p == '\0' || *p == '.' || c_isspace(*p)))
- *mhz = ui;
- /* No other interesting infos are available in /proc/cpuinfo.
- * However, there is a line identifying processor's version,
- * identification and machine, but we don't want it to be caught
- * and parsed in next iteration, because it is not in expected
- * format and thus lead to error. */
- }
- } else if (ARCH_IS_ARM(arch)) {
- char *buf = line;
- if (STRPREFIX(buf, "BogoMIPS")) {
- char *p;
- unsigned int ui;
-
- buf += 8;
- while (*buf && c_isspace(*buf))
- buf++;
-
- if (*buf != ':' || !buf[1]) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("parsing cpu MHz from cpuinfo"));
- goto cleanup;
- }
-
- if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0
- /* Accept trailing fractional part. */
- && (*p == '\0' || *p == '.' || c_isspace(*p)))
- *mhz = ui;
- }
- } else if (ARCH_IS_S390(arch)) {
- /* s390x has no realistic value for CPU speed,
- * assign a value of zero to signify this */
- *mhz = 0;
- } else {
- VIR_WARN("Parser for /proc/cpuinfo needs to be adapted for your architecture");
- break;
- }
- }
+ if (virHostCPUParseFrequency(cpuinfo, arch, mhz) < 0)
+ VIR_WARN("Unable to parse processor frequency information from /proc/cpuinfo");
/* Get information about what CPUs are present in the host and what
* CPUs are online, so that we don't have to so for each node */
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wednesday, 13 December 2017 17:10:19 CET Andrea Bolognani wrote:
> From: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>
> All different architectures use the same copy-pasted code to parse
> processor frequency information from /proc/cpuinfo. Let's extract that
> code into a function to avoid repetition.
>
> We now also tolerate if the parsing of /proc/cpuinfo is not successful
> and just report a warning instead of bailing out and abandoning the rest
> of the CPU information.
>
> Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> ---
> [...]
> +static int
> +virHostCPUParseFrequency(FILE *cpuinfo,
> + virArch arch,
> + unsigned int *mhz)
> +{
> + const char *prefix = NULL;
> + char line[1024];
> +
> + if (ARCH_IS_X86(arch))
> + prefix = "cpu MHz";
> + else if (ARCH_IS_PPC(arch))
> + prefix = "clock";
> + else if (ARCH_IS_ARM(arch))
> + prefix = "BogoMIPS";
> +
> + if (!prefix) {
> + VIR_WARN("Parser for /proc/cpuinfo needs to be adapted for your architecture");
> + return 1;
I'd print the architecture in the warning, so sysadmins can see easily
which architecture it is, even when looking at logs collected from
different libvirt installations.
> + while (fgets(line, sizeof(line), cpuinfo) != NULL) {
> + if (!STRPREFIX(line, prefix))
> + continue;
IMHO here it would be a good idea to check that line[strlen(prefix)]
is either a space or ':', to avoid prefix matching more keys than the
actual intended one(s) -- something like:
char c = line[strlen(prefix)];
if (c != ':' && !c_isspace(*str))
continue;
--
Pino Toscano--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-12-13 at 17:35 +0100, Pino Toscano wrote:
> > + if (!prefix) {
> > + VIR_WARN("Parser for /proc/cpuinfo needs to be adapted for your architecture");
> > + return 1;
>
> I'd print the architecture in the warning, so sysadmins can see easily
> which architecture it is, even when looking at logs collected from
> different libvirt installations.
Sure.
> > + while (fgets(line, sizeof(line), cpuinfo) != NULL) {
> > + if (!STRPREFIX(line, prefix))
> > + continue;
>
> IMHO here it would be a good idea to check that line[strlen(prefix)]
> is either a space or ':', to avoid prefix matching more keys than the
> actual intended one(s) -- something like:
>
> char c = line[strlen(prefix)];
> if (c != ':' && !c_isspace(*str))
> continue;
We skip the prefix and pass the rest of the line to
virHostCPUParseFrequencyString(), which starts by skipping all
whitespace and then checking the first non-whitespace character
is a semicolon. So I don't see how we could end up matching
anything but the intended line.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thursday, 14 December 2017 10:46:33 CET Andrea Bolognani wrote:
> On Wed, 2017-12-13 at 17:35 +0100, Pino Toscano wrote:
> > > + while (fgets(line, sizeof(line), cpuinfo) != NULL) {
> > > + if (!STRPREFIX(line, prefix))
> > > + continue;
> >
> > IMHO here it would be a good idea to check that line[strlen(prefix)]
> > is either a space or ':', to avoid prefix matching more keys than the
> > actual intended one(s) -- something like:
> >
> > char c = line[strlen(prefix)];
> > if (c != ':' && !c_isspace(*str))
> > continue;
>
> We skip the prefix and pass the rest of the line to
> virHostCPUParseFrequencyString(), which starts by skipping all
> whitespace and then checking the first non-whitespace character
> is a semicolon. So I don't see how we could end up matching
> anything but the intended line.
Ah sorry, I did not explain all: the situation I see is that
virHostCPUParseFrequencyString errors out if it does not find the
colon. Let's say that on x86_64 /proc/cpuinfo contains:
cpu MHz new : 1000.000
cpu MHz : 2000.000
since "cpu MHz" is the prefix on x86, then the "cpu MHz new" line
matches it so virHostCPUParseFrequencyString will be called, but then
virHostCPUParseFrequencyString will error out because (after skipping
spaces) it will find 'n'. A failure in virHostCPUParseFrequencyString
is propagated directly by virHostCPUParseFrequency, so the real key in
cpuinfo will not be read.
--
Pino Toscano--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-12-14 at 11:07 +0100, Pino Toscano wrote:
> On Thursday, 14 December 2017 10:46:33 CET Andrea Bolognani wrote:
> > On Wed, 2017-12-13 at 17:35 +0100, Pino Toscano wrote:
> > > > + while (fgets(line, sizeof(line), cpuinfo) != NULL) {
> > > > + if (!STRPREFIX(line, prefix))
> > > > + continue;
> > >
> > > IMHO here it would be a good idea to check that line[strlen(prefix)]
> > > is either a space or ':', to avoid prefix matching more keys than the
> > > actual intended one(s) -- something like:
> > >
> > > char c = line[strlen(prefix)];
> > > if (c != ':' && !c_isspace(*str))
> > > continue;
> >
> > We skip the prefix and pass the rest of the line to
> > virHostCPUParseFrequencyString(), which starts by skipping all
> > whitespace and then checking the first non-whitespace character
> > is a semicolon. So I don't see how we could end up matching
> > anything but the intended line.
>
> Ah sorry, I did not explain all: the situation I see is that
> virHostCPUParseFrequencyString errors out if it does not find the
> colon. Let's say that on x86_64 /proc/cpuinfo contains:
>
> cpu MHz new : 1000.000
> cpu MHz : 2000.000
>
> since "cpu MHz" is the prefix on x86, then the "cpu MHz new" line
> matches it so virHostCPUParseFrequencyString will be called, but then
> virHostCPUParseFrequencyString will error out because (after skipping
> spaces) it will find 'n'. A failure in virHostCPUParseFrequencyString
> is propagated directly by virHostCPUParseFrequency, so the real key in
> cpuinfo will not be read.
You realize the change you propose[1] wouldn't deal properly with
your very example, right? ;)
I'll come up with something: not that I expect this to cause much
actual harm, but since we're fixing it already might as well go the
extra mile.
[1] Assuming passing '*str' rather than 'c' to c_isspace() is a
mere pasto, which would be consistent with your explanation
of its intended purpose.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Pino Toscano <ptoscano@redhat.com> [2017-12-13, 05:35PM +0100]:
> On Wednesday, 13 December 2017 17:10:19 CET Andrea Bolognani wrote:
> > From: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> >
> > All different architectures use the same copy-pasted code to parse
> > processor frequency information from /proc/cpuinfo. Let's extract that
> > code into a function to avoid repetition.
> >
> > We now also tolerate if the parsing of /proc/cpuinfo is not successful
> > and just report a warning instead of bailing out and abandoning the rest
> > of the CPU information.
> >
> > Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> > Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> > Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> > Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> > ---
> > [...]
> > +static int
> > +virHostCPUParseFrequency(FILE *cpuinfo,
> > + virArch arch,
> > + unsigned int *mhz)
> > +{
> > + const char *prefix = NULL;
> > + char line[1024];
> > +
> > + if (ARCH_IS_X86(arch))
> > + prefix = "cpu MHz";
> > + else if (ARCH_IS_PPC(arch))
> > + prefix = "clock";
> > + else if (ARCH_IS_ARM(arch))
> > + prefix = "BogoMIPS";
> > +
> > + if (!prefix) {
> > + VIR_WARN("Parser for /proc/cpuinfo needs to be adapted for your architecture");
> > + return 1;
>
> I'd print the architecture in the warning, so sysadmins can see easily
> which architecture it is, even when looking at logs collected from
> different libvirt installations.
>
That's probably a good idea.
> > + while (fgets(line, sizeof(line), cpuinfo) != NULL) {
> > + if (!STRPREFIX(line, prefix))
> > + continue;
>
> IMHO here it would be a good idea to check that line[strlen(prefix)]
> is either a space or ':', to avoid prefix matching more keys than the
> actual intended one(s) -- something like:
>
> char c = line[strlen(prefix)];
> if (c != ':' && !c_isspace(*str))
> continue;
>
Here I'm not sure. This would be for optimization, right? Because we
have this check in the parsing function. I don't think that this code is
so hard-pressed for optimizations that we should make it more
complicated. But I have no hard feelings over this, it's Andrea's call.
> --
> Pino Toscano
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.