[libvirt PATCH 09/10] virDomainFeaturesDefParse: Use virXMLProp*

[Peter Krempa]

On Fri, Apr 23, 2021 at 17:39:22 +0200, Tim Wiederhake wrote:

Function-level granularity for a function this massive seems to be too
coarse.

> Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> ---
>  src/conf/domain_conf.c                        | 333 +++++++-----------
>  .../qemuxml2argvdata/aarch64-gic-invalid.err  |   2 +-
>  2 files changed, 119 insertions(+), 216 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 867a83a605..bff17057af 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17858,6 +17858,7 @@ virDomainFeaturesDefParse(virDomainDef *def,
>      g_autofree xmlNodePtr *nodes = NULL;
>      size_t i;
>      int n;
> +    int rc;
>  
>      if ((n = virXPathNodeSet("./features/*", ctxt, &nodes)) < 0)
>          return -1;
> @@ -17895,76 +17896,67 @@ virDomainFeaturesDefParse(virDomainDef *def,
>              def->features[val] = VIR_TRISTATE_SWITCH_ON;
>              break;
>  
> -        case VIR_DOMAIN_FEATURE_CAPABILITIES:
> -            if ((tmp = virXMLPropString(nodes[i], "policy"))) {
> -                if ((def->features[val] = virDomainCapabilitiesPolicyTypeFromString(tmp)) == -1) {
> -                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                                   _("unknown policy attribute '%s' of feature '%s'"),
> -                                   tmp, virDomainFeatureTypeToString(val));
> -                    return -1;
> -                }
> -            } else {
> -                def->features[val] = VIR_TRISTATE_SWITCH_ABSENT;
> -            }
> +        case VIR_DOMAIN_FEATURE_CAPABILITIES: {
> +            virDomainCapabilitiesPolicy policy;
> +
> +            def->features[val] = VIR_TRISTATE_SWITCH_ABSENT;

While faithfully reimplementing the code, this doesn't make sense. You
should initialize 'policy' properly to the default value ...

> +
> +            if ((rc = virXMLPropEnum(nodes[i], "policy",
> +                                     virDomainCapabilitiesPolicyTypeFromString,
> +                                     VIR_XML_PROP_NONE, &policy)) < 0)
> +                return -1;

... and then assign it here to def->features[val] without any check.

It makes no sense to check 'rc' at all, since virXMLPropEnum doesn't
touch the value stored in the pointer passed in the last argument if the
property is missing.

> +
> +            if (rc == 1)
> +                def->features[val] = policy;
>              break;
> +        }
>  
>          case VIR_DOMAIN_FEATURE_VMCOREINFO:
>          case VIR_DOMAIN_FEATURE_HAP:
>          case VIR_DOMAIN_FEATURE_PMU:
>          case VIR_DOMAIN_FEATURE_PVSPINLOCK:
>          case VIR_DOMAIN_FEATURE_VMPORT:
> -        case VIR_DOMAIN_FEATURE_SMM:
> -            if ((tmp = virXMLPropString(nodes[i], "state"))) {
> -                if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) == -1) {
> -                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                                   _("unknown state attribute '%s' of feature '%s'"),
> -                                   tmp, virDomainFeatureTypeToString(val));
> -                    return -1;
> -                }
> -            } else {
> -                def->features[val] = VIR_TRISTATE_SWITCH_ON;
> -            }
> +        case VIR_DOMAIN_FEATURE_SMM: {
> +            virTristateSwitch state;
> +
> +            def->features[val] = VIR_TRISTATE_SWITCH_ON;
> +
> +            if ((rc = virXMLPropTristateSwitch(nodes[i], "state",
> +                                               VIR_XML_PROP_NONE, &state)) < 0)
> +                return -1;
> +
> +            if (rc == 1)
> +                def->features[val] = state;
>              break;


Same as above here and in all other refactored VIR_DOMAIN_FEATURE_*
below ... (snipped).


[...]


> @@ -18054,13 +18039,12 @@ virDomainFeaturesDefParse(virDomainDef *def,
>  
>      if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) {
>          int feature;
> -        int value;
>          xmlNodePtr node = ctxt->node;
>          if ((n = virXPathNodeSet("./features/hyperv/*", ctxt, &nodes)) < 0)
>              return -1;
>  
>          for (i = 0; i < n; i++) {
> -            g_autofree char *tmp = NULL;
> +            virTristateSwitch value;
>              feature = virDomainHypervTypeFromString((const char *)nodes[i]->name);
>              if (feature < 0) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -18071,21 +18055,9 @@ virDomainFeaturesDefParse(virDomainDef *def,
>  
>              ctxt->node = nodes[i];

[1] (see below)

>  
> -            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
> -                virReportError(VIR_ERR_XML_ERROR,
> -                               _("missing 'state' attribute for "
> -                                 "HyperV Enlightenment feature '%s'"),
> -                               nodes[i]->name);
> -                return -1;
> -            }
> -
> -            if ((value = virTristateSwitchTypeFromString(tmp)) < 0) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                               _("invalid value of state argument "
> -                                 "for HyperV Enlightenment feature '%s'"),
> -                               nodes[i]->name);
> +            if (virXMLPropTristateSwitch(nodes[i], "state",
> +                                         VIR_XML_PROP_REQUIRED, &value) < 0)
>                  return -1;
> -            }
>  
>              def->hyperv_features[feature] = value;
>  
> @@ -18108,12 +18080,10 @@ virDomainFeaturesDefParse(virDomainDef *def,
>                  if (value != VIR_TRISTATE_SWITCH_ON)
>                      break;
>  
> -                if (virXPathUInt("string(./@retries)", ctxt,
> -                             &def->hyperv_spinlocks) < 0) {

This removes the only piece of code which uses the XPath context (ctxt)
for parsing in this whole loop, so [1] and the resetting of ctxt at the
end of the loop should be removed too.

> -                    virReportError(VIR_ERR_XML_ERROR, "%s",
> -                                   _("invalid HyperV spinlock retry count"));
> +                if (virXMLPropUInt(nodes[i], "retries", 0,
> +                                   VIR_XML_PROP_REQUIRED,
> +                                   &def->hyperv_spinlocks) < 0)
>                      return -1;
> -                }
>  
>                  if (def->hyperv_spinlocks < 0xFFF) {
>                      virReportError(VIR_ERR_XML_ERROR, "%s",

[...]