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

7 Jan 2012


      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@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

    