On 02/16/2015 01:51 PM, Peter Krempa wrote:
Rewrite the function to save a few local variables and reorder the
code
to make more sense.
Additionally the ncells_max member of the virCPUDef structure is used
only for tracking alocation when parsing the numa definition, which can
a/alocation/allocation
be avoided by switching to VIR_ALLOC_N as the array is not resized
after initial allocation.
Looks like initial implementation added RESIZE_N commit id '5f7b71b4'
and 'ncells_max' for some reason...
---
src/conf/cpu_conf.c | 1 -
src/conf/cpu_conf.h | 1 -
src/conf/numa_conf.c | 129 +++++++++++++++++++++++---------------------------
tests/testutilsqemu.c | 1 -
4 files changed, 58 insertions(+), 74 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index f14b37a..98b309b 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -154,7 +154,6 @@ virCPUDefCopy(const virCPUDef *cpu)
if (cpu->ncells) {
if (VIR_ALLOC_N(copy->cells, cpu->ncells) < 0)
goto error;
- copy->ncells_max = copy->ncells = cpu->ncells;
for (i = 0; i < cpu->ncells; i++) {
copy->cells[i].mem = cpu->cells[i].mem;
diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index 46fce25..e201656 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -127,7 +127,6 @@ struct _virCPUDef {
size_t nfeatures_max;
virCPUFeatureDefPtr features;
size_t ncells;
- size_t ncells_max;
virCellDefPtr cells;
unsigned int cells_cpus;
};
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index e36a4be..c50369d 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -688,45 +688,36 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def,
{
xmlNodePtr *nodes = NULL;
xmlNodePtr oldNode = ctxt->node;
+ char *tmp = NULL;
int n;
size_t i;
int ret = -1;
- if (virXPathNode("/domain/cpu/numa[1]", ctxt)) {
- VIR_FREE(nodes);
+ /* check if NUMA definition is present */
+ if (!virXPathNode("/domain/cpu/numa[1]", ctxt))
+ return 0;
- n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes);
- if (n <= 0) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("NUMA topology defined without NUMA cells"));
- goto error;
- }
+ if ((n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes))
<= 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("NUMA topology defined without NUMA cells"));
+ goto cleanup;
+ }
+
+ if (VIR_ALLOC_N(def->cells, n) < 0)
+ goto cleanup;
+ def->ncells = n;
- if (VIR_RESIZE_N(def->cells, def->ncells_max,
- def->ncells, n) < 0)
- goto error;
-
- def->ncells = n;
-
- for (i = 0; i < n; i++) {
- char *cpus, *memAccessStr;
- int rc, ncpus = 0;
- unsigned int cur_cell;
- char *tmp = NULL;
-
- tmp = virXMLPropString(nodes[i], "id");
- if (!tmp) {
- cur_cell = i;
- } else {
- rc = virStrToLong_ui(tmp, NULL, 10, &cur_cell);
- if (rc == -1) {
- virReportError(VIR_ERR_XML_ERROR,
- _("Invalid 'id' attribute in NUMA cell:
%s"),
- tmp);
- VIR_FREE(tmp);
- goto error;
- }
- VIR_FREE(tmp);
+ for (i = 0; i < n; i++) {
+ int rc, ncpus = 0;
+ unsigned int cur_cell = i;
+
+ /* cells are in order of parsing or explicitly numbered */
+ if ((tmp = virXMLPropString(nodes[i], "id"))) {
+ if (virStrToLong_uip(tmp, NULL, 10, &cur_cell) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid 'id' attribute in NUMA cell:
'%s'"),
+ tmp);
+ goto cleanup;
}
if (cur_cell >= n) {
@@ -734,60 +725,56 @@ virDomainNumaDefCPUParseXML(virCPUDefPtr def,
_("Exactly one 'cell' element per guest
"
"NUMA cell allowed, non-contiguous ranges or
"
"ranges not starting from 0 are not
allowed"));
- goto error;
- }
-
- if (def->cells[cur_cell].cpustr) {
- virReportError(VIR_ERR_XML_ERROR,
- _("Duplicate NUMA cell info for cell id
'%u'"),
- cur_cell);
- goto error;
+ goto cleanup;
}
+ }
+ VIR_FREE(tmp);
- cpus = virXMLPropString(nodes[i], "cpus");
- if (!cpus) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Missing 'cpus' attribute in NUMA
cell"));
- goto error;
- }
- def->cells[cur_cell].cpustr = cpus;
+ if (def->cells[cur_cell].cpumask) {
using cpumask instead of cpustr - I guess it doesn't matter...
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Duplicate NUMA cell info for cell id
'%u'"),
+ cur_cell);
+ goto cleanup;
+ }
- ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask,
- VIR_DOMAIN_CPUMASK_LEN);
- if (ncpus <= 0)
- goto error;
- def->cells_cpus += ncpus;
+ if (!(tmp = virXMLPropString(nodes[i], "cpus"))) {
^
extraneous space
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Missing 'cpus' attribute in NUMA
cell"));
+ goto cleanup;
+ }
- ctxt->node = nodes[i];
- if (virDomainParseMemory("./@memory", "./@unit", ctxt,
- &def->cells[cur_cell].mem, true, false) <
0)
- goto cleanup;
+ ncpus = virBitmapParse(tmp, 0, &def->cells[cur_cell].cpumask,
+ VIR_DOMAIN_CPUMASK_LEN);
- memAccessStr = virXMLPropString(nodes[i], "memAccess");
- if (memAccessStr) {
- rc = virMemAccessTypeFromString(memAccessStr);
+ if (ncpus <= 0)
+ goto cleanup;
+ def->cells_cpus += ncpus;
- if (rc <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Invalid 'memAccess' attribute "
- "value '%s'"),
- memAccessStr);
- VIR_FREE(memAccessStr);
- goto error;
- }
+ def->cells[cur_cell].cpustr = tmp;
tmp = NULL;
Just in case we jump to cleanup as I suspect cleanup of *.cpustr later
on will be VIR_FREE'd
- def->cells[cur_cell].memAccess = rc;
+ ctxt->node = nodes[i];
+ if (virDomainParseMemory("./@memory", "./@unit", ctxt,
+ &def->cells[cur_cell].mem, true, false) < 0)
+ goto cleanup;
- VIR_FREE(memAccessStr);
+ if ((tmp = virXMLPropString(nodes[i], "memAccess"))) {
+ if ((rc = virMemAccessTypeFromString(tmp)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid 'memAccess' attribute value
'%s'"),
+ tmp);
+ goto cleanup;
}
+
+ def->cells[cur_cell].memAccess = rc;
+ VIR_FREE(tmp);
}
}
ret = 0;
- error:
Yep - sometimes I look at multiple patches before sending and sometimes
I don't... oh well nevermind my 2/24 comment ;-)
ACK,
John
cleanup:
ctxt->node = oldNode;
VIR_FREE(nodes);
+ VIR_FREE(tmp);
return ret;
}
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 7b26e50..64f709a 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -243,7 +243,6 @@ virCapsPtr testQemuCapsInit(void)
ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */
host_cpu_features, /* features */
0, /* ncells */
- 0, /* ncells_max */
NULL, /* cells */
0, /* cells_cpus */
};