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