On 02/16/2015 01:51 PM, Peter Krempa wrote:
> For weird historical reasons NUMA cells are added as a subelement of
> <cpu> while the actual configuration is done in <numatune>.
>
> This patch splits out the cell parser code from cpu config to NUMA
> config. Note that the changes to the code are minimal just to make it
> work and the function will be refactored in the next patch.
> ---
> src/conf/cpu_conf.c | 90 ---------------------------------------
> src/conf/domain_conf.c | 17 +++++---
> src/conf/numa_conf.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/numa_conf.h | 4 ++
> 4 files changed, 126 insertions(+), 96 deletions(-)
>
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index 4a367a1..f14b37a 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -426,96 +426,6 @@ virCPUDefParseXML(xmlNodePtr node,
> def->features[i].policy = policy;
> }
>
> - if (virXPathNode("./numa[1]", ctxt)) {
> - VIR_FREE(nodes);
> - n = virXPathNodeSet("./numa[1]/cell", ctxt, &nodes);
> - if (n <= 0) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("NUMA topology defined without NUMA cells"));
> - goto error;
> - }
> -
> - 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 ret, ncpus = 0;
> - unsigned int cur_cell;
> - char *tmp = NULL;
> -
> - tmp = virXMLPropString(nodes[i], "id");
> - if (!tmp) {
> - cur_cell = i;
> - } else {
> - ret = virStrToLong_ui(tmp, NULL, 10, &cur_cell);
> - if (ret == -1) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Invalid 'id' attribute in NUMA cell: %s"),
> - tmp);
> - VIR_FREE(tmp);
> - goto error;
> - }
> - VIR_FREE(tmp);
> - }
> -
> - if (cur_cell >= n) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("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;
> - }
> -
> - 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;
> -
> - ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask,
> - VIR_DOMAIN_CPUMASK_LEN);
> - if (ncpus <= 0)
> - goto error;
> - def->cells_cpus += ncpus;
> -
> - ctxt->node = nodes[i];
> - if (virDomainParseMemory("./@memory", "./@unit", ctxt,
> - &def->cells[cur_cell].mem, true, false) < 0)
> - goto cleanup;
> -
> - memAccessStr = virXMLPropString(nodes[i], "memAccess");
> - if (memAccessStr) {
> - int rc = virMemAccessTypeFromString(memAccessStr);
> -
> - if (rc <= 0) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Invalid 'memAccess' attribute "
> - "value '%s'"),
> - memAccessStr);
> - VIR_FREE(memAccessStr);
> - goto error;
> - }
> -
> - def->cells[cur_cell].memAccess = rc;
> -
> - VIR_FREE(memAccessStr);
> - }
> - }
> - }
> -
> cleanup:
> ctxt->node = oldnode;
> VIR_FREE(fallback);
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b13cae8..06ed0fd 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13495,12 +13495,17 @@ virDomainDefParseXML(xmlDocPtr xml,
> goto error;
> }
>
> - if (def->cpu->cells_cpus > def->maxvcpus) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Number of CPUs in <numa> exceeds the"
> - " <vcpu> count"));
> - goto error;
> - }
> + }
> +
> + if (virDomainNumaDefCPUParseXML(def->cpu, ctxt) < 0)
> + goto error;
> +
> + if (def->cpu &&
> + def->cpu->cells_cpus > def->maxvcpus) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Number of CPUs in <numa> exceeds the"
> + " <vcpu> count"));
> + goto error;
> }
>
> if (virDomainNumatuneParseXML(&def->numatune,
> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> index f6a8248..e36a4be 100644
> --- a/src/conf/numa_conf.c
> +++ b/src/conf/numa_conf.c
> @@ -680,3 +680,114 @@ virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune,
>
> return true;
> }
> +
> +
> +int
> +virDomainNumaDefCPUParseXML(virCPUDefPtr def,
> + xmlXPathContextPtr ctxt)
> +{
> + xmlNodePtr *nodes = NULL;
> + xmlNodePtr oldNode = ctxt->node;
> + int n;
> + size_t i;
> + int ret = -1;
> +
> + if (virXPathNode("/domain/cpu/numa[1]", ctxt)) {
> + VIR_FREE(nodes);
> +
> + 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 (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);
> + }
> +
> + if (cur_cell >= n) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("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;
> + }
> +
> + 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;
> +
> + ncpus = virBitmapParse(cpus, 0, &def->cells[cur_cell].cpumask,
> + VIR_DOMAIN_CPUMASK_LEN);
> + if (ncpus <= 0)
> + goto error;
> + def->cells_cpus += ncpus;
> +
> + ctxt->node = nodes[i];
> + if (virDomainParseMemory("./@memory", "./@unit", ctxt,
> + &def->cells[cur_cell].mem, true, false) < 0)
> + goto cleanup;
> +
> + memAccessStr = virXMLPropString(nodes[i], "memAccess");
> + if (memAccessStr) {
> + rc = virMemAccessTypeFromString(memAccessStr);
> +
> + if (rc <= 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Invalid 'memAccess' attribute "
> + "value '%s'"),
> + memAccessStr);
> + VIR_FREE(memAccessStr);
> + goto error;
> + }
> +
> + def->cells[cur_cell].memAccess = rc;
> +
> + VIR_FREE(memAccessStr);
> + }
> + }
> + }
> +
> + ret = 0;
> +
> + error:
> + cleanup:
Although this is mostly a cut'n'paste, why not just change the one "goto
cleanup;" to goto error and then not have two labels?
ACK - since I'm sure you'll do the right thing...
John
> + ctxt->node = oldNode;
> + VIR_FREE(nodes);
> + return ret;
> +}
> diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
> index bcc8c8a..276d25a 100644
> --- a/src/conf/numa_conf.h
> +++ b/src/conf/numa_conf.h
> @@ -29,6 +29,7 @@
> # include "virutil.h"
> # include "virbitmap.h"
> # include "virbuffer.h"
> +# include "cpu_conf.h"
>
>
> typedef struct _virDomainNumatune virDomainNumatune;
> @@ -111,4 +112,7 @@ int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune);
>
> bool virDomainNumatuneNodesetIsAvailable(virDomainNumatunePtr numatune,
> virBitmapPtr auto_nodeset);
> +
> +int virDomainNumaDefCPUParseXML(virCPUDefPtr def, xmlXPathContextPtr ctxt);
> +
> #endif /* __NUMA_CONF_H__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list