On 01/06/2012 08:04 AM, Jiri Denemark wrote:
> The mode can be either of "custom" (default), "host-model",
> "host-passthrough". The semantics of each mode is described in the
> following examples:
<snip> nice examples
> ---
> Oh man, I just realized I forgot to update documentation. I'll transform this
> commit message into a proper documentation in the next version of this series.
> But it shouldn't be a showstopper for normal review :-)
And you were on a roll, too, since 2/6 touched docs. At any rate, I
agree with your sentiment that I can review the code, even though we
should wait for the patch to docs/formatdomain.html.in before pushing
anything. I also agree with your assessment that the commit message is
a fine start for documenting the new feature.
> tests/cputestdata/x86-baseline-1-result.xml | 2 +-
> tests/cputestdata/x86-baseline-2-result.xml | 2 +-
Another round of lots of mechanical fallout by outputting the default
attribute value, even when it was omittedon input. And whereas the
attribute in 2/6 was just a boolean, here it is a tri-state, so I agree
that outputting the mode always makes the most sense.
> +++ b/docs/schemas/domaincommon.rng
> @@ -2425,6 +2425,20 @@
> </interleave>
> </group>
> <group>
> + <ref name="cpuMode"/>
> + <interleave>
> + <optional>
> + <ref name="cpuModel"/>
> + </optional>
> + <optional>
> + <ref name="cpuNuma"/>
> + </optional>
> + </interleave>
> + </group>
> + <group>
> + <optional>
> + <ref name="cpuMode"/>
> + </optional>
> <ref name="cpuMatch"/>
> <interleave>
> <ref name="cpuModel"/>
> @@ -2446,6 +2460,16 @@
> </element>
> </define>
>
> + <define name="cpuMode">
> + <attribute name="mode">
> + <choice>
> + <value>custom</value>
> + <value>host-model</value>
> + <value>host-passthrough</value>
> + </choice>
> + </attribute>
> + </define>
Your RNG does not match your commit message, in two places.
1. You called out this arrangement as valid:
<cpu mode='custom'>
<topology sockets='1' cores='2' threads='1'/>
</cpu>
but your added <group> lacks <ref name="cpuTopology">, and the existing
group that includes cpuTopology lacks the new cpuMode. Here, I think
the solution is to add an optional <ref name="cpuTopology"/> in your
added second group.
2. You called out this arrangement as valid:
<cpu mode='host-model'>
<model fallback='forbid'/>
</cpu>
but cpuModel has mandatory <text/> contents. Here, I think the solution
might be to change cpuModel to use this definition (if fallback=forbid,
then the text is optional; otherwise, text is mandatory but
fallback=allow is default and therefore optional):
<define name="cpuModel">
<element name="model">
<choice>
<group>
<attribute name="fallback">
<value>forbid</value>
</attribute>
<choice>
<text/>
<empty/>
</choice>
</group>
<group>
<optional>
<attribute name="fallback">
<value>allow</value>
</attribute>
</optional>
<text/>
</group>
</choice>
</element>
</define>
> @@ -173,10 +180,35 @@ virCPUDefParseXML(const xmlNodePtr node,
> goto error;
> }
> def->type = VIR_CPU_TYPE_HOST;
> - } else
> + } else {
> def->type = VIR_CPU_TYPE_GUEST;
> - } else
> + }
> + } else {
> def->type = mode;
> + }
Thanks for the style cleanups while in the area.
> @@ -504,29 +566,32 @@ virCPUDefFormatBuf(virBufferPtr buf,
> virBufferAddLit(buf, "/>\n");
> }
>
> - for (i = 0 ; i < def->nfeatures ; i++) {
> - virCPUFeatureDefPtr feature = def->features + i;
> + if (formatModel) {
> + for (i = 0 ; i < def->nfeatures ; i++) {
I almost would have written this as:
for (i = 0; formatModel && i < def->nfeatures; i++) {
to avoid reindenting the for loop. But that's cosmetics; the
transformation looked correct as you did it.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list