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@xxxxxxxxxx>
> > ---
> > src/conf/domain_conf.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
>
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
