Re: [libvirt] [PATCH 03/24] conf: Refactor virDomainNumaDefCPUParseXML

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

be avoided by switching to VIR_ALLOC_N as the array is not resized after initial allocation.

--- 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) {

+ 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"))) {

+ 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;

- 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:

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 */ };