<div dir="ltr"><div>Hi!</div><div> Is this going to be merged? Do I need to do something else?</div><div> </div><div>Thanks! </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 24, 2020 at 5:08 PM Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com">jjongsma@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 <<a href="mailto:nmbrignone@gmail.com" target="_blank">nmbrignone@gmail.com</a>> > --- > 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 <<a href="mailto:jjongsma@redhat.com" target="_blank">jjongsma@redhat.com</a>> 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. </blockquote></div>