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

[Michal Privoznik]

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