[libvirt PATCH 0/5] Some fixes in domain_conf.c: virDomainFeaturesDefParse
Patches 1-4 are minor cleanups, patch 5 is the actual one that I want out of the way before continuing down my rabbit hole of the week. Tim Wiederhake (5): domain_conf: Reduce scope of tmp in virDomainFeaturesDefParse domain_conf: Reduce scope of gic_version in virDomainFeaturesDefParse domain_conf: Reduce scope of node in virDomainFeaturesDefParse domain_conf: Add missing break in switch domain_conf: Fix check for hyperv stimer src/conf/domain_conf.c | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) -- 2.26.2
Variables using `g_autofree` should not be manually VIR_FREE'd and reused. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f194909b13..1d0ef17318 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18156,7 +18156,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, xmlXPathContextPtr ctxt) { g_autofree xmlNodePtr *nodes = NULL; - g_autofree char *tmp = NULL; xmlNodePtr node = NULL; int gic_version; size_t i; @@ -18166,6 +18165,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; for (i = 0; i < n; i++) { + g_autofree char *tmp = NULL; int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name); if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18184,7 +18184,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } def->apic_eoi = eoi; - VIR_FREE(tmp); } G_GNUC_FALLTHROUGH; case VIR_DOMAIN_FEATURE_ACPI: @@ -18206,7 +18205,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, tmp, virDomainFeatureTypeToString(val)); return -1; } - VIR_FREE(tmp); } else { def->features[val] = VIR_TRISTATE_SWITCH_ABSENT; } @@ -18225,7 +18223,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, tmp, virDomainFeatureTypeToString(val)); return -1; } - VIR_FREE(tmp); } else { def->features[val] = VIR_TRISTATE_SWITCH_ON; } @@ -18240,7 +18237,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } def->gic_version = gic_version; - VIR_FREE(tmp); } def->features[val] = VIR_TRISTATE_SWITCH_ON; break; @@ -18256,7 +18252,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } def->features[val] = value; - VIR_FREE(tmp); } break; @@ -18271,7 +18266,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } def->hpt_resizing = (virDomainHPTResizing) value; - VIR_FREE(tmp); } if (virParseScaledValue("./features/hpt/maxpagesize", @@ -18305,7 +18299,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } def->features[val] = value; - VIR_FREE(tmp); } break; @@ -18320,7 +18313,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } def->features[val] = value; - VIR_FREE(tmp); } break; @@ -18335,7 +18327,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } def->features[val] = value; - VIR_FREE(tmp); } break; @@ -18354,7 +18345,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, tmp, virDomainFeatureTypeToString(val)); return -1; } - VIR_FREE(tmp); break; /* coverity[dead_error_begin] */ @@ -18372,6 +18362,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; for (i = 0; i < n; i++) { + g_autofree char *tmp = NULL; feature = virDomainHypervTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18398,7 +18389,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } - VIR_FREE(tmp); def->hyperv_features[feature] = value; switch ((virDomainHyperv) feature) { @@ -18477,6 +18467,8 @@ virDomainFeaturesDefParse(virDomainDefPtr def, 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"), @@ -18498,7 +18490,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } - VIR_FREE(tmp); def->hyperv_stimer_direct = value; } VIR_FREE(nodes); @@ -18511,6 +18502,8 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; for (i = 0; i < n; i++) { + g_autofree char *tmp = NULL; + feature = virDomainKVMTypeFromString((const char *)nodes[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18539,7 +18532,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } - VIR_FREE(tmp); def->kvm_features[feature] = value; break; @@ -18555,6 +18547,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, int feature; int value; g_autofree char *ptval = NULL; + g_autofree char *tmp = NULL; if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0) return -1; @@ -18584,7 +18577,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, return -1; } - VIR_FREE(tmp); def->xen_features[feature] = value; switch ((virDomainXen) feature) { @@ -18638,6 +18630,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, } if (def->features[VIR_DOMAIN_FEATURE_MSRS] == VIR_TRISTATE_SWITCH_ON) { + g_autofree char *tmp = NULL; if ((node = virXPathNode("./features/msrs", ctxt)) == NULL) return -1; @@ -18654,13 +18647,13 @@ virDomainFeaturesDefParse(virDomainDefPtr def, tmp); return -1; } - VIR_FREE(tmp); } if ((n = virXPathNodeSet("./features/capabilities/*", ctxt, &nodes)) < 0) return -1; for (i = 0; i < n; i++) { + g_autofree char *tmp = NULL; int val = virDomainProcessCapsFeatureTypeFromString((const char *)nodes[i]->name); if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18675,7 +18668,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, tmp, virDomainProcessCapsFeatureTypeToString(val)); return -1; } - VIR_FREE(tmp); } else { def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; } -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1d0ef17318..f73814f3c7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18157,7 +18157,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, { g_autofree xmlNodePtr *nodes = NULL; xmlNodePtr node = NULL; - int gic_version; size_t i; int n; @@ -18230,7 +18229,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, case VIR_DOMAIN_FEATURE_GIC: if ((tmp = virXMLPropString(nodes[i], "version"))) { - gic_version = virGICVersionTypeFromString(tmp); + 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); -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f73814f3c7..4bc82b69fc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18156,7 +18156,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, xmlXPathContextPtr ctxt) { g_autofree xmlNodePtr *nodes = NULL; - xmlNodePtr node = NULL; size_t i; int n; @@ -18356,7 +18355,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { int feature; int value; - node = ctxt->node; + xmlNodePtr node = ctxt->node; if ((n = virXPathNodeSet("./features/hyperv/*", ctxt, &nodes)) < 0) return -1; @@ -18630,6 +18629,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, if (def->features[VIR_DOMAIN_FEATURE_MSRS] == VIR_TRISTATE_SWITCH_ON) { g_autofree char *tmp = NULL; + xmlNodePtr node = NULL; if ((node = virXPathNode("./features/msrs", ctxt)) == NULL) return -1; -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4bc82b69fc..48fd078b90 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18449,6 +18449,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("HyperV vendor_id value is invalid")); return -1; } + break; /* coverity[dead_error_begin] */ case VIR_DOMAIN_HYPERV_LAST: -- 2.26.2
VIR_DOMAIN_HYPERV_STIMER happens to have the same numerical value as VIR_DOMAIN_FEATURE_HYPERV, resulting in the if-block to always being executed when a "<hyperv>" tag is found, whether or not it actually contained a "<stimer>" tag. This had no ill effects, as virXPathNodeSet() would simply return 0 if that tag does not exist. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 48fd078b90..05b6cb3000 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18460,7 +18460,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, ctxt->node = node; } - if (def->features[VIR_DOMAIN_HYPERV_STIMER] == VIR_TRISTATE_SWITCH_ON) { + 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; -- 2.26.2
On 3/4/21 9:35 AM, Tim Wiederhake wrote:
VIR_DOMAIN_HYPERV_STIMER happens to have the same numerical value as VIR_DOMAIN_FEATURE_HYPERV, resulting in the if-block to always being executed when a "<hyperv>" tag is found, whether or not it actually contained a "<stimer>" tag. This had no ill effects, as virXPathNodeSet() would simply return 0 if that tag does not exist.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 48fd078b90..05b6cb3000 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18460,7 +18460,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, ctxt->node = node; }
- if (def->features[VIR_DOMAIN_HYPERV_STIMER] == VIR_TRISTATE_SWITCH_ON) { + 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;
Nice catch. Michal
On 3/4/21 9:35 AM, Tim Wiederhake wrote:
Patches 1-4 are minor cleanups, patch 5 is the actual one that I want out of the way before continuing down my rabbit hole of the week.
Tim Wiederhake (5): domain_conf: Reduce scope of tmp in virDomainFeaturesDefParse domain_conf: Reduce scope of gic_version in virDomainFeaturesDefParse domain_conf: Reduce scope of node in virDomainFeaturesDefParse domain_conf: Add missing break in switch domain_conf: Fix check for hyperv stimer
src/conf/domain_conf.c | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
participants (2)
-
Michal Privoznik -
Tim Wiederhake