[libvirt] [PATCH v2 5/5] conf: report errors when parsing video acceleration

[Cole Robinson]

On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
> In order to differentiate between a configuration that does not specify
> any video acceleration and one that is incorrectly specified, change the
> signature of virDomainVideoAccelDefParseXML() to return an error
> response.
> 
> If any of the values are invalid, report an error rather than returning
> a partially-specified accel object. If no 'acceleration' node is found,
> do not report an error since it is optional.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>  src/conf/domain_conf.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b3f32d0b63..1c3fc5c4ce 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15285,8 +15285,8 @@ virDomainVideoDefaultType(const virDomainDef *def)
>      }
>  }
>  
> -static virDomainVideoAccelDefPtr
> -virDomainVideoAccelDefParseXML(xmlNodePtr node)
> +static int
> +virDomainVideoAccelDefParseXML(xmlNodePtr node, virDomainVideoAccelDefPtr *accel)
>  {
>      xmlNodePtr cur;
>      g_autofree virDomainVideoAccelDefPtr def = NULL;
> @@ -15295,21 +15295,25 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>      g_autofree char *accel3d = NULL;
>      g_autofree char *rendernode = NULL;
>  
> +    *accel = NULL;
>      cur = node->children;
>      while (cur != NULL) {
> -        if (cur->type == XML_ELEMENT_NODE) {
> -            if (!accel3d && !accel2d &&
> -                virXMLNodeNameEqual(cur, "acceleration")) {
> -                accel3d = virXMLPropString(cur, "accel3d");
> -                accel2d = virXMLPropString(cur, "accel2d");
> -                rendernode = virXMLPropString(cur, "rendernode");
> -            }
> +        if ((cur->type == XML_ELEMENT_NODE) &&
> +            virXMLNodeNameEqual(cur, "acceleration")) {
> +            accel3d = virXMLPropString(cur, "accel3d");
> +            accel2d = virXMLPropString(cur, "accel2d");
> +            rendernode = virXMLPropString(cur, "rendernode");
> +            break;
>          }
>          cur = cur->next;
>      }
>  

THe virXMLNodeNameEqual check should be moved to the parent, same as
mentioned in patch 4

> +    /* no acceleration node was specified */
> +    if (cur == NULL)
> +        return 0;
> +
>      if (!accel3d && !accel2d && !rendernode)
> -        return NULL;
> +        return -1;
>  
>      def = g_new0(virDomainVideoAccelDef, 1);
>  
> @@ -15317,7 +15321,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>          if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                             _("unknown accel3d value '%s'"), accel3d);
> -            goto cleanup;
> +            return -1;
>          }
>          def->accel3d = val;
>      }
> @@ -15326,7 +15330,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>          if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                             _("unknown accel2d value '%s'"), accel2d);
> -            goto cleanup;
> +            return -1;
>          }
>          def->accel2d = val;
>      }
> @@ -15334,8 +15338,8 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>      if (rendernode)
>          def->rendernode = virFileSanitizePath(rendernode);
>  
> - cleanup:
> -    return g_steal_pointer(&def);
> +    *accel = g_steal_pointer(&def);
> +    return 0;
>  }
>  
>  static int
> @@ -15469,7 +15473,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>                      VIR_FREE(primary);
>                  }
>  
> -                def->accel = virDomainVideoAccelDefParseXML(cur);
> +                if (virDomainVideoAccelDefParseXML(cur, &def->accel) < 0) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                                   "%s", _("Cannot parse video acceleration"));
> +                    goto error;
> +                }
>                  if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) {
>                      virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                                     "%s", _("Cannot parse video resolution"));
> 

Same error overwriting issue here as in patch 4

- Cole