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

[Cole Robinson]

On 11/13/19 5:41 PM, Jonathon Jongsma wrote:
> On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
>> 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
> 
> It seems that this sentence is unfinished?
> 

I meant to delete that unformed thought because it was going to turn
into more than a sentence and would distract from this patch :)

Generally the XML parser should only raise errors that are specific to
the XML input. Anything that's validating a value after it has already
been parsed is better served in the Validate callbacks. The existing
code is not the best guide for this though, it's just the goal we should
shoot for IMO

- Cole