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