On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Unlike most other features, VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy instead of virTristateSwitch, so we need to handle it separately for the error message to make sense.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e4d01b869..9f019c906 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21336,7 +21336,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_HYPERV: case VIR_DOMAIN_FEATURE_KVM: case VIR_DOMAIN_FEATURE_PVSPINLOCK: - case VIR_DOMAIN_FEATURE_CAPABILITIES: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_VMPORT: case VIR_DOMAIN_FEATURE_GIC: @@ -21355,6 +21354,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } break;
+ 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). 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... Reviewed-by: John Ferlan <jferlan@redhat.com>
+ virDomainCapabilitiesPolicyTypeToString(src->features[i]), + virDomainCapabilitiesPolicyTypeToString(dst->features[i])); + return false; + } + break; + case VIR_DOMAIN_FEATURE_LAST: break; }