Re: [libvirt] [PATCH 3/6] Add support for cpu mode attribute
On Sat, Jan 07, 2012 at 07:06:31 -0700, Eric Blake wrote:
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:
...
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.
Oh, my bad, there should be no mode attribute in this example since it would
be ignored anyway :-)
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>
It's a bit more complicated since <model fallback='allow'/> is perfectly
valid
within <cpu mode='host-model'/> since it's just explicitly stating the
default
value. However, expressing the reality in RNG schema is would make it very
complicated and unreadable and also given that we don't do so in other areas
of the schema, I just marked the text in model element as optional.
> @@ -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.
Heh, I don't really want to sacrifice readability to a smaller patch :-)
Jirka