[libvirt] [PATCH 05/11] conf: Validate VIR_DOMAIN_FEATURE_CAPABILITIES properly

[Andrea Bolognani]

On Mon, 2018-02-12 at 15:18 -0500, John Ferlan wrote:
> > +        case VIR_DOMAIN_FEATURE_CAPABILITIES:
> > +            if (src->features[i] != dst->features[i]) {
> > +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                               _("State of feature '%s:%s' differs: "
> > +                                 "source: '%s', destination: '%s'"),
> > +                               featureName, "policy",
> 
> OK - based on what I saw Peter ACK for Michal's patches related to his
> similar usage, fine. Still looks strange especially since we're talking
> about a named XML attribute which we document as "policy". I just hope
> there isn't some language variant that alters the meaning.  It does look
> strange to me "State of feature 'capabilities:policy' differs: "... I
> almost wonder if "State of feature 'capabilities' attribute 'policy'
> differs: " would help (or really matter).

Hm. Maybe it could look like

  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                 _("State of feature '%s' differs: "
                   "source: '%s,%s=%s', destination: '%s,%s=%s'"),
                 featureName,
                 virTristateSwitchTypeToString(src->features[i]),
                 "version", virGICVersionTypeToString(src->gic_version),
                 virTristateSwitchTypeToString(dst->features[i]),
                 "version", virGICVersionTypeToString(dst->gic_version));

so that the state of the feature itself and that of its various
attributes, when present, would be displayed without ambiguity.

> Also, based on the tests/domainschemadata/domain-caps-features.xml:
> 
>         <capabilities policy="deny">
>             <mknod state="on"/>
>         </capabilities>
> 
> /me wonders how many more yaks might get shaved if we needed to check
> differences w/r/t the caps_features array?
> 
> IOW: If src->features[i] != VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT, then
> should we be checking each of the VIR_DOMAIN_CAPS_FEATURE_LAST to ensure
> that they're not different too.
> 
> Not something required for this patch, but perhaps a follow-up...

For some reason, I thought caps_features were handled later on along
with kvm_features and hyperv_features, but I see now that's not the
case. I agree that's something that should be addressed.

-- 
Andrea Bolognani / Red Hat / Virtualization