[libvirt] [PATCH v2 04/16] conf, schema: add 'id' field for cells

[Michal Privoznik]

On 08.07.2014 13:50, Martin Kletzander wrote:
> In XML format, by definition, order of fields should not matter, so
> order of parsing the elements doesn't affect the end result.  When
> specifying guest NUMA cells, we depend only on the order of the 'cell'
> elements.  With this patch all older domain XMLs are parsed as before,
> but with the 'id' attribute they are parsed and formatted according to
> that field.  This will be useful when we have tuning settings for
> particular guest NUMA node.
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>   docs/formatdomain.html.in                          | 11 +++---
>   docs/schemas/domaincommon.rng                      |  5 +++
>   src/conf/cpu_conf.c                                | 39 +++++++++++++++++++---
>   src/conf/cpu_conf.h                                |  3 +-
>   src/qemu/qemu_command.c                            |  2 +-
>   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml  |  6 ++--
>   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.xml  |  6 ++--
>   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa3.xml  | 25 ++++++++++++++
>   tests/qemuxml2argvtest.c                           |  1 +
>   .../qemuxml2xmlout-cpu-numa1.xml                   | 28 ++++++++++++++++
>   .../qemuxml2xmlout-cpu-numa2.xml                   | 28 ++++++++++++++++
>   tests/qemuxml2xmltest.c                            |  3 ++
>   12 files changed, 139 insertions(+), 18 deletions(-)
>   create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa3.xml
>   create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-numa1.xml
>   create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-numa2.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index b69da4c..ad87b7c 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1030,8 +1030,8 @@
>     <cpu>
>       ...
>       <numa>
> -      <cell cpus='0-3' memory='512000'/>
> -      <cell cpus='4-7' memory='512000'/>
> +      <cell id='0' cpus='0-3' memory='512000'/>
> +      <cell id='1' cpus='4-7' memory='512000'/>
>       </numa>
>       ...
>     </cpu>
> @@ -1041,8 +1041,11 @@
>         Each <code>cell</code> element specifies a NUMA cell or a NUMA node.
>         <code>cpus</code> specifies the CPU or range of CPUs that are part of
>         the node. <code>memory</code> specifies the node memory in kibibytes
> -      (i.e. blocks of 1024 bytes). Each cell or node is assigned cellid
> -      or nodeid in the increasing order starting from 0.
> +      (i.e. blocks of 1024 bytes). All cells should have <code>id</code>
> +      attribute in case referring to some cell is necessary in the code,
> +      otherwise the cells are assigned ids in the increasing order starting
> +      from 0.  Mixing cells with and without the <code>id</code> attribute
> +      is not recommended as it may result in unwanted behaviour.

I'd note here, that the @id attribute is since 1.2.7

>       </p>
> 
>       <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 7be028d..155a33e 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3902,6 +3902,11 @@
> 
>     <define name="numaCell">
>       <element name="cell">
> +      <optional>
> +        <attribute name="id">
> +          <ref name="unsignedInt"/>
> +        </attribute>
> +      </optional>
>         <attribute name="cpus">
>           <ref name="cpuset"/>
>         </attribute>
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index 811893d..9e0af08 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -152,7 +152,6 @@ virCPUDefCopy(const virCPUDef *cpu)
>           copy->ncells_max = copy->ncells = cpu->ncells;
> 
>           for (i = 0; i < cpu->ncells; i++) {
> -            copy->cells[i].cellid = cpu->cells[i].cellid;
>               copy->cells[i].mem = cpu->cells[i].mem;
> 
>               copy->cells[i].cpumask = virBitmapNewCopy(cpu->cells[i].cpumask);
> @@ -438,17 +437,46 @@ virCPUDefParseXML(xmlNodePtr node,
>           for (i = 0; i < n; i++) {
>               char *cpus, *memory;
>               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);
> +                VIR_FREE(tmp);
> +                if (ret == -1) {
> +                    virReportError(VIR_ERR_XML_ERROR, "%s",
> +                                   _("Invalid 'id' attribute in NUMA cell"));
> +                    goto error;
> +                }
> +            }

If there's a typo in the @id, I think this can make users lives easier:

diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 9e0af08..5003cf1 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -445,12 +445,14 @@ virCPUDefParseXML(xmlNodePtr node,
                 cur_cell = i;
             } else {
                 ret  = virStrToLong_ui(tmp, NULL, 10, &cur_cell);
-                VIR_FREE(tmp);
                 if (ret == -1) {
-                    virReportError(VIR_ERR_XML_ERROR, "%s",
-                                   _("Invalid 'id' attribute in NUMA cell"));
+                    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) {


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


Michal