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

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&gt; >> --- >> 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: