[libvirt] [PATCH v2 07/16] numatune: Encapsulate numatune configuration in order to unify results

[Michal Privoznik]

On 08.07.2014 13:50, Martin Kletzander wrote:
> There were numerous places where numatune configuration (and thus
> domain config as well) was changed in different ways.  On some
> places this even resulted in persistent domain definition not to be
> stable (it would change with daemon's restart).
>
> In order to uniformly change how numatune config is dealt with, all
> the internals are now accessible directly only in numatune_conf.c and
> outside this file accessors must be used.
>
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>   po/POTFILES.in                                     |   1 +
>   src/conf/domain_conf.c                             | 159 ++---------
>   src/conf/domain_conf.h                             |   8 +-
>   src/conf/numatune_conf.c                           | 316 +++++++++++++++++++++
>   src/conf/numatune_conf.h                           |  72 ++++-
>   src/libvirt_private.syms                           |  11 +
>   src/lxc/lxc_cgroup.c                               |  19 +-
>   src/lxc/lxc_controller.c                           |   5 +-
>   src/lxc/lxc_native.c                               |  15 +-
>   src/parallels/parallels_driver.c                   |   7 +-
>   src/qemu/qemu_cgroup.c                             |  23 +-
>   src/qemu/qemu_driver.c                             |  84 +++---
>   src/qemu/qemu_process.c                            |   8 +-
>   src/util/virnuma.c                                 |  48 ++--
>   src/util/virnuma.h                                 |   2 +-
>   .../qemuxml2argv-numatune-auto-prefer.xml          |  29 ++
>   .../qemuxml2xmlout-numatune-auto-prefer.xml        |  29 ++
>   tests/qemuxml2xmltest.c                            |   2 +
>   18 files changed, 553 insertions(+), 285 deletions(-)
>   create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-auto-prefer.xml
>   create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-numatune-auto-prefer.xml

Nice.

> diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
> index 14300f0..8b66558 100644
> --- a/src/conf/numatune_conf.c
> +++ b/src/conf/numatune_conf.c

> +int
> +virDomainNumatuneParseXML(virDomainDefPtr def,
> +                          xmlXPathContextPtr ctxt)
> +{
> +    char *tmp = NULL;
> +    int mode = -1;
> +    int n = 0;
> +    int placement = -1;
> +    int ret = -1;
> +    virBitmapPtr nodeset = NULL;
> +    xmlNodePtr node = NULL;
> +
> +    if (virXPathInt("count(./numatune)", ctxt, &n) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("cannot extract numatune nodes"));
> +        goto cleanup;
> +    } else if (n > 1) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("only one numatune is supported"));
> +        goto cleanup;
> +    }
> +
> +    node = virXPathNode("./numatune/memory[1]", ctxt);
> +
> +    if (def->numatune) {
> +        virDomainNumatuneFree(def->numatune);
> +        def->numatune = NULL;
> +    }
> +
> +    if (!node && def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
> +        return 0;
> +
> +    if (!node) {
> +        /* We know that def->placement_mode is "auto" if we're here */
> +        if (virDomainNumatuneSet(def, VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO,
> +                                 -1, NULL) < 0)
> +            goto cleanup;
> +        return 0;
> +    }
> +
> +    tmp = virXMLPropString(node, "mode");
> +    if (tmp) {
> +        mode = virDomainNumatuneMemModeTypeFromString(tmp);
> +        if (mode < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unsupported NUMA memory tuning mode '%s'"),
> +                           tmp);
> +            goto cleanup;
> +        }
> +    }
> +    VIR_FREE(tmp);
> +
> +    tmp = virXMLPropString(node, "placement");
> +    if (tmp) {
> +        placement = virDomainNumatunePlacementTypeFromString(tmp);
> +        if (placement < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unsupported NUMA memory placement mode '%s'"),
> +                           tmp);
> +            goto cleanup;
> +        }
> +    }
> +    VIR_FREE(tmp);
> +
> +    tmp = virXMLPropString(node, "nodeset");
> +    if (tmp && virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
> +        goto cleanup;
> +    VIR_FREE(tmp);
> +
> +    if (virDomainNumatuneSet(def, placement, mode, nodeset) < 0)

The virDomainNumatuneSet() takes a copy of @nodeset, so you need to call 
virBitmaskFree(nodeset); at the cleanup label.

> +        goto cleanup;
> +
> +    if (!n) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(tmp);
> +    return ret;
> +}
> +
> +int
> +virDomainNumatuneFormatXML(virBufferPtr buf,
> +                           virDomainNumatunePtr numatune)
> +{
> +    const char *tmp = NULL;

s /const// ..

> +
> +    if (!numatune)
> +        return 0;
> +
> +    virBufferAddLit(buf, "<numatune>\n");
> +    virBufferAdjustIndent(buf, 2);
> +
> +    tmp = virDomainNumatuneMemModeTypeToString(numatune->memory.mode);
> +    virBufferAsprintf(buf, "<memory mode='%s' ", tmp);
> +
> +    if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC) {
> +        if (!(tmp = virBitmapFormat(numatune->memory.nodeset)))
> +            return -1;
> +        virBufferAsprintf(buf, "nodeset='%s'/>\n", tmp);
> +        VIR_FREE(tmp);

.. because free()-ing a const char * is not nice. If you, however, do 
this I bet you'll get error in TypeToString(). So just leave tmp as 
const char * and introduce char *nodeset;

> +    } else if (numatune->memory.placement) {
> +        tmp = virDomainNumatunePlacementTypeToString(numatune->memory.placement);
> +        virBufferAsprintf(buf, "placement='%s'/>\n", tmp);
> +    }
> +
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</numatune>\n");
> +    return 0;
> +}
> +

Michal