1:33 a.m.
On Fri, Jul 11, 2014 at 05:11:00PM +0200, Michal Privoznik wrote:
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(a)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.
Thanks :)
[...]
> + 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.
Yep, that happens when you change the behaviour of a function that
used to steal a pointer, in a rebase. Thanks!
> + 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;
I take the 'const' as a sign of the fact that I won't be modifying
any part of the string. Just adding 'const' to a pointer should be
perfectly OK, but I have not objections to your idea, so I squashed
this in:
diff --git i/src/conf/numatune_conf.c w/src/conf/numatune_conf.c
index 8b66558..375428c 100644
--- i/src/conf/numatune_conf.c
+++ w/src/conf/numatune_conf.c
@@ -141,6 +142,7 @@ virDomainNumatuneFormatXML(virBufferPtr buf,
virDomainNumatunePtr numatune)
{
const char *tmp = NULL;
+ char *nodeset = NULL;
if (!numatune)
return 0;
@@ -152,10 +154,10 @@ virDomainNumatuneFormatXML(virBufferPtr buf,
virBufferAsprintf(buf, "<memory mode='%s' ", tmp);
if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC) {
- if (!(tmp = virBitmapFormat(numatune->memory.nodeset)))
+ if (!(nodeset = virBitmapFormat(numatune->memory.nodeset)))
return -1;
virBufferAsprintf(buf, "nodeset='%s'/>\n", tmp);
- VIR_FREE(tmp);
+ VIR_FREE(nodeset);
} else if (numatune->memory.placement) {
tmp = virDomainNumatunePlacementTypeToString(numatune->memory.placement);
virBufferAsprintf(buf, "placement='%s'/>\n", tmp);
--
Martin