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

Tim Wiederhake posted 10 patches 1 month, 3 weeks ago

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

Posted by Tim Wiederhake 1 month, 3 weeks ago
Signed-off-by: Tim Wiederhake <twiederh@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;
+
+            if ((rc = virXMLPropEnum(nodes[i], "policy",
+                                     virDomainCapabilitiesPolicyTypeFromString,
+                                     VIR_XML_PROP_NONE, &policy)) < 0)
+                return -1;
+
+            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;
+        }
 
         case VIR_DOMAIN_FEATURE_GIC:
-            if ((tmp = virXMLPropString(nodes[i], "version"))) {
-                int gic_version = virGICVersionTypeFromString(tmp);
-                if (gic_version < 0 || gic_version == VIR_GIC_VERSION_NONE) {
-                    virReportError(VIR_ERR_XML_ERROR,
-                                   _("malformed gic version: %s"), tmp);
-                    return -1;
-                }
-                def->gic_version = gic_version;
-            }
             def->features[val] = VIR_TRISTATE_SWITCH_ON;
+
+            if (virXMLPropEnum(nodes[i], "version",
+                               virGICVersionTypeFromString,
+                               VIR_XML_PROP_NONZERO, &def->gic_version) < 0)
+                return -1;
             break;
 
-        case VIR_DOMAIN_FEATURE_IOAPIC:
-            tmp = virXMLPropString(nodes[i], "driver");
-            if (tmp) {
-                int value = virDomainIOAPICTypeFromString(tmp);
-                if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("Unknown driver mode: %s"),
-                                   tmp);
-                    return -1;
-                }
-                def->features[val] = value;
-            }
+        case VIR_DOMAIN_FEATURE_IOAPIC: {
+            virDomainIOAPIC driver;
+
+            if ((rc = virXMLPropEnum(nodes[i], "driver",
+                                     virDomainIOAPICTypeFromString,
+                                     VIR_XML_PROP_NONZERO, &driver)) < 0)
+                return -1;
+
+            if (rc == 1)
+                def->features[val] = driver;
             break;
+        }
 
         case VIR_DOMAIN_FEATURE_HPT:
-            tmp = virXMLPropString(nodes[i], "resizing");
-            if (tmp) {
-                int value = virDomainHPTResizingTypeFromString(tmp);
-                if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("Unknown HPT resizing setting: %s"),
-                                   tmp);
-                    return -1;
-                }
-                def->hpt_resizing = (virDomainHPTResizing) value;
-            }
+            if (virXMLPropEnum(nodes[i], "resizing",
+                               virDomainHPTResizingTypeFromString,
+                               VIR_XML_PROP_NONZERO, &def->hpt_resizing) < 0)
+                return -1;
 
             if (virParseScaledValue("./features/hpt/maxpagesize",
                                     NULL,
@@ -17986,64 +17978,57 @@ virDomainFeaturesDefParse(virDomainDef *def,
             }
             break;
 
-        case VIR_DOMAIN_FEATURE_CFPC:
-            tmp = virXMLPropString(nodes[i], "value");
-            if (tmp) {
-                int value = virDomainCFPCTypeFromString(tmp);
-                if (value < 0 || value == VIR_DOMAIN_CFPC_NONE) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("Unknown value: %s"),
-                                   tmp);
-                    return -1;
-                }
+        case VIR_DOMAIN_FEATURE_CFPC: {
+            virDomainCFPC value;
+
+            if ((rc = virXMLPropEnum(nodes[i], "value",
+                                     virDomainCFPCTypeFromString,
+                                     VIR_XML_PROP_NONZERO, &value)) < 0)
+                return -1;
+
+            if (rc == 1)
                 def->features[val] = value;
-            }
             break;
+        }
 
-        case VIR_DOMAIN_FEATURE_SBBC:
-            tmp = virXMLPropString(nodes[i], "value");
-            if (tmp) {
-                int value = virDomainSBBCTypeFromString(tmp);
-                if (value < 0 || value == VIR_DOMAIN_SBBC_NONE) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("Unknown value: %s"),
-                                   tmp);
-                    return -1;
-                }
+        case VIR_DOMAIN_FEATURE_SBBC: {
+            virDomainSBBC value;
+
+            if ((rc = virXMLPropEnum(nodes[i], "value",
+                                     virDomainSBBCTypeFromString,
+                                     VIR_XML_PROP_NONZERO, &value)) < 0)
+                return -1;
+
+            if (rc == 1)
                 def->features[val] = value;
-            }
             break;
+        }
 
-        case VIR_DOMAIN_FEATURE_IBS:
-            tmp = virXMLPropString(nodes[i], "value");
-            if (tmp) {
-                int value = virDomainIBSTypeFromString(tmp);
-                if (value < 0 || value == VIR_DOMAIN_IBS_NONE) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("Unknown value: %s"),
-                                   tmp);
-                    return -1;
-                }
+        case VIR_DOMAIN_FEATURE_IBS: {
+            virDomainIBS value;
+
+            if ((rc = virXMLPropEnum(nodes[i], "value",
+                                      virDomainIBSTypeFromString,
+                                      VIR_XML_PROP_NONZERO, &value)) < 0)
+                return -1;
+
+            if (rc == 1)
                 def->features[val] = value;
-            }
             break;
+        }
 
         case VIR_DOMAIN_FEATURE_HTM:
         case VIR_DOMAIN_FEATURE_NESTED_HV:
-        case VIR_DOMAIN_FEATURE_CCF_ASSIST:
-            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("missing state attribute '%s' of feature '%s'"),
-                               tmp, virDomainFeatureTypeToString(val));
-                return -1;
-            }
-            if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("unknown state attribute '%s' of feature '%s'"),
-                               tmp, virDomainFeatureTypeToString(val));
+        case VIR_DOMAIN_FEATURE_CCF_ASSIST: {
+            virTristateSwitch state;
+
+            if (virXMLPropTristateSwitch(nodes[i], "state",
+                                         VIR_XML_PROP_REQUIRED, &state) < 0)
                 return -1;
-            }
+
+            def->features[val] = state;
             break;
+        }
 
         /* coverity[dead_error_begin] */
         case VIR_DOMAIN_FEATURE_LAST:
@@ -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];
 
-            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) {
-                    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",
@@ -18161,13 +18131,10 @@ virDomainFeaturesDefParse(virDomainDef *def,
     }
 
     if (def->hyperv_features[VIR_DOMAIN_HYPERV_STIMER] == VIR_TRISTATE_SWITCH_ON) {
-        int value;
         if ((n = virXPathNodeSet("./features/hyperv/stimer/*", ctxt, &nodes)) < 0)
             return -1;
 
         for (i = 0; i < n; i++) {
-            g_autofree char *tmp = NULL;
-
             if (STRNEQ((const char *)nodes[i]->name, "direct")) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                _("unsupported Hyper-V stimer feature: %s"),
@@ -18175,33 +18142,21 @@ virDomainFeaturesDefParse(virDomainDef *def,
                 return -1;
             }
 
-            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
-                virReportError(VIR_ERR_XML_ERROR,
-                               _("missing 'state' attribute for "
-                                 "Hyper-V stimer '%s' feature"), "direct");
-                        return -1;
-            }
-
-            if ((value = virTristateSwitchTypeFromString(tmp)) < 0) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("invalid value of state argument "
-                                 "for Hyper-V stimer '%s' feature"), "direct");
+            if (virXMLPropTristateSwitch(nodes[i], "state",
+                                         VIR_XML_PROP_REQUIRED,
+                                         &def->hyperv_stimer_direct) < 0)
                 return -1;
-            }
-
-            def->hyperv_stimer_direct = value;
         }
         VIR_FREE(nodes);
     }
 
     if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) {
-        int feature;
-        int value;
         if ((n = virXPathNodeSet("./features/kvm/*", ctxt, &nodes)) < 0)
             return -1;
 
         for (i = 0; i < n; i++) {
-            g_autofree char *tmp = NULL;
+            virTristateSwitch value;
+            int feature;
 
             feature = virDomainKVMTypeFromString((const char *)nodes[i]->name);
             if (feature < 0) {
@@ -18215,21 +18170,10 @@ virDomainFeaturesDefParse(virDomainDef *def,
                 case VIR_DOMAIN_KVM_HIDDEN:
                 case VIR_DOMAIN_KVM_DEDICATED:
                 case VIR_DOMAIN_KVM_POLLCONTROL:
-                    if (!(tmp = virXMLPropString(nodes[i], "state"))) {
-                        virReportError(VIR_ERR_XML_ERROR,
-                                       _("missing 'state' attribute for "
-                                         "KVM feature '%s'"),
-                                       nodes[i]->name);
+                    if (virXMLPropTristateSwitch(nodes[i], "state",
+                                                 VIR_XML_PROP_REQUIRED,
+                                                 &value) < 0)
                         return -1;
-                    }
-
-                    if ((value = virTristateSwitchTypeFromString(tmp)) < 0) {
-                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                       _("invalid value of state argument "
-                                         "for KVM feature '%s'"),
-                                       nodes[i]->name);
-                        return -1;
-                    }
 
                     def->kvm_features[feature] = value;
                     break;
@@ -18243,15 +18187,12 @@ virDomainFeaturesDefParse(virDomainDef *def,
     }
 
     if (def->features[VIR_DOMAIN_FEATURE_XEN] == VIR_TRISTATE_SWITCH_ON) {
-        int feature;
-        int value;
-        g_autofree char *ptval = NULL;
-
         if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0)
             return -1;
 
         for (i = 0; i < n; i++) {
-            g_autofree char *tmp = NULL;
+            virTristateSwitch value;
+            int feature;
             feature = virDomainXenTypeFromString((const char *)nodes[i]->name);
             if (feature < 0) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -18260,21 +18201,9 @@ virDomainFeaturesDefParse(virDomainDef *def,
                 return -1;
             }
 
-            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
-                virReportError(VIR_ERR_XML_ERROR,
-                               _("missing 'state' attribute for "
-                                 "Xen feature '%s'"),
-                               nodes[i]->name);
-                return -1;
-            }
-
-            if ((value = virTristateSwitchTypeFromString(tmp)) < 0) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("invalid value of state argument "
-                                 "for Xen feature '%s'"),
-                               nodes[i]->name);
+            if (virXMLPropTristateSwitch(nodes[i], "state",
+                                         VIR_XML_PROP_REQUIRED, &value) < 0)
                 return -1;
-            }
 
             def->xen_features[feature] = value;
 
@@ -18286,25 +18215,11 @@ virDomainFeaturesDefParse(virDomainDef *def,
                 if (value != VIR_TRISTATE_SWITCH_ON)
                     break;
 
-                if ((ptval = virXMLPropString(nodes[i], "mode"))) {
-                    int mode = virDomainXenPassthroughModeTypeFromString(ptval);
-
-                    if (mode < 0) {
-                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                       _("unsupported mode '%s' for Xen passthrough feature"),
-                                       ptval);
-                        return -1;
-                    }
-
-                    if (mode != VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SYNC_PT &&
-                        mode != VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SHARE_PT) {
-                        virReportError(VIR_ERR_XML_ERROR, "%s",
-                                       _("'mode' attribute for Xen feature "
-                                         "'passthrough' must be 'sync_pt' or 'share_pt'"));
-                        return -1;
-                    }
-                    def->xen_passthrough_mode = mode;
-                }
+                if (virXMLPropEnum(nodes[i], "mode",
+                                   virDomainXenPassthroughModeTypeFromString,
+                                   VIR_XML_PROP_NONZERO,
+                                   &def->xen_passthrough_mode) < 0)
+                    return -1;
                 break;
 
                 /* coverity[dead_error_begin] */
@@ -18329,31 +18244,23 @@ virDomainFeaturesDefParse(virDomainDef *def,
     }
 
     if (def->features[VIR_DOMAIN_FEATURE_MSRS] == VIR_TRISTATE_SWITCH_ON) {
-        g_autofree char *tmp = NULL;
+        virDomainMsrsUnknown unknown;
         xmlNodePtr node = NULL;
         if ((node = virXPathNode("./features/msrs", ctxt)) == NULL)
             return -1;
 
-        if (!(tmp = virXMLPropString(node, "unknown"))) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("missing 'unknown' attribute for feature '%s'"),
-                           virDomainFeatureTypeToString(VIR_DOMAIN_FEATURE_MSRS));
+        if (virXMLPropEnum(node, "unknown", virDomainMsrsUnknownTypeFromString,
+                           VIR_XML_PROP_REQUIRED, &unknown) < 0)
             return -1;
-        }
 
-        if ((def->msrs_features[VIR_DOMAIN_MSRS_UNKNOWN] = virDomainMsrsUnknownTypeFromString(tmp)) < 0) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("unknown 'unknown' value '%s'"),
-                           tmp);
-            return -1;
-        }
+        def->msrs_features[VIR_DOMAIN_MSRS_UNKNOWN] = unknown;
     }
 
     if ((n = virXPathNodeSet("./features/capabilities/*", ctxt, &nodes)) < 0)
         return -1;
 
     for (i = 0; i < n; i++) {
-        g_autofree char *tmp = NULL;
+        virTristateSwitch state = VIR_TRISTATE_SWITCH_ON;
         int val = virDomainProcessCapsFeatureTypeFromString((const char *)nodes[i]->name);
         if (val < 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -18361,16 +18268,12 @@ virDomainFeaturesDefParse(virDomainDef *def,
             return -1;
         }
 
-        if ((tmp = virXMLPropString(nodes[i], "state"))) {
-            if ((def->caps_features[val] = virTristateSwitchTypeFromString(tmp)) == -1) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("unknown state attribute '%s' of feature capability '%s'"),
-                               tmp, virDomainProcessCapsFeatureTypeToString(val));
-                return -1;
-            }
-        } else {
-            def->caps_features[val] = VIR_TRISTATE_SWITCH_ON;
-        }
+
+        if (virXMLPropTristateSwitch(nodes[i], "state", VIR_XML_PROP_NONE,
+                                     &state) < 0)
+            return -1;
+
+        def->caps_features[val] = state;
     }
     VIR_FREE(nodes);
     return 0;
diff --git a/tests/qemuxml2argvdata/aarch64-gic-invalid.err b/tests/qemuxml2argvdata/aarch64-gic-invalid.err
index c2e9f4aa3f..18de19f660 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-invalid.err
+++ b/tests/qemuxml2argvdata/aarch64-gic-invalid.err
@@ -1 +1 @@
-XML error: malformed gic version: none
+XML error: Invalid value for attribute 'version' in element 'gic': 'none'.
-- 
2.26.3

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

Posted by Peter Krempa 1 month, 3 weeks ago
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@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",

[...]