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&gt; > --- > src/conf/domain_conf.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; 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); >