[libvirt] [PATCH 2/6] util: Introduce virHostCPUGetInfoParseCPUInfo()

[Andrea Bolognani]

On Wed, 2017-12-13 at 08:32 +0100, Bjoern Walk wrote:
> > -int
> > -virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
> > -                               virArch arch,
> > -                               unsigned int *cpus,
> > -                               unsigned int *mhz,
> > -                               unsigned int *nodes,
> > -                               unsigned int *sockets,
> > -                               unsigned int *cores,
> > -                               unsigned int *threads)
> > +
> > +static int
> > +virHostCPUGetInfoParseCPUInfo(FILE *cpuinfo,
> > +                              virArch arch,
> > +                              unsigned int *mhz)
> >  {
> > -    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;
> > -    int threads_per_subcore = 0;
> > -    unsigned int node;
> >      int ret = -1;
> > -    char *sysfs_nodedir = NULL;
> > -    char *sysfs_cpudir = NULL;
> > -    int direrr;
> > 
> >      *mhz = 0;
> 
> I wouldn't move this initialization.

So you'd leave it in virHostCPUGetInfoPopulateLinux()? Why?

With my approach, the value is changed only in
virHostCPUGetInfoParseCPUInfo(), which makes IMHO more sense than
spreading the code that changes it across two functions.

> > +    /* Start with parsing CPU clock speed from /proc/cpuinfo */
> > +    if (virHostCPUGetInfoParseCPUInfo(cpuinfo, arch, mhz) < 0)
> > +        goto cleanup;
> 
> Why do we cleanup here and abandon the rest of the information? Since
> the information in /proc/cpuinfo is kind of volatile in its format,
> shouldn't we be liberal in what we accept? If we can't parse it, we just
> report mhz = 0, but gathering the rest of the information in this
> function is still valuable.

Most functions in libvirt either perform all tasks succesfully or
return a failure, so failing here is in line both with that and
with the existing behavior.

-- 
Andrea Bolognani / Red Hat / Virtualization