Re: [libvirt] [PATCH] Fix virCgroupGetPercpuStats with non-continuous present CPUs

On 01/22/2015 03:21 PM, Michal Privoznik wrote: > On 22.01.2015 12:26, Ján Tomko wrote: >> Per-cpu stats are only shown for present CPUs in the cgroups, >> but we were only parsing the largest CPU number from >> /sys/devices/system/cpu/present and looking for stats even for >> non-present CPUs. >> This resulted in: >> internal error: cpuacct parse error >> --- >> cfg.mk | 2 +- >> src/nodeinfo.c | 17 +++++++++++++++++ >> src/nodeinfo.h | 1 + >> src/util/vircgroup.c | 24 ++++++++++++++++++------ >> tests/vircgroupmock.c | 44 ++++++++++++++++++++++++++++++++++++++------ >> tests/vircgrouptest.c | 47 +++++++++++++++++++++++++++++++++++------------ >> 6 files changed, 110 insertions(+), 25 deletions(-) >> >> diff --git a/cfg.mk b/cfg.mk >> index 21f83c3..70612f8 100644 >> --- a/cfg.mk >> +++ b/cfg.mk >> @@ -1095,7 +1095,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ >> ^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) >> >> exclude_file_name_regexp--sc_prohibit_strdup = \ >> - ^(docs/|examples/|src/util/virstring\.c|tests/virnetserverclientmock.c$$) >> + ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$) >> >> exclude_file_name_regexp--sc_prohibit_close = \ >> (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$) >> diff --git a/src/nodeinfo.c b/src/nodeinfo.c >> index 3c22ebc..3a27c22 100644 >> --- a/src/nodeinfo.c >> +++ b/src/nodeinfo.c >> @@ -1242,6 +1242,23 @@ nodeGetCPUCount(void) >> } >> >> virBitmapPtr >> +nodeGetPresentCPUBitmap(void) >> +{ >> + int max_present; >> + >> + if ((max_present = nodeGetCPUCount()) < 0) >> + return NULL; >> + >> +#ifdef __linux__ >> + if (virFileExists(SYSFS_SYSTEM_PATH "/cpu/present")) >> + return linuxParseCPUmap(max_present, SYSFS_SYSTEM_PATH "/cpu/present"); >> +#endif >> + virReportError(VIR_ERR_NO_SUPPORT, "%s", >> + _("non-continuous host cpu numbers not implemented on this platform")); >> + return NULL; >> +} >> + >> +virBitmapPtr >> nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED) >> { > > >From my understanding the nodeGetPresentCPUBitmap() is no different than > nodeGetCPUBitmap(). The latter can do everything that the former and > more. Can we just use already existing function then? > This functions checks the present CPUs, nodeGetCPUBitmap checks the online CPUs. I don't think splitting their common part into another function would be more readable - IMO the fact that both use linuxParseCPUmap should be enough.