Re: [libvirt] [PATCH 20/41] domcaps: Add CPU usable flag
[...]
>> @@ -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