Andrea Bolognani <abologna@redhat.com> [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@redhat.com> --- 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@redhat.com 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@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