[libvirt] [PATCH 20/41] domcaps: Add CPU usable flag

[John Ferlan]

[...]

>>> @@ -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
>