[libvirt] [PATCH v2 07/16] numatune: Encapsulate numatune configuration in order to unify results
Michal Privoznik
mprivozn at redhat.com
Tue Jul 15 08:44:05 UTC 2014
On 15.07.2014 08:33, Martin Kletzander wrote:
> 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 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.
>>
>
> 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:
Well, I look at free()-ing as modification of the pointee. Therefore
freeing a const pointer is in fact its modification and hence should be
rejected. It's just that our VIR_FREE throws away the const-ness of
passed pointers. Maybe (as completely separate patchset) we may fix the
VIR_FREE() macro which is obviously const-incorrect.
Michal
More information about the libvir-list
mailing list