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

John Ferlan jferlan at redhat.com
Thu Feb 19 21:34:46 UTC 2015



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




More information about the libvir-list mailing list