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

John Ferlan jferlan at redhat.com
Mon Feb 12 20:18:04 UTC 2018



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 at 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 at redhat.com>


> +                               virDomainCapabilitiesPolicyTypeToString(src->features[i]),
> +                               virDomainCapabilitiesPolicyTypeToString(dst->features[i]));
> +                return false;
> +            }
> +            break;
> +
>          case VIR_DOMAIN_FEATURE_LAST:
>              break;
>          }
> 




More information about the libvir-list mailing list