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

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

> 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