[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]