[...]
>>> @@ -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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list