Fix virCgroupGetPercpuStats with non-continuous present CPUs

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

6 files changed, 110 insertions, 25 deletions

diff --git a/cfg.mk b/cfg.mk
index 21f83c3435..70612f8e03 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 3c22ebc584..3a27c22eef 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)
 {
 #ifdef __linux__
diff --git a/src/nodeinfo.h b/src/nodeinfo.h
index a993c6367d..047bd5ca64 100644
--- a/src/nodeinfo.h
+++ b/src/nodeinfo.h
@@ -43,6 +43,7 @@ int nodeGetCellsFreeMemory(unsigned long long *freeMems,
 int nodeGetMemory(unsigned long long *mem,
                   unsigned long long *freeMem);
 
+virBitmapPtr nodeGetPresentCPUBitmap(void);
 virBitmapPtr nodeGetCPUBitmap(int *max_id);
 int nodeGetCPUCount(void);
 
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index fe34290aca..e65617a8a4 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2985,7 +2985,8 @@ static int
 virCgroupGetPercpuVcpuSum(virCgroupPtr group,
                           unsigned int nvcpupids,
                           unsigned long long *sum_cpu_time,
-                          unsigned int num)
+                          size_t nsum,
+                          virBitmapPtr cpumap)
 {
     int ret = -1;
     size_t i;
@@ -2995,7 +2996,7 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group,
     for (i = 0; i < nvcpupids; i++) {
         char *pos;
         unsigned long long tmp;
-        size_t j;
+        ssize_t j;
 
         if (virCgroupNewVcpu(group, i, false, &group_vcpu) < 0)
             goto cleanup;
@@ -3004,7 +3005,9 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group,
             goto cleanup;
 
         pos = buf;
-        for (j = 0; j < num; j++) {
+        for (j = virBitmapNextSetBit(cpumap, -1);
+             j >= 0 && j < nsum;
+             j = virBitmapNextSetBit(cpumap, j)) {
             if (virStrToLong_ull(pos, &pos, 10, &tmp) < 0) {
                 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                                _("cpuacct parse error"));
@@ -3042,6 +3045,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,
     virTypedParameterPtr ent;
     int param_idx;
     unsigned long long cpu_time;
+    virBitmapPtr cpumap = NULL;
 
     /* return the number of supported params */
     if (nparams == 0 && ncpus != 0) {
@@ -3052,9 +3056,11 @@ virCgroupGetPercpuStats(virCgroupPtr group,
     }
 
     /* To parse account file, we need to know how many cpus are present.  */
-    if ((total_cpus = nodeGetCPUCount()) < 0)
+    if (!(cpumap = nodeGetPresentCPUBitmap()))
         return rv;
 
+    total_cpus = virBitmapSize(cpumap);
+
     if (ncpus == 0)
         return total_cpus;
 
@@ -3077,7 +3083,11 @@ virCgroupGetPercpuStats(virCgroupPtr group,
     need_cpus = MIN(total_cpus, start_cpu + ncpus);
 
     for (i = 0; i < need_cpus; i++) {
-        if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) {
+        bool present;
+        ignore_value(virBitmapGetBit(cpumap, i, &present));
+        if (!present) {
+            cpu_time = 0;
+        } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("cpuacct parse error"));
             goto cleanup;
@@ -3097,7 +3107,8 @@ virCgroupGetPercpuStats(virCgroupPtr group,
 
     if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0)
         goto cleanup;
-    if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, need_cpus) < 0)
+    if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, need_cpus,
+                                  cpumap) < 0)
         goto cleanup;
 
     for (i = start_cpu; i < need_cpus; i++) {
@@ -3113,6 +3124,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,
     rv = param_idx + 1;
 
  cleanup:
+    virBitmapFree(cpumap);
     VIR_FREE(sum_cpu_time);
     VIR_FREE(buf);
     return rv;
diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
index 6e0a0e7c32..f2fee7d1ba 100644
--- a/tests/vircgroupmock.c
+++ b/tests/vircgroupmock.c
@@ -51,6 +51,8 @@ const char *fakedevicedir1 = FAKEDEVDIR1;
 
 
 # define SYSFS_PREFIX "/not/really/sys/fs/cgroup/"
+# define SYSFS_CPU_PRESENT "/sys/devices/system/cpu/present"
+# define SYSFS_CPU_PRESENT_MOCKED "devices_system_cpu_present"
 
 /*
  * The plan:
@@ -215,7 +217,15 @@ static int make_controller(const char *path, mode_t mode)
                   "user 216687025\n"
                   "system 43421396\n");
         MAKE_FILE("cpuacct.usage", "2787788855799582\n");
-        MAKE_FILE("cpuacct.usage_percpu", "1413142688153030 1374646168910542\n");
+        MAKE_FILE("cpuacct.usage_percpu",
+                  "7059492996 0 0 0 0 0 0 0 4180532496 0 0 0 0 0 0 0 "
+                  "1957541268 0 0 0 0 0 0 0 2065932204 0 0 0 0 0 0 0 "
+                  "18228689414 0 0 0 0 0 0 0 4245525148 0 0 0 0 0 0 0 "
+                  "2911161568 0 0 0 0 0 0 0 1407758136 0 0 0 0 0 0 0 "
+                  "1836807700 0 0 0 0 0 0 0 1065296618 0 0 0 0 0 0 0 "
+                  "2046213266 0 0 0 0 0 0 0 747889778 0 0 0 0 0 0 0 "
+                  "709566900 0 0 0 0 0 0 0 444777342 0 0 0 0 0 0 0 "
+                  "5683512916 0 0 0 0 0 0 0 635751356 0 0 0 0 0 0 0\n");
     } else if (STRPREFIX(controller, "cpuset")) {
         MAKE_FILE("cpuset.cpu_exclusive", "1\n");
         if (STREQ(controller, "cpuset"))
@@ -431,6 +441,9 @@ static void init_sysfs(void)
     MAKE_CONTROLLER("blkio");
     MAKE_CONTROLLER("memory");
     MAKE_CONTROLLER("freezer");
+
+    if (make_file(fakesysfsdir, SYSFS_CPU_PRESENT_MOCKED, "8-23,48-159\n") < 0)
+        abort();
 }
 
 
@@ -623,21 +636,27 @@ int __xstat(int ver, const char *path, struct stat *sb)
 
 int stat(const char *path, struct stat *sb)
 {
+    char *newpath = NULL;
     int ret;
 
     init_syms();
 
-    if (STRPREFIX(path, SYSFS_PREFIX)) {
+    if (STREQ(path, SYSFS_CPU_PRESENT)) {
+        init_sysfs();
+        if (asprintf(&newpath, "%s/%s",
+                     fakesysfsdir,
+                     SYSFS_CPU_PRESENT_MOCKED) < 0) {
+            errno = ENOMEM;
+            return -1;
+        }
+    } else if (STRPREFIX(path, SYSFS_PREFIX)) {
         init_sysfs();
-        char *newpath;
         if (asprintf(&newpath, "%s/%s",
                      fakesysfsdir,
                      path + strlen(SYSFS_PREFIX)) < 0) {
             errno = ENOMEM;
             return -1;
         }
-        ret = realstat(newpath, sb);
-        free(newpath);
     } else if (STRPREFIX(path, fakedevicedir0)) {
         sb->st_mode = S_IFBLK;
         sb->st_rdev = makedev(8, 0);
@@ -647,8 +666,11 @@ int stat(const char *path, struct stat *sb)
         sb->st_rdev = makedev(9, 0);
         return 0;
     } else {
-        ret = realstat(path, sb);
+        if (!(newpath = strdup(path)))
+            return -1;
     }
+    ret = realstat(newpath, sb);
+    free(newpath);
     return ret;
 }
 
@@ -682,6 +704,16 @@ int open(const char *path, int flags, ...)
 
     init_syms();
 
+    if (STREQ(path, SYSFS_CPU_PRESENT)) {
+        init_sysfs();
+        if (asprintf(&newpath, "%s/%s",
+                     fakesysfsdir,
+                     SYSFS_CPU_PRESENT_MOCKED) < 0) {
+            errno = ENOMEM;
+            return -1;
+        }
+    }
+
     if (STRPREFIX(path, SYSFS_PREFIX)) {
         init_sysfs();
         if (asprintf(&newpath, "%s/%s",
diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
index 35ac0c0067..b65ea3fba5 100644
--- a/tests/vircgrouptest.c
+++ b/tests/vircgrouptest.c
@@ -538,12 +538,35 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED)
     virCgroupPtr cgroup = NULL;
     size_t i;
     int rv, ret = -1;
-    virTypedParameter params[2];
+    virTypedParameterPtr params = NULL;
+# define EXPECTED_NCPUS 160
 
-    // TODO: mock nodeGetCPUCount() as well & check 2nd cpu, too
     unsigned long long expected[] = {
-        1413142688153030ULL
+        0, 0, 0, 0, 0, 0, 0, 0,
+        7059492996, 0, 0, 0, 0, 0, 0, 0,
+        4180532496, 0, 0, 0, 0, 0, 0, 0,
+        0, 0, 0, 0, 0, 0, 0, 0,
+        0, 0, 0, 0, 0, 0, 0, 0,
+        0, 0, 0, 0, 0, 0, 0, 0,
+        1957541268, 0, 0, 0, 0, 0, 0, 0,
+        2065932204, 0, 0, 0, 0, 0, 0, 0,
+        18228689414, 0, 0, 0, 0, 0, 0, 0,
+        4245525148, 0, 0, 0, 0, 0, 0, 0,
+        2911161568, 0, 0, 0, 0, 0, 0, 0,
+        1407758136, 0, 0, 0, 0, 0, 0, 0,
+        1836807700, 0, 0, 0, 0, 0, 0, 0,
+        1065296618, 0, 0, 0, 0, 0, 0, 0,
+        2046213266, 0, 0, 0, 0, 0, 0, 0,
+        747889778, 0, 0, 0, 0, 0, 0, 0,
+        709566900, 0, 0, 0, 0, 0, 0, 0,
+        444777342, 0, 0, 0, 0, 0, 0, 0,
+        5683512916, 0, 0, 0, 0, 0, 0, 0,
+        635751356, 0, 0, 0, 0, 0, 0, 0,
     };
+    verify(ARRAY_CARDINALITY(expected) == EXPECTED_NCPUS);
+
+    if (VIR_ALLOC_N(params, EXPECTED_NCPUS) < 0)
+        goto cleanup;
 
     if ((rv = virCgroupNewPartition("/virtualmachines", true,
                                     (1 << VIR_CGROUP_CONTROLLER_CPU) |
@@ -553,37 +576,37 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (nodeGetCPUCount() < 1) {
+    if (nodeGetCPUCount() != EXPECTED_NCPUS) {
         fprintf(stderr, "Unexpected: nodeGetCPUCount() yields: %d\n", nodeGetCPUCount());
         goto cleanup;
     }
 
     if ((rv = virCgroupGetPercpuStats(cgroup,
                                       params,
-                                      2, 0, 1, 0)) < 0) {
+                                      1, 0, EXPECTED_NCPUS, 0)) < 0) {
         fprintf(stderr, "Failed call to virCgroupGetPercpuStats for /virtualmachines cgroup: %d\n", -rv);
         goto cleanup;
     }
 
-    for (i = 0; i < ARRAY_CARDINALITY(expected); i++) {
+    for (i = 0; i < EXPECTED_NCPUS; i++) {
         if (!STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME)) {
             fprintf(stderr,
-                    "Wrong parameter name value from virCgroupGetPercpuStats (is: %s)\n",
-                    params[i].field);
+                    "Wrong parameter name value from virCgroupGetPercpuStats at %zu (is: %s)\n",
+                    i, params[i].field);
             goto cleanup;
         }
 
         if (params[i].type != VIR_TYPED_PARAM_ULLONG) {
             fprintf(stderr,
-                    "Wrong parameter value type from virCgroupGetPercpuStats (is: %d)\n",
-                    params[i].type);
+                    "Wrong parameter value type from virCgroupGetPercpuStats at %zu (is: %d)\n",
+                    i, params[i].type);
             goto cleanup;
         }
 
         if (params[i].value.ul != expected[i]) {
             fprintf(stderr,
-                    "Wrong value from virCgroupGetMemoryUsage (expected %llu)\n",
-                    params[i].value.ul);
+                    "Wrong value from virCgroupGetMemoryUsage at %zu (expected %llu)\n",
+                    i, params[i].value.ul);
             goto cleanup;
         }
     }