[libvirt] [PATCH v3 2/5] util: virhostcpu: factor out frequency parsing

[John Ferlan]

On 12/14/2017 07:33 AM, Andrea Bolognani wrote:
> From: Bjoern Walk <bwalk at 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 at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Andrea Bolognani <abologna at redhat.com>
> Signed-off-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
> ---
>  src/util/virhostcpu.c | 141 ++++++++++++++++++++++----------------------------
>  1 file changed, 62 insertions(+), 79 deletions(-)
> 

Couple of nits... see below

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index c485a9721..d47062013 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -508,6 +508,65 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
>      return ret;
>  }
>  

Two blank lines between functions...

> +static int
> +virHostCPUParseFrequencyString(const char *str,
> +                               const char *prefix,
> +                               unsigned int *mhz)
> +{
> +    char *p;
> +    unsigned int ui;
> +
> +    if (!STRPREFIX(str, prefix))
> +        return 1;
> +
> +    str += strlen(prefix);
> +
> +    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;
> +}
> +

blank line

> +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("Your architecture is not supported by the %s parser",
> +                 CPUINFO_PATH);
> +        return 1;
> +    }
> +
> +    while (fgets(line, sizeof(line), cpuinfo) != NULL) {
> +        if (virHostCPUParseFrequencyString(line, prefix, mhz) < 0)
> +            return -1;
> +    }
> +
> +    return 0;
> +}
> +

extra blank line...

>  int
>  virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
>                                 virArch arch,
> @@ -520,7 +579,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 +593,9 @@ 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 CPU frequency information from %s",
> +                 CPUINFO_PATH);
>  
>      /* 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 */
>