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

[Andrea Bolognani]

On Thu, 2017-12-14 at 11:07 +0100, Pino Toscano wrote:
> On Thursday, 14 December 2017 10:46:33 CET Andrea Bolognani wrote:
> > On Wed, 2017-12-13 at 17:35 +0100, Pino Toscano wrote:
> > > > +    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;
> > 
> > We skip the prefix and pass the rest of the line to
> > virHostCPUParseFrequencyString(), which starts by skipping all
> > whitespace and then checking the first non-whitespace character
> > is a semicolon. So I don't see how we could end up matching
> > anything but the intended line.
> 
> Ah sorry, I did not explain all: the situation I see is that
> virHostCPUParseFrequencyString errors out if it does not find the
> colon.  Let's say that on x86_64 /proc/cpuinfo contains:
> 
>   cpu MHz new     : 1000.000
>   cpu MHz         : 2000.000
> 
> since "cpu MHz" is the prefix on x86, then the "cpu MHz new" line
> matches it so virHostCPUParseFrequencyString will be called, but then
> virHostCPUParseFrequencyString will error out because (after skipping
> spaces) it will find 'n'.  A failure in virHostCPUParseFrequencyString
> is propagated directly by virHostCPUParseFrequency, so the real key in
> cpuinfo will not be read.

You realize the change you propose[1] wouldn't deal properly with
your very example, right? ;)

I'll come up with something: not that I expect this to cause much
actual harm, but since we're fixing it already might as well go the
extra mile.


[1] Assuming passing '*str' rather than 'c' to c_isspace() is a
    mere pasto, which would be consistent with your explanation
    of its intended purpose.
-- 
Andrea Bolognani / Red Hat / Virtualization