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 */
> };
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list