Re: [libvirt] [PATCH 05/17] Support hypervisorpin xml parse.

On 08/03/2012 12:36 AM, Hu Tao wrote: > From: Tang Chen <tangchen(a)cn.fujitsu.com&gt; > > This patch adds a new xml element <hypervisorpin cpuset='1'>, I would mention that this is a sibling to the existing <vcpupin> element under the <cputune>. > and also the parser functions, docs, and tests. > hypervisorpin means pinning hypervisor threads, and cpuset = '1' > means pinning all hypervisor threads to cpu 1. > > Signed-off-by: Tang Chen <tangchen(a)cn.fujitsu.com&gt; > Signed-off-by: Hu Tao <hutao(a)cn.fujitsu.com&gt; > --- > docs/schemas/domaincommon.rng | 7 ++ Missing documentation. I can't apply this as-is unless we also update the elementsCPUTuning section of docs/formatdomain.html.in. That won't stop me from reviewing the rest of the patch, though. > src/conf/domain_conf.c | 97 ++++++++++++++++++++++- > src/conf/domain_conf.h | 1 + > tests/qemuxml2argvdata/qemuxml2argv-cputune.xml | 1 + > 4 files changed, 103 insertions(+), 3 deletions(-) > > +++ b/src/conf/domain_conf.c > @@ -7828,6 +7828,51 @@ error: > goto cleanup; > } > > +/* Parse the XML definition for hypervisorpin */ > +static virDomainVcpuPinDefPtr > +virDomainHypervisorPinDefParseXML(const xmlNodePtr node) > +{ ... > +} This is a lot of code duplication. It might be worth refactoring things to write a helper function that parses '@cpuset', and which can be shared by both the existing virDomainVcpuPinDefParseXML and your new function. > @@ -9280,7 +9353,7 @@ no_memory: > virReportOOMError(); > /* fallthrough */ > > - error: > +error: > VIR_FREE(tmp); > VIR_FREE(nodes); > virBitmapFree(bootMap); On its own, this whitespace cleanup has no bearing on the patch; it's generally wise to limit cleanups to portions already affected by the patch. But in general, the patch looked reasonable.