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