[libvirt] [PATCH 1/6] conf: remove pointless check on enum value

[Daniel P. Berrangé]

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