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@xxxxxxxxxx>
> > ---
> > 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?
>
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
