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