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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux