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

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.