On Wed, Jan 30, 2019 at 04:07:13PM -0500, John Ferlan wrote:
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@redhat.com> --- src/conf/domain_conf.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
Reviewed-by: John Ferlan <jferlan@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.
Oh fun. I'll fix this in a separate patch with your R-b.
+ goto error; } + VIR_FREE(tmp); + } else { + def->caps_features[val] = VIR_TRISTATE_SWITCH_ON; } } VIR_FREE(nodes);
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|