[...]
@@ -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.
...
I suppose I was more grousing about the 3 times through, but then starting thinking is it worth it to print unknown, but since this is output only so I can agree it's fine to be explicit. Sorry if taking the route of ACK'ing every 10 or so patches is/was hard to work with - it's a large series and was not possible for me to review in one sitting, so I tried to break it up into more manageable chunks. In particular, this one I reviewed after 7PM (considering I typically am up/online by 6AM - that's a long day). It was mostly a workflow thing for me and I've seen other reviews take a similar approach with larger series. I should have also replied to the cover letter, but forgot. In any case, I think you've answered my issues so (more explicit) ACK for this one with the noted changes. John
@@ -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