Fix up nodeinfo parsing code.

As pointed out by eblake, I made a real hash of the
nodeinfo code with commit
aa2f6f96dd.  This patch
cleans it up:

1)  Do more work at compile time instead of runtime (minor)
2)  Properly handle the hex digits that come from
/sys/devices/system/cpu/cpu*/topology/thread_siblings
3)  Fix up some error paths that could cause SEGV
4)  Used unsigned's for the cpu numbers (cpu -1 doesn't
make any sense)

Along with the recent patch from jdenemar to zero out
the nodeinfo structure, I've re-tested this on the
machines having the problems, and it seems to be good.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
This commit is contained in:
Chris Lalancette 2010-03-09 10:17:59 -05:00
parent 89bf843a6d
commit 7be9270c24

View File

@ -63,15 +63,15 @@
int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo,
virNodeInfoPtr nodeinfo); virNodeInfoPtr nodeinfo);
static unsigned long count_thread_siblings(int cpu) static unsigned long count_thread_siblings(unsigned int cpu)
{ {
unsigned long ret = 0; unsigned long ret = 0;
char *path = NULL; char *path;
FILE *pathfp = NULL; FILE *pathfp;
char str[1024]; char str[1024];
int i; int i;
if (virAsprintf(&path, "%s/cpu%d/topology/thread_siblings", CPU_SYS_PATH, if (virAsprintf(&path, CPU_SYS_PATH "/cpu%u/topology/thread_siblings",
cpu) < 0) { cpu) < 0) {
virReportOOMError(); virReportOOMError();
return 0; return 0;
@ -91,8 +91,12 @@ static unsigned long count_thread_siblings(int cpu)
i = 0; i = 0;
while (str[i] != '\0') { while (str[i] != '\0') {
if (str[i] != '\n' && str[i] != ',') if (c_isdigit(str[i]))
ret += count_one_bits(str[i] - '0'); ret += count_one_bits(str[i] - '0');
else if (str[i] >= 'A' && str[i] <= 'F')
ret += count_one_bits(str[i] - 'A' + 10);
else if (str[i] >= 'a' && str[i] <= 'f')
ret += count_one_bits(str[i] - 'a' + 10);
i++; i++;
} }
@ -103,16 +107,16 @@ cleanup:
return ret; return ret;
} }
static int parse_socket(int cpu) static int parse_socket(unsigned int cpu)
{ {
char *path = NULL; char *path;
FILE *pathfp; FILE *pathfp;
char socket_str[1024]; char socket_str[1024];
char *tmp; char *tmp;
int socket; int socket = -1;
if (virAsprintf(&path, "%s/cpu%d/topology/physical_package_id", if (virAsprintf(&path, CPU_SYS_PATH "/cpu%u/topology/physical_package_id",
CPU_SYS_PATH, cpu) < 0) { cpu) < 0) {
virReportOOMError(); virReportOOMError();
return -1; return -1;
} }
@ -120,7 +124,8 @@ static int parse_socket(int cpu)
pathfp = fopen(path, "r"); pathfp = fopen(path, "r");
if (pathfp == NULL) { if (pathfp == NULL) {
virReportSystemError(errno, _("cannot open %s"), path); virReportSystemError(errno, _("cannot open %s"), path);
goto cleanup; VIR_FREE(path);
return -1;
} }
if (fgets(socket_str, sizeof(socket_str), pathfp) == NULL) { if (fgets(socket_str, sizeof(socket_str), pathfp) == NULL) {
@ -147,7 +152,7 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo,
char line[1024]; char line[1024];
DIR *cpudir = NULL; DIR *cpudir = NULL;
struct dirent *cpudirent = NULL; struct dirent *cpudirent = NULL;
int cpu; unsigned int cpu;
unsigned long cur_threads; unsigned long cur_threads;
int socket; int socket;
unsigned long long socket_mask = 0; unsigned long long socket_mask = 0;
@ -157,7 +162,7 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo,
nodeinfo->nodes = nodeinfo->cores = 1; nodeinfo->nodes = nodeinfo->cores = 1;
/* NB: It is impossible to fill our nodes, since cpuinfo /* NB: It is impossible to fill our nodes, since cpuinfo
* has not knowledge of NUMA nodes */ * has no knowledge of NUMA nodes */
/* NOTE: hyperthreads are ignored here; they are parsed out of /sys */ /* NOTE: hyperthreads are ignored here; they are parsed out of /sys */
while (fgets(line, sizeof(line), cpuinfo) != NULL) { while (fgets(line, sizeof(line), cpuinfo) != NULL) {
@ -220,7 +225,7 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo,
return -1; return -1;
} }
while ((cpudirent = readdir(cpudir))) { while ((cpudirent = readdir(cpudir))) {
if (sscanf(cpudirent->d_name, "cpu%d", &cpu) != 1) if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
continue; continue;
socket = parse_socket(cpu); socket = parse_socket(cpu);
@ -244,6 +249,18 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo,
closedir(cpudir); closedir(cpudir);
/* there should always be at least one socket and one thread */
if (nodeinfo->sockets == 0) {
nodeReportError(conn, VIR_ERR_INTERNAL_ERROR,
"%s", _("no sockets found"));
return -1;
}
if (nodeinfo->threads == 0) {
nodeReportError(conn, VIR_ERR_INTERNAL_ERROR,
"%s", _("no threads found"));
return -1;
}
return 0; return 0;
} }