On Mon, Aug 06, 2012 at 04:51:49PM -0600, Eric Blake wrote:
> On 08/03/2012 12:36 AM, Hu Tao wrote:
> > From: Tang Chen <tangchen@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Hu Tao <hutao@xxxxxxxxxxxxxx>
> > ---
> > 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.
Thank you, I'll improve this patch in v2.
--
Thanks,
Hu Tao
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list