Re: [PATCH 3/6] util: Introduce virHostCPUGetInfoParseCPUFrequency()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Andrea Bolognani <abologna@xxxxxxxxxx> [2017-12-11, 05:40PM +0100]:
> The algorithm used to extract the CPU frequency from /proc/cpuinfo
> is the same regardless of architecture, the only difference being
> the label used to identify the relevant field.
> 
> Factor the parsing code out to a new helper and use it to implement
> virHostCPUGetInfoParseCPUInfo(), thus removing some duplication.
> 
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
>  src/util/virhostcpu.c | 105 +++++++++++++++++++-------------------------------
>  1 file changed, 40 insertions(+), 65 deletions(-)
> 
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 4d5c56659..85803d527 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -509,6 +509,40 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
>  }
> 
> 
> +static int
> +virHostCPUGetInfoParseCPUFrequency(const char *buf,
> +                                   const char *label,
> +                                   unsigned int *mhz)
> +{
> +    int ret = -1;
> +
> +    if (STRPREFIX(buf, label)) {

I'd prefer negation and early exit to get rid of one indentation level.
I also moved this conditional into the loop below. Save function calls.

> +        char *p;
> +        unsigned int ui;
> +
> +        buf += strlen(label);
> +        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;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    return ret;
> +}
> +
> +
>  static int
>  virHostCPUGetInfoParseCPUInfo(FILE *cpuinfo,
>                                virArch arch,
> @@ -521,73 +555,14 @@ virHostCPUGetInfoParseCPUInfo(FILE *cpuinfo,
> 
>      while (fgets(line, sizeof(line), cpuinfo) != NULL) {
>          if (ARCH_IS_X86(arch)) {

Probably (hopefully?) the compiler can optimize, but I get a bit iffy if
I see a loop invariant conditional inside a loop. I find this also
harder to read.

> -            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;
> -            }
> +            if (virHostCPUGetInfoParseCPUFrequency(line, "cpu MHz", mhz) < 0)
> +                goto cleanup;
>          } 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. */
> -            }
> +            if (virHostCPUGetInfoParseCPUFrequency(line, "clock", mhz) < 0)
> +                goto cleanup;
>          } 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;
> -            }
> +            if (virHostCPUGetInfoParseCPUFrequency(line, "BogoMIPS", mhz) < 0)
> +                goto cleanup;
>          } else if (ARCH_IS_S390(arch)) {
>              /* s390x has no realistic value for CPU speed,
>               * assign a value of zero to signify this */
> -- 
> 2.14.3
> 
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

I attach my take on this refactorization for comparison. Feel free to
take stuff you like. Or don't :)

-- 
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@xxxxxxxxxx
------------------------------------------------------------------------
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 
From 0a6cda1c0ff218585df811c952b149793f00ca17 Mon Sep 17 00:00:00 2001
From: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
Date: Thu, 30 Nov 2017 12:50:47 +0100
Subject: [PATCH 1/4] util: virhostcpu: factor out frequency parsing

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@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
---
 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 c485a972..0a4b241d 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -508,6 +508,61 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
     return ret;
 }
 
+static int
+virHostCPUParseFrequencyLine(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 (virHostCPUParseFrequencyLine(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

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux