8:55 a.m.
On Tue, Feb 21, 2017 at 09:27:17 -0500, John Ferlan wrote:
On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> The element will be generalized in the following commit.
>
> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> ---
>
> Notes:
> Version 2:
> - no change
>
> src/qemu/qemu_capabilities.c | 14 +-
> tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 30 +--
> tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 322 +++++++++++------------
> 3 files changed, 183 insertions(+), 183 deletions(-)
>
This is essentially assuming the capabilities cache is being re-created
and not re-read... Hopefully that works correctly... It was the change
of an element name without some sort of pseudonym added or failure
returned if "feature" was found. I didn't check callers - but if
something here failed would it *ensure* that the capabilities cache was
re-created?
Well, libvirt doesn't use cached capabilities generated by older
libvirt as they are considered invalid whenever libvirtd's ctime
changes. We don't need to worry about compatibility here. And yes, if
parsing the XML fails, the cache is re-created.
> diff --git a/src/qemu/qemu_capabilities.c
b/src/qemu/qemu_capabilities.c
> index 688d19504..aab336954 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3180,7 +3180,7 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
>
> ctxt->node = hostCPUNode;
>
> - if ((n = virXPathNodeSet("./feature", ctxt, &featureNodes)) >
0) {
> + if ((n = virXPathNodeSet("./property", ctxt, &featureNodes)) >
0) {
featureNodes should be renamed to propertyNodes too...
or to just "nodes".
> if (VIR_ALLOC_N(hostCPU->props, n) < 0)
> goto cleanup;
>
> @@ -3191,14 +3191,14 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> if (!hostCPU->props[i].name) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("missing 'name' attribute for a host
CPU"
> - " model feature in QEMU capabilities
cache"));
> + " model property in QEMU capabilities
cache"));
> goto cleanup;
> }
>
> - if (!(str = virXMLPropString(featureNodes[i], "supported")))
{
> + if (!(str = virXMLPropString(featureNodes[i], "boolean"))) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("missing 'supported' attribute for a
host CPU"
> - " model feature in QEMU capabilities
cache"));
> + _("missing 'boolean' attribute for a
host CPU"
> + " model property in QEMU capabilities
cache"));
> goto cleanup;
> }
> if (STREQ(str, "yes")) {
> @@ -3207,7 +3207,7 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> hostCPU->props[i].supported = false;
> } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("invalid supported value:
'%s'"), str);
> + _("invalid boolean value: '%s'"),
str);
boolean's are typically true/false - so if "boolean='no'", then
does
that mean it's something else ;-)
Hmm, the s/supported/boolean/ change should not even be part of this
patch.
I'm not sure boolean is the best name, but I see from the next
patch
it's used because of the "type" being read from monitor/json.
Perhaps rather than "boolean" go with "value"... That way when you
implement more types in the next patch you get:
property name='%s' value='yes|no'
There's no need to support missing type attribute, though.
property name='%s' type='string' value='%s'
property name='%s' type='ull' value=%ull
I was thinking about going this route but I chose the other one. But I
agree this looks cleaner so I'll rework the following patch.
Not a fan of 'ull' either - I think it should just be
'number' and the
implementation details is that the number is a ULL.
The whole capabilities XML is an implementation detail :-) Anyway, "ull"
is an important detail since we need to know how to parse the number.
But since "ull" is the only type we currently support I agree with
renaming the type to number. We can always change it if we need more
types of numbers.
Jirka