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@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@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.