[libvirt] [PATCH v2 2/4] util: virhostcpu: factor out frequency parsing

Bjoern Walk bwalk at linux.vnet.ibm.com
Thu Dec 14 05:45:45 UTC 2017


Pino Toscano <ptoscano at redhat.com> [2017-12-13, 05:35PM +0100]:
> On Wednesday, 13 December 2017 17:10:19 CET 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>
> > ---
> > [...]
> > +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 at 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 at 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 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 906 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171214/288f0d17/attachment-0001.sig>


More information about the libvir-list mailing list