nodeinfo: avoid probing host filesystem during test

We had previously weakened our nodeinfotest in order to ignore parsed
node values, because the parse function was mistakenly relying on
host files.  A better fix is to avoid using the numactl library, but
to instead parse the same files that numactl would read, all while
allowing the files to be relative to our choice of directory.

* src/nodeinfo.c (CPU_SYS_PATH, NODE_SYS_PATH): Replace with...
(SYSFS_SYSTEM_PATH): ...parent directory.
(linuxNodeInfoCPUPopulate): Check NUMA nodes from requested
directory (by inlining numactl code).
(nodeGetCPUmap, nodeGetMemoryStats): Adjust macro use.
* tests/nodeinfotest.c (linuxTestCompareFiles, linuxTestNodeInfo):
Update test to match.

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 7e68880f765814b788603471f8d6bd7a69bbd6ed..cb047c40f451597b20a83b80a61c2b23044a9881 100644 (file)
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -58,10 +58,9 @@
 
 #ifdef __linux__
 # define CPUINFO_PATH "/proc/cpuinfo"
-# define CPU_SYS_PATH "/sys/devices/system/cpu"
+# define SYSFS_SYSTEM_PATH "/sys/devices/system"
 # define PROCSTAT_PATH "/proc/stat"
 # define MEMINFO_PATH "/proc/meminfo"
-# define NODE_SYS_PATH "/sys/devices/system/node"
 
 # define LINUX_NB_CPU_STATS 4
 # define LINUX_NB_MEMORY_STATS_ALL 4
@@ -69,7 +68,7 @@
 
 /* NB, this is not static as we need to call it from the testsuite */
 int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
-                             const char *sysfs_cpudir,
+                             const char *sysfs_dir,
                              virNodeInfoPtr nodeinfo);
 
 static int linuxNodeGetCPUStats(FILE *procstat,
@@ -189,8 +188,43 @@ virNodeParseSocket(const char *dir, unsigned int cpu)
     return ret;
 }
 
+static int
+virNodeParseNode(const char *sysfs_dir)
+{
+    char *file = NULL;
+    char *possible = NULL;
+    char *tmp;
+    int ret = -1;
+
+    if (virAsprintf(&file, "%s/node/possible", sysfs_dir) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+    /* Assume that a missing node/possible file implies no NUMA
+     * support, and hence all cpus belong to the same node.  */
+    if (!virFileExists(file)) {
+        ret = 1;
+        goto cleanup;
+    }
+    if (virFileReadAll(file, 1024, &possible) < 0)
+        goto cleanup;
+    if (virStrToLong_i(possible, &tmp, 10, &ret) < 0 ||
+        (*tmp == '-' && virStrToLong_i(tmp+1, &tmp, 10, &ret) < 0) ||
+        *tmp != '\n') {
+        nodeReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("failed to parse possible nodes '%s'"), possible);
+        goto cleanup;
+    }
+    ret++;
+
+cleanup:
+    VIR_FREE(file);
+    VIR_FREE(possible);
+    return ret;
+}
+
 int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
-                             const char *sysfs_cpudir,
+                             const char *sysfs_dir,
                              virNodeInfoPtr nodeinfo)
 {
     char line[1024];
@@ -201,21 +235,15 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
     cpu_set_t core_mask;
     cpu_set_t socket_mask;
     int online;
+    int ret = -1;
+    char *sysfs_cpudir = NULL;
 
     nodeinfo->cpus = 0;
     nodeinfo->mhz = 0;
     nodeinfo->cores = 0;
 
-    nodeinfo->nodes = 1;
-# if HAVE_NUMACTL
-    if (numa_available() >= 0)
-        nodeinfo->nodes = numa_max_node() + 1;
-# endif
-
-    /* NB: It is impossible to fill our nodes, since cpuinfo
-     * has no knowledge of NUMA nodes */
-
-    /* NOTE: hyperthreads are ignored here; they are parsed out of /sys */
+    /* Start with parsing /proc/cpuinfo; although it tends to have
+     * fewer details.  Hyperthreads are ignored at this stage.  */
     while (fgets(line, sizeof(line), cpuinfo) != NULL) {
 # if defined(__x86_64__) || \
     defined(__amd64__)  || \
@@ -230,7 +258,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
             if (*buf != ':' || !buf[1]) {
                 nodeReportError(VIR_ERR_INTERNAL_ERROR,
                                 "%s", _("parsing cpuinfo cpu MHz"));
-                return -1;
+                goto cleanup;
             }
             if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0
                 /* Accept trailing fractional part.  */
@@ -249,7 +277,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
             if (*buf != ':' || !buf[1]) {
                 nodeReportError(VIR_ERR_INTERNAL_ERROR,
                                 "%s", _("parsing cpuinfo cpu MHz"));
-                return -1;
+                goto cleanup;
             }
             if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0
                 /* Accept trailing fractional part.  */
@@ -266,13 +294,17 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
 # endif
     }
 
-    /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the core, socket
-     * thread and topology information from /sys
+    /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the
+     * core, node, socket, thread and topology information from /sys
      */
+    if (virAsprintf(&sysfs_cpudir, "%s/cpu", sysfs_dir) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
     cpudir = opendir(sysfs_cpudir);
     if (cpudir == NULL) {
         virReportSystemError(errno, _("cannot opendir %s"), sysfs_cpudir);
-        return -1;
+        goto cleanup;
     }
 
     CPU_ZERO(&core_mask);
@@ -285,7 +317,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
         online = virNodeGetCpuValue(sysfs_cpudir, cpu, "online", true);
         if (online < 0) {
             closedir(cpudir);
-            return -1;
+            goto cleanup;
         }
         if (!online)
             continue;
@@ -308,7 +340,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
         cur_threads = virNodeCountThreadSiblings(sysfs_cpudir, cpu);
         if (cur_threads == 0) {
             closedir(cpudir);
-            return -1;
+            goto cleanup;
         }
         if (cur_threads > nodeinfo->threads)
             nodeinfo->threads = cur_threads;
@@ -317,26 +349,32 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
         virReportSystemError(errno,
                              _("problem reading %s"), sysfs_cpudir);
         closedir(cpudir);
-        return -1;
+        goto cleanup;
+    }
+    if (closedir(cpudir) < 0) {
+        virReportSystemError(errno,
+                             _("problem closing %s"), sysfs_cpudir);
+        goto cleanup;
     }
 
-    closedir(cpudir);
+    if ((nodeinfo->nodes = virNodeParseNode(sysfs_dir)) <= 0)
+        goto cleanup;
 
-    /* there should always be at least one cpu, socket and one thread */
+    /* There should always be at least one cpu, socket, node, and thread. */
     if (nodeinfo->cpus == 0) {
         nodeReportError(VIR_ERR_INTERNAL_ERROR,
                         "%s", _("no CPUs found"));
-        return -1;
+        goto cleanup;
     }
     if (nodeinfo->sockets == 0) {
         nodeReportError(VIR_ERR_INTERNAL_ERROR,
                         "%s", _("no sockets found"));
-        return -1;
+        goto cleanup;
     }
     if (nodeinfo->threads == 0) {
         nodeReportError(VIR_ERR_INTERNAL_ERROR,
                         "%s", _("no threads found"));
-        return -1;
+        goto cleanup;
     }
 
     /* nodeinfo->sockets is supposed to be a number of sockets per NUMA node,
@@ -349,7 +387,11 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
     else
         nodeinfo->nodes = 1;
 
-    return 0;
+    ret = 0;
+
+cleanup:
+    VIR_FREE(sysfs_cpudir);
+    return ret;
 }
 
 # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))
@@ -620,7 +662,7 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) {
         return -1;
     }
 
-    ret = linuxNodeInfoCPUPopulate(cpuinfo, CPU_SYS_PATH, nodeinfo);
+    ret = linuxNodeInfoCPUPopulate(cpuinfo, SYSFS_SYSTEM_PATH, nodeinfo);
     if (ret < 0)
         goto cleanup;
 
@@ -707,8 +749,8 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED,
             }
 # endif
 
-            if (virAsprintf(&meminfo_path, "%s/node%d/meminfo",
-                            NODE_SYS_PATH, cellNum) < 0) {
+            if (virAsprintf(&meminfo_path, "%s/node/node%d/meminfo",
+                            SYSFS_SYSTEM_PATH, cellNum) < 0) {
                 virReportOOMError();
                 return -1;
             }
@@ -743,7 +785,7 @@ nodeGetCPUmap(virConnectPtr conn ATTRIBUTE_UNUSED,
     char *path;
     char *cpumap;
 
-    if (virAsprintf(&path, CPU_SYS_PATH "/%s", mapname) < 0) {
+    if (virAsprintf(&path, SYSFS_SYSTEM_PATH "/cpu/%s", mapname) < 0) {
         virReportOOMError();
         return NULL;
     }

diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
index 74bf811c68c53be63955a972bef77b10e80fd4aa..872a526bdc7c25b7d3710ef44d0f3aa0746d714e 100644 (file)
--- a/tests/nodeinfotest.c
+++ b/tests/nodeinfotest.c
@@ -26,12 +26,12 @@ main(void)
 #else
 
 extern int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
-                                    char *sysfs_cpuinfo,
+                                    char *sysfs_dir,
                                     virNodeInfoPtr nodeinfo);
 
 static int
 linuxTestCompareFiles(const char *cpuinfofile,
-                      char *sysfs_cpuinfo,
+                      char *sysfs_dir,
                       const char *outputfile)
 {
     int ret = -1;
@@ -48,7 +48,7 @@ linuxTestCompareFiles(const char *cpuinfofile,
         goto fail;
 
     memset(&nodeinfo, 0, sizeof(nodeinfo));
-    if (linuxNodeInfoCPUPopulate(cpuinfo, sysfs_cpuinfo, &nodeinfo) < 0) {
+    if (linuxNodeInfoCPUPopulate(cpuinfo, sysfs_dir, &nodeinfo) < 0) {
         if (virTestGetDebug()) {
             virErrorPtr error = virSaveLastError();
             if (error && error->code != VIR_ERR_OK)
@@ -60,21 +60,13 @@ linuxTestCompareFiles(const char *cpuinfofile,
     }
     VIR_FORCE_FCLOSE(cpuinfo);
 
-    /* 'nodes' is filled using libnuma.so from current machine
-     * topology, which makes it unsuitable for the test suite
-     * so blank it to a predictable value */
-    nodeinfo.nodes = 1;
-
     if (virAsprintf(&actualData, "CPUs: %u, MHz: %u, Nodes: %u, Cores: %u\n",
                     nodeinfo.cpus, nodeinfo.mhz, nodeinfo.nodes,
                     nodeinfo.cores) < 0)
         goto fail;
 
     if (STRNEQ(actualData, expectData)) {
-        if (getenv("DEBUG_TESTS")) {
-            printf("Expect %d '%s'\n", (int)strlen(expectData), expectData);
-            printf("Actual %d '%s'\n", (int)strlen(actualData), actualData);
-        }
+        virtTestDifference(stderr, expectData, actualData);
         goto fail;
     }
 
@@ -92,34 +84,31 @@ linuxTestNodeInfo(const void *data)
 {
     int result = -1;
     char *cpuinfo = NULL;
-    char *sysfs_cpuinfo = NULL;
+    char *sysfs_dir = NULL;
     char *output = NULL;
+    const char *test = data;
+    const char *arch = "x86";
 
 # if defined(__powerpc__) || \
      defined(__powerpc64__)
-    if (virAsprintf(&sysfs_cpuinfo, "%s/nodeinfodata/linux-%s/cpu/",
-                    abs_srcdir, (const char*)data) < 0 ||
-        virAsprintf(&cpuinfo, "%s/nodeinfodata/linux-%s-ppc.cpuinfo",
-                    abs_srcdir, (const char*)data) < 0 ||
-        virAsprintf(&output, "%s/nodeinfodata/linux-%s-cpu-ppc-output.txt",
-                    abs_srcdir, (const char*)data) < 0) {
-# else
-    if (virAsprintf(&sysfs_cpuinfo, "%s/nodeinfodata/linux-%s/cpu/",
-                    abs_srcdir, (const char*)data) < 0 ||
-        virAsprintf(&cpuinfo, "%s/nodeinfodata/linux-%s-x86.cpuinfo",
-                    abs_srcdir, (const char*)data) < 0 ||
-        virAsprintf(&output, "%s/nodeinfodata/linux-%s-cpu-x86-output.txt",
-                    abs_srcdir, (const char*)data) < 0) {
+    arch = "ppc";
 # endif
+
+    if (virAsprintf(&sysfs_dir, "%s/nodeinfodata/linux-%s",
+                    abs_srcdir, test) < 0 ||
+        virAsprintf(&cpuinfo, "%s/nodeinfodata/linux-%s-%s.cpuinfo",
+                    abs_srcdir, test, arch) < 0 ||
+        virAsprintf(&output, "%s/nodeinfodata/linux-%s-cpu-%s-output.txt",
+                    abs_srcdir, test, arch) < 0) {
         goto cleanup;
     }
 
-    result = linuxTestCompareFiles(cpuinfo, sysfs_cpuinfo, output);
+    result = linuxTestCompareFiles(cpuinfo, sysfs_dir, output);
 
 cleanup:
     VIR_FREE(cpuinfo);
     VIR_FREE(output);
-    VIR_FREE(sysfs_cpuinfo);
+    VIR_FREE(sysfs_dir);
 
     return result;
 }