Re: [libvirt] [PATCH 1/6] conf: remove pointless check on enum value
On 1/30/19 12:40 PM, Daniel P. Berrangé wrote:
'val' is initialized from virDomainCapsFeatureTypeFromString
and a
few lines earlier there was already a check for 'val < 0'.
The 'val >= 0' is thus always true. The enum conversion similarly
ensures that the val will be less than VIR_DOMAIN_CAPS_FEATURE_LAST,
so "val < VIR_DOMAIN_CAPS_FEATURE_LAST' is thus always true too.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/conf/domain_conf.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
However, do see below for a change that either could be made now or as a
separate patch afterwards with an implied R-by as well.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9409d93c23..f2ef53a9a3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -20524,18 +20524,16 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
}
- if (val >= 0 && val < VIR_DOMAIN_CAPS_FEATURE_LAST) {
- 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, virDomainFeatureTypeToString(val));
- goto error;
- }
- VIR_FREE(tmp);
- } else {
- def->caps_features[val] = VIR_TRISTATE_SWITCH_ON;
+ 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, virDomainFeatureTypeToString(val));
^^^^
This caught my eye...
Since @val was sourced from virDomainCapsFeatureTypeFromString so one
would think it should use virDomainCapsFeatureTypeToString(val) at the
very least. Or use 'nodes[i]->name' directly.
As a test I modified tests/domainschemadata/domain-caps-features.xml to
set state to "foo" and then creating the lxc guest would result in :
error: unsupported configuration: unknown state attribute 'foo' of
feature capability 'hpt'
fixing to use DomainCapsFeature got:
error: unsupported configuration: unknown state attribute 'foo' of
feature capability 'mknod'
I checked other virDomainFeatureTypeToString callers and they all seemed
OK - it was just this one.
+ goto error;
}
+ VIR_FREE(tmp);
+ } else {
+ def->caps_features[val] = VIR_TRISTATE_SWITCH_ON;
}
}
VIR_FREE(nodes);