[libvirt] [PATCH v2 10/33] qemu: Rename hostCPU/feature element in capabilities cache

[Jiri Denemark]

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