[libvirt] [PATCH 3/6] Add support for cpu mode attribute

[Jiri Denemark]

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