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

[Cole Robinson]

I pushed the first three patches. Comments below

On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
> The current code doesn't properly handle errors when parsing a video
> device's resolution.
> 
> This patch changes the parse function signature to return an error
> when we're missing an 'x' or 'y' parameter or when the 'x' or 'y'
> parameters are not positive integers. No error is returned when no
> 'resolution' element is found.
> 
> Previously we were returning a NULL structure for the case where 'x' or
> 'y' were missing, but were returning an under-specified structure for
> the other error cases.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>  src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 269a6ec2c3..b3f32d0b63 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>      return g_steal_pointer(&def);
>  }
>  
> -static virDomainVideoResolutionDefPtr
> -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
> +static int
> +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
> +                                    virDomainVideoResolutionDefPtr *res)
>  {
>      xmlNodePtr cur;
>      g_autofree virDomainVideoResolutionDefPtr def = NULL;
>      g_autofree char *x = NULL;
>      g_autofree char *y = NULL;
>  
> +    *res = NULL;
>      cur = node->children;
>      while (cur != NULL) {
> -        if (cur->type == XML_ELEMENT_NODE) {
> -            if (!x && !y &&
> -                virXMLNodeNameEqual(cur, "resolution")) {
> -                x = virXMLPropString(cur, "x");
> -                y = virXMLPropString(cur, "y");
> -            }
> +        if ((cur->type == XML_ELEMENT_NODE) &&
> +            virXMLNodeNameEqual(cur, "resolution")) {
> +            x = virXMLPropString(cur, "x");
> +            y = virXMLPropString(cur, "y");
> +            break;
>          }
>          cur = cur->next;
>      }
>  

This loop can be removed if you move the virXMLNodeNameEqual to the
parent function, like how it is handled for parent call of
virDomainVirtioOptionsParseXML

> +    /* resolution not specified */
> +    if (cur == NULL)
> +        return 0;
> +
> +    /* resolution specified, but x or y is missing. report error */
>      if (!x || !y)
> -        return NULL;
> +        return -1;
>  
>      def = g_new0(virDomainVideoResolutionDef, 1);
>  
>      if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("cannot parse video x-resolution '%s'"), x);
> -        goto cleanup;
> +        return -1;
>      }
>  
>      if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("cannot parse video y-resolution '%s'"), y);
> -        goto cleanup;
> +        return -1;
>      }
>  
> - cleanup:
> -    return g_steal_pointer(&def);
> +    if (def->x == 0 || def->y == 0) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("resolution values must be greater than 0"));
> +        return -1;
> +    }
> +
> +    *res = g_steal_pointer(&def);
> +    return 0;
>  }
> 

This patch is doing two things: converting to the more common error
handling pattern, but also adding this error check. Separating them will
help review.

It's borderline pedantic but this type of error should actually be in
the Validate callback machinery instead. Some drivers will fill in a
virDomainDef manually in code (like virtualbox). If they accidentally
set an x or y value of 0, Validate won't catch it as an error, but the
XML parser will catch it later. Generally the XML parser should only
throw errors about

>  static virDomainVideoDriverDefPtr
> @@ -15458,7 +15470,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>                  }
>  
>                  def->accel = virDomainVideoAccelDefParseXML(cur);
> -                def->res = virDomainVideoResolutionDefParseXML(cur);
> +                if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                                   "%s", _("Cannot parse video resolution"));
> +                    goto error;
> +                }
>              }

Calling virReporError here will overwrite the error raised by
virDomainVideoResolutionDefParseXML. The error reporting machinery
doesn't have any smarts to merge errors or anything like that, it needs
to be done manually. In this case you can just drop the virReportError
entirely

I'll be quicker about reviewing v3.

Thanks,
Cole