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

[John Ferlan]

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.


> +                goto error;
>              }
> +            VIR_FREE(tmp);
> +        } else {
> +            def->caps_features[val] = VIR_TRISTATE_SWITCH_ON;
>          }
>      }
>      VIR_FREE(nodes);
>