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