[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