[libvirt] [PATCH v2 1/3] conf: Format numatune XML correctly while placement is none

[Osier Yang]

On 2012年07月10日 17:41, Daniel P. Berrange wrote:
> On Tue, Jul 10, 2012 at 05:31:17PM +0800, Osier Yang wrote:
>> ping?
>>
>> On 2012年06月25日 12:28, Osier Yang wrote:
>>> setNumaParameters tunes the numa setting using cgroup, it's another
>>> entry except libnuma/numad for numa tuning. And it doesn't set the
>>> placement, and further more, the formating codes doesn't take this
>>> into consideration.
>>>
>>> How to reproduce:
>>>
>>> conn = libvirt.open(None)
>>> dom = conn.lookupByName('linux')
>>> param = {'numa_nodeset': '0', 'numa_mode': 1}
>>> dom.setNumaParameters(param, 2)
>>>
>>> % virsh start linux
>>> error: Failed to start domain rhel6.3rc
>>> error: (domain_definition):8: error parsing attribute name
>>>      <memory mode='preferred'</numatune>
>>> -------------------------------^
>>> ---
>>>   src/conf/domain_conf.c |   17 ++++++++++-------
>>>   1 files changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 81c6308..c44d89d 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -12795,23 +12795,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>>           const char *placement;
>>>
>>>           mode = virDomainNumatuneMemModeTypeToString(def->numatune.memory.mode);
>>> -        virBufferAsprintf(buf, "<memory mode='%s' ", mode);
>>> +        virBufferAsprintf(buf, "<memory mode='%s'", mode);
>>>
>>> -        if (def->numatune.memory.placement_mode ==
>>> -            VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
>>> +        if (def->numatune.memory.nodemask) {
>>>               nodemask = virDomainCpuSetFormat(def->numatune.memory.nodemask,
>>> -                                         VIR_DOMAIN_CPUMASK_LEN);
>>> +                                             VIR_DOMAIN_CPUMASK_LEN);
>>>               if (nodemask == NULL) {
>>>                   virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>                                        _("failed to format nodeset for "
>>>                                          "NUMA memory tuning"));
>>>                   goto cleanup;
>>>               }
>>> -            virBufferAsprintf(buf, "nodeset='%s'/>\n", nodemask);
>>> +            virBufferAsprintf(buf, " nodeset='%s'/>\n", nodemask);
>>>               VIR_FREE(nodemask);
>>> -        } else if (def->numatune.memory.placement_mode) {
>>> +        } else if (def->numatune.memory.placement_mode ==
>>> +            VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO) {
>>>               placement = virDomainNumatuneMemPlacementModeTypeToString(def->numatune.memory.placement_mode);
>>> -            virBufferAsprintf(buf, "placement='%s'/>\n", placement);
>>> +            virBufferAsprintf(buf, " placement='%s'/>\n", placement);
>>> +        } else {
>>> +            /* Should not hit here. */
>>> +            virBufferAddLit(buf, "/>\n");
>>>           }
>>>           virBufferAddLit(buf, "</numatune>\n");
>>>       }
>
> The fact that this bug existed shows that the test suite for the XML
> parser is incomplete. Please resubmit with an extra test XML datafile
> for the test suite to validate this scenario.
>

we already has good enough XMLs for the test suite:

% ls tests/qemuxml2argvdata/qemuxml2argv-numa
qemuxml2argv-numad.args 
qemuxml2argv-numad-auto-vcpu-static-numatune.xml
qemuxml2argv-numad-auto-memory-vcpu-cpuset.args 
qemuxml2argv-numad-static-memory-auto-vcpu.args
qemuxml2argv-numad-auto-memory-vcpu-cpuset.xml 
qemuxml2argv-numad-static-memory-auto-vcpu.xml
qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.args 
qemuxml2argv-numad-static-vcpu-no-numatune.xml
qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.xml 
qemuxml2argv-numad.xml
qemuxml2argv-numad-auto-vcpu-no-numatune.xml 
qemuxml2argv-numatune-memory.args
qemuxml2argv-numad-auto-vcpu-static-numatune.args 
qemuxml2argv-numatune-memory.xml

The problem is we have two entries to change the numatune config,
and they share the same XML syntax (btw, I was thinking it's a
bad idea to do so, it could just cause crasy results). XML parser
actually ensures the placement mode can be always set with either
'static' or 'auto', but API via cgroup don't set placement mode
as placement is meaningless for it, so IMHO no need to add
XMLs to test the parser, instead we need to add tests to test
the API.

Regards,
Osier