On Mon, Aug 29, 2016 at 19:21:24 -0400, John Ferlan wrote: ...
@@ -183,6 +183,10 @@ <dd> The <code>mode</code> element contains a list of supported CPU models, each described by a dedicated <code>model</code> element. + The <code>usable</code> attribute specifies whether the model can + be used on the host. A special value <code>unknown</code> says
s/says/indicates/
Fixed.
+ libvirt does not have enough information to provide the usability + data. </dd> </dl>
diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 9f3d225..5a605a7 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -105,6 +105,13 @@ <ref name='supported'/> <zeroOrMore> <element name='model'> + <attribute name='usable'> + <choice> + <value>yes</value> + <value>no</value> + <value>unknown</value> + </choice> + </attribute>
Why is this not optional? Hmm... Seems to be output only - still an odd construct though. Not sure I have a better idea (yet).
Exactly, it's output only and we always format it so there's no reason for the attribute to be optional. ...
@@ -399,7 +412,16 @@ virDomainCapsCPUFormat(virBufferPtr buf, virCPUModeTypeToString(VIR_CPU_MODE_CUSTOM)); if (cpu->custom && cpu->custom->count) { virBufferAddLit(buf, "supported='yes'>\n"); - virDomainCapsCPUCustomFormat(buf, cpu->custom); + virBufferAdjustIndent(buf, 2); + + virDomainCapsCPUCustomFormat(buf, cpu->custom, + VIR_DOMCAPS_CPU_USABLE_YES); + virDomainCapsCPUCustomFormat(buf, cpu->custom, + VIR_DOMCAPS_CPU_USABLE_NO); + virDomainCapsCPUCustomFormat(buf, cpu->custom, + VIR_DOMCAPS_CPU_USABLE_UNKNOWN);
So we're listing all the usable ones first, followed by the unusable, followed by the unknown. Hence the full.xml output change.
Hmm, it really seems pretty stupid to group them by usable value, doesn't it? I just fixed the code so that the CPU models are printed in a single loop no matter whether they're usable or not.
In any case, that seem like something that would be documentable - the sorting algorithm... It wasn't listed in the cover letter either (the usable attribute isn't there).
Hmm, you're right, I missed this in the cover letter.
I guess it just seems inefficient to run through the custom list 3 times just so we can print out in a specific/sorted order. Not sure what printing "unknown" really buys us - seems to be ignorable to me at least at this point in the review process.
For any hypervisor that doesn't give us the usability data, all CPUs will have usable='unknown'. Or were you thinking about why don't we just skip this attribute if it's value is unknown? If so, I think it's better to be explicit. ...
@@ -2960,7 +2964,8 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, }
if (virDomainCapsCPUModelsAddSteal(qemuCaps->cpuDefinitions, - &str) < 0) + &str, + VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)]
So we can go from 'yes' or 'no' to 'unknown' (for at least a short period of time). I guess I would have expected to "read" and use the cached data like other places... Leads me to wonder why it's being saved. Can it be possible to go from 'yes' to 'no' if we go through unknown? Guess it's just not clear what the dynamics of the conversion are and when is (should be) expected.
There's no code in QEMU driver which would set the value to anything but 'unknown' yet. There's no conversion involved. This is just a preparation for a later patch that would really ask QEMU for this stuff. Jirka