From: Peter Krempa <pkrempa(a)redhat.com&gt; Use 'g_autfree' for the two temporary strings. 'sysfs_cpudir' was used in two places, one of which is in a loop. Add another helper variable for it and declare the other one in the loop. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virhostcpu.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 3b4a11effb..c4138076d9 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -644,8 +644,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, int threads_per_subcore = 0; unsigned int node; int ret = -1; - char *sysfs_nodedir = NULL; - char *sysfs_cpudir = NULL; + g_autofree char *sysfs_nodedir = NULL; + g_autofree char *sysfs_cpudir_fallback = NULL; int direrr; *mhz = 0; @@ -707,6 +707,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, threads_per_subcore = 0; while ((direrr = virDirRead(nodedir, &nodedirent, sysfs_nodedir)) > 0) { + char *sysfs_cpudir = NULL; + if (sscanf(nodedirent->d_name, "node%u", &node) != 1) continue; @@ -723,8 +725,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, &nodethreads, &offline)) < 0) goto cleanup; - VIR_FREE(sysfs_cpudir); - *cpus += nodecpus; if (nodesockets > *sockets) @@ -744,11 +744,9 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, goto done; fallback: - VIR_FREE(sysfs_cpudir); - - sysfs_cpudir = g_strdup_printf("%s/cpu", SYSFS_SYSTEM_PATH); + sysfs_cpudir_fallback = g_strdup_printf("%s/cpu", SYSFS_SYSTEM_PATH); - if ((nodecpus = virHostCPUParseNode(sysfs_cpudir, arch, + if ((nodecpus = virHostCPUParseNode(sysfs_cpudir_fallback, arch, present_cpus_map, online_cpus_map, threads_per_subcore, @@ -799,8 +797,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, ret = 0; cleanup: - VIR_FREE(sysfs_nodedir); - VIR_FREE(sysfs_cpudir); return ret; } -- 2.49.0

From: Peter Krempa <pkrempa(a)redhat.com&gt; As the cleanup section is empty; the code can now return directly on errors. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virhostcpu.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index c4138076d9..2c4c75731c 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -643,7 +643,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, int nodecpus, nodecores, nodesockets, nodethreads, offline = 0; int threads_per_subcore = 0; unsigned int node; - int ret = -1; g_autofree char *sysfs_nodedir = NULL; g_autofree char *sysfs_cpudir_fallback = NULL; int direrr; @@ -659,12 +658,11 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, /* Get information about what CPUs are present in the host and what * CPUs are online, so that we don't have to so for each node */ - present_cpus_map = virHostCPUGetPresentBitmap(); - if (!present_cpus_map) - goto cleanup; - online_cpus_map = virHostCPUGetOnlineBitmap(); - if (!online_cpus_map) - goto cleanup; + if (!(present_cpus_map = virHostCPUGetPresentBitmap())) + return -1; + + if (!(online_cpus_map = virHostCPUGetOnlineBitmap())) + return -1; /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the * core, node, socket, thread and topology information from /sys @@ -699,7 +697,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, * On hosts other than POWER this will be 0, in which case a simpler * thread-counting logic will be used */ if ((threads_per_subcore = virHostCPUGetThreadsPerSubcore(arch)) < 0) - goto cleanup; + return -1; /* If the subcore configuration is not valid, just pretend subcores * are not in use and count threads one by one */ @@ -723,7 +721,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, threads_per_subcore, &nodesockets, &nodecores, &nodethreads, &offline)) < 0) - goto cleanup; + return -1; *cpus += nodecpus; @@ -738,7 +736,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, } if (direrr < 0) - goto cleanup; + return -1; if (*cpus && *nodes) goto done; @@ -752,7 +750,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, threads_per_subcore, &nodesockets, &nodecores, &nodethreads, &offline)) < 0) - goto cleanup; + return -1; *nodes = 1; *cpus = nodecpus; @@ -764,17 +762,17 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, /* There should always be at least one cpu, socket, node, and thread. */ if (*cpus == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no CPUs found")); - goto cleanup; + return -1; } if (*sockets == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no sockets found")); - goto cleanup; + return -1; } if (*threads == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no threads found")); - goto cleanup; + return -1; } /* Now check if the topology makes sense. There are machines that don't @@ -794,10 +792,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, *threads = 1; } - ret = 0; - - cleanup: - return ret; + return 0; } # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) -- 2.49.0

From: Peter Krempa <pkrempa(a)redhat.com&gt; virNodeGetInfo due to the rigid desing of the filled struct can't faithfully represent all topologies. Improve the description when that happens and outline the fallback topology. The function docs already state that users ought to use virConnectGetCapabilities() instead. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt-host.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 9845549e36..d82ff993c2 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -414,10 +414,13 @@ virConnectGetMaxVcpus(virConnectPtr conn, * speed that the first CPU in the machine is currently running at. This speed * may vary across CPUs and changes continually as the host OS throttles. * - * The nodes/sockets/cores/threads data is potentially inaccurate as - * it assumes a symmetric installation. If one NUMA node has more - * sockets populated that another NUMA node this information will be - * wrong. It is also not able to report about CPU dies. + * The virNodeInfo structure is not extensible thus only supports global + * nodes/sockets/cores/threads (sockets/cores/threads is per NUMA node) + * topology information. If the host CPU has any further groupings (e.g. + * dies, clusters, etc) or the NUMA topology is non-symmetrical the structure + * can't faithfully represent the system. In such cases a fake topology + * (nodes = 1, sockets = 1, cores = number of host cpus, threads = 1) which + * only correctly represents the total host CPU count is reported. * * Applications are recommended to use the virConnectGetCapabilities() * call instead, which provides all the information except CPU frequency, -- 2.49.0