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

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(a)redhat.com&gt; >> --- >> 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?