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

[Martin Kletzander]

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:

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140715/2fac25eb/attachment-0001.sig>