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

On 01/06/2012 08:04 AM, Jiri Denemark wrote: <snip> nice examples 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. 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. 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> Thanks for the style cleanups while in the area. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org