[libvirt PATCH 1/1] conf: move graphics validation checks out of *ParseXML function.

[Nicolás Brignone]

Hi!
    Is this going to be merged? Do I need to do something else?

Thanks!

On Fri, Jul 24, 2020 at 5:08 PM Jonathon Jongsma <jjongsma at redhat.com>
wrote:

> On Wed, 2020-07-22 at 14:56 -0300, Nicolas Brignone wrote:
> > Existing virDomainDefPostParseGraphics function seems to be the right
> > place to put this validations.
> >
> > After moving this validation, one less argument is needed in
> > virDomainGraphicsListenDefParseXML, so removing the "graphics"
> > argument
> > from the function signature.
> >
> > Signed-off-by: Nicolas Brignone <nmbrignone at gmail.com>
> > ---
> >  src/conf/domain_conf.c | 66 +++++++++++++++++++++++-----------------
> > --
> >  1 file changed, 36 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 8d328819..3228f12a 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -4804,13 +4804,15 @@ virDomainDefPostParseTimer(virDomainDefPtr
> > def)
> >  }
> >
> >
> > -static void
> > +static int
> >  virDomainDefPostParseGraphics(virDomainDef *def)
> >  {
> >      size_t i;
> >
> >      for (i = 0; i < def->ngraphics; i++) {
> > +        size_t j;
> >          virDomainGraphicsDefPtr graphics = def->graphics[i];
> > +        const char *graphicsType =
> > virDomainGraphicsTypeToString(graphics->type);
> >
> >          /* If spice graphics is configured without ports and with
> > autoport='no'
> >           * then we start qemu with Spice to not listen
> > anywhere.  Let's convert
> > @@ -4826,8 +4828,38 @@ virDomainDefPostParseGraphics(virDomainDef
> > *def)
> >                  VIR_FREE(glisten->address);
> >                  glisten->type =
> > VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE;
> >              }
> > +
> > +        }
> > +
> > +        for (j = 0; j < graphics->nListens; j++) {
> > +            virDomainGraphicsListenDefPtr glisten =
> > virDomainGraphicsGetListen(graphics, j);
> > +            switch (glisten->type) {
> > +                case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> > +                    if (graphics->type !=
> > VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> > +                            graphics->type !=
> > VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> > +                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                                _("listen type 'socket' is not
> > available for "
> > +                                    "graphics type '%s'"),
> > graphicsType);
> > +                        return -1;
> > +                    }
> > +                    break;
> > +                case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> > +                    if (graphics->type !=
> > VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> > +                            graphics->type !=
> > VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> > +                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                                _("listen type 'none' is not
> > available for "
> > +                                    "graphics type '%s'"),
> > graphicsType);
> > +                        return -1;
> > +                    }
> > +                    break;
> > +                case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> > +                case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> > +                case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> > +                    break;
> > +            }
> >          }
> >      }
> > +    return 0;
> >  }
> >
> >
> > @@ -5915,7 +5947,8 @@ virDomainDefPostParseCommon(virDomainDefPtr
> > def,
> >      /* clean up possibly duplicated metadata entries */
> >      virXMLNodeSanitizeNamespaces(def->metadata);
> >
> > -    virDomainDefPostParseGraphics(def);
> > +    if (virDomainDefPostParseGraphics(def) < 0)
> > +        return -1;
> >
> >      if (virDomainDefPostParseCPU(def) < 0)
> >          return -1;
> > @@ -14140,13 +14173,11 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr
> > node,
> >   */
> >  static int
> >  virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
> > def,
> > -                                   virDomainGraphicsDefPtr graphics,
> >                                     xmlNodePtr node,
> >                                     xmlNodePtr parent,
> >                                     unsigned int flags)
> >  {
> >      int ret = -1;
> > -    const char *graphicsType =
> > virDomainGraphicsTypeToString(graphics->type);
> >      int tmp, typeVal;
> >      g_autofree char *type = virXMLPropString(node, "type");
> >      g_autofree char *address = virXMLPropString(node, "address");
> > @@ -14175,31 +14206,6 @@
> > virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> >      }
> >      def->type = typeVal;
> >
> > -    switch (def->type) {
> > -    case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> > -        if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> > -            graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> > -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -                           _("listen type 'socket' is not available
> > for "
> > -                             "graphics type '%s'"), graphicsType);
> > -            goto error;
> > -        }
> > -        break;
> > -    case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> > -        if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> > -            graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> > -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > -                           _("listen type 'none' is not available
> > for "
> > -                             "graphics type '%s'"), graphicsType);
> > -            goto error;
> > -        }
> > -        break;
> > -    case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> > -    case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> > -    case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> > -        break;
> > -    }
> > -
> >      if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
> >          if (address && addressCompat && STRNEQ(address,
> > addressCompat)) {
> >              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > @@ -14309,7 +14315,7 @@
> > virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def,
> >              goto cleanup;
> >
> >          for (i = 0; i < nListens; i++) {
> > -            if (virDomainGraphicsListenDefParseXML(&def->listens[i],
> > def,
> > +            if (virDomainGraphicsListenDefParseXML(&def->listens[i],
> >                                                     listenNodes[i],
> >                                                     i == 0 ? node :
> > NULL,
> >                                                     flags) < 0)
>
>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
>
> It looks like there are still some additional validation checks in this
> function that could be also moved in subsequent patches if you're
> motivated.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200730/cd6ff224/attachment-0001.htm>