On 06/27/2018 09:34 AM, Erik Skultety wrote:
> Since we support gl with SPICE and SDL with future VNC enablement in
> sight (egl-headless), let's separate the gl-related attributes into a
> standalone structure.
>
> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 137 +++++++++++++++++++++++++-------------------
> src/conf/domain_conf.h | 12 +++-
> src/qemu/qemu_cgroup.c | 10 ++--
> src/qemu/qemu_command.c | 66 ++++++++++++---------
> src/qemu/qemu_domain.c | 7 ++-
> src/security/security_dac.c | 7 ++-
> 6 files changed, 138 insertions(+), 101 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 09d9bac739..6bfa3ca130 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1420,8 +1420,6 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
> break;
>
> case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> - VIR_FREE(def->data.spice.rendernode);
> - VIR_FREE(def->data.spice.keymap);
Perhaps a bit too aggressive here? Restore the keymap FREE - it's
allocated in virDomainGraphicsDefParseXMLSpice
> virDomainGraphicsAuthDefClear(&def->data.spice.auth);
> break;
>
> @@ -1429,6 +1427,8 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
> break;
> }
>
> + virDomainGraphicsGLDefFree(def->gl);
> +
> for (i = 0; i < def->nListens; i++)
> virDomainGraphicsListenDefClear(&def->listens[i]);
> VIR_FREE(def->listens);
> @@ -1436,6 +1436,18 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
> VIR_FREE(def);
> }
>
> +
> +void
> +virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def)
The only caller is above - even after the last patch, thus this should
be static to this function and above the previous.
> +{
> + if (!def)
> + return;
> +
> + VIR_FREE(def->rendernode);
> + VIR_FREE(def);
> +}
> +
> +
> const char *virDomainInputDefGetPath(virDomainInputDefPtr input)
> {
> switch ((virDomainInputType) input->type) {
> @@ -13555,6 +13567,48 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def,
> }
>
>
> +static int
> +virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def,
> + xmlNodePtr node)
> +{
> + virDomainGraphicsGLDefPtr gl = NULL;
> + char *enable = NULL;
> + int ret = -1;
> +
> + if (!node)
> + return 0;
> +
> + if (VIR_ALLOC(gl) < 0)
> + return -1;
> +
> + if (!(enable = virXMLPropString(node, "enable"))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("'enable' attribute is mandatory for graphics "
> + "<gl> element"));
> + goto cleanup;
> + }
> +
> + if ((gl->enable =
> + virTristateBoolTypeFromString(enable)) <= 0) {
This should go on the previous line - it fits, I checked.
The <= changes from the previous code which had:
- enableVal = virTristateBoolTypeFromString(enable);
- if (enableVal < 0) {
hopefully there's no "default"'s in the wild. But it is fine given the
previous usage model of absent being the default and being checked.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown value for attribute enable '%s'"),
> + enable);
> + goto cleanup;
> + }
> +
> + if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)
> + gl->rendernode = virXMLPropString(node, "rendernode");
> +
> + VIR_STEAL_PTR(def->gl, gl);
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(enable);
> + virDomainGraphicsGLDefFree(gl);
> + return ret;
> +}
> +
> +
> static int
> virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,
> xmlNodePtr node,
> @@ -13644,8 +13698,6 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
> {
> xmlNodePtr save = ctxt->node;
> char *enable = NULL;
> - int enableVal;
> - xmlNodePtr glNode;
> char *fullscreen = virXMLPropString(node, "fullscreen");
> int ret = -1;
>
> @@ -13668,23 +13720,9 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
> def->data.sdl.xauth = virXMLPropString(node, "xauth");
> def->data.sdl.display = virXMLPropString(node, "display");
>
> - glNode = virXPathNode("./gl", ctxt);
> - if (glNode) {
> - enable = virXMLPropString(glNode, "enable");
> - if (!enable) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("sdl gl element missing enable"));
> - goto cleanup;
> - }
> -
> - enableVal = virTristateBoolTypeFromString(enable);
> - if (enableVal < 0) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("unknown enable value '%s'"), enable);
> - goto cleanup;
> - }
> - def->data.sdl.gl = enableVal;
> - }
> + if (virDomainGraphicsGLDefParseXML(def,
> + virXPathNode("./gl[1]", ctxt)) < 0)
Move to the previous line, it fits, I checked.
> + goto cleanup;
>
> ret = 0;
> cleanup:
> @@ -14026,31 +14064,6 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,
> VIR_FREE(enable);
>
> def->data.spice.filetransfer = enableVal;
> - } else if (virXMLNodeNameEqual(cur, "gl")) {
> - char *enable = virXMLPropString(cur, "enable");
> - char *rendernode = virXMLPropString(cur, "rendernode");
> - int enableVal;
> -
> - if (!enable) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("spice gl element missing enable"));
> - VIR_FREE(rendernode);
> - goto error;
> - }
> -
> - if ((enableVal =
> - virTristateBoolTypeFromString(enable)) <= 0) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("unknown enable value '%s'"), enable);
> - VIR_FREE(enable);
> - VIR_FREE(rendernode);
> - goto error;
> - }
> - VIR_FREE(enable);
> -
> - def->data.spice.gl = enableVal;
> - def->data.spice.rendernode = rendernode;
> -
Was moving to the last else if intentional? IDC either way, but it is
strange.
> } else if (virXMLNodeNameEqual(cur, "mouse")) {
> char *mode = virXMLPropString(cur, "mode");
> int modeVal;
> @@ -14071,6 +14084,9 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,
[...]
> static int
> virDomainGraphicsDefFormat(virBufferPtr buf,
> virDomainGraphicsDefPtr def,
> @@ -26247,18 +26270,12 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
> if (def->data.sdl.fullscreen)
> virBufferAddLit(buf, " fullscreen='yes'");
>
> - if (!children && def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) {
> + if (!children && def->gl) {
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
> children = true;
> }
>
> - if (def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) {
> - virBufferAsprintf(buf, "<gl enable='%s'",
> - virTristateBoolTypeToString(def->data.sdl.gl));
> - virBufferAddLit(buf, "/>\n");
> - }
> -
> break;
>
> case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> @@ -26405,8 +26422,7 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
> if (!children && (def->data.spice.image || def->data.spice.jpeg ||
> def->data.spice.zlib || def->data.spice.playback ||
> def->data.spice.streaming || def->data.spice.copypaste ||
> - def->data.spice.mousemode || def->data.spice.filetransfer ||
> - def->data.spice.gl)) {
> + def->data.spice.mousemode || def->data.spice.filetransfer)) {
Interesting, it's a bit sneaky but even when not provided the above
format for <listen type...> will format to 'none' meaning we don't have
to worry about def->gl here (even if autoport isn't provided for
<graphics...>
Hopefully that remains "true" going forward in time.
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
> children = true;
> @@ -26436,9 +26452,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n",
> virTristateBoolTypeToString(def->data.spice.filetransfer));
>
> - virDomainSpiceGLDefFormat(buf, def);
> }
>
> + virDomainGraphicsGLDefFormat(buf, def);
> +
> if (children) {
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</graphics>\n");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0924fc4f3c..20dc1334c4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
[...]
>
> typedef enum {
> @@ -2837,6 +2842,7 @@ int virDomainObjWaitUntil(virDomainObjPtr vm,
> void virDomainPanicDefFree(virDomainPanicDefPtr panic);
> void virDomainResourceDefFree(virDomainResourceDefPtr resource);
> void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def);
> +void virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def);
This won't be necessary.... The "tip off" is that the private syms file
wasn't touched...
> const char *virDomainInputDefGetPath(virDomainInputDefPtr input);
> void virDomainInputDefFree(virDomainInputDefPtr def);
> virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt);
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 195d03e373..ef0be95b0f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7740,7 +7740,8 @@ qemuBuildGraphicsSDLCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
> virCommandAddArg(cmd, "-display");
> virBufferAddLit(&opt, "sdl");
>
> - if (graphics->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) {
> + if (graphics->gl &&
> + graphics->gl->enable != VIR_TRISTATE_BOOL_ABSENT) {
Ironically, because the <= exists in the Parse code, that means that if
->gl is true, then we don't have to check ABSENT I would think.
I also see other/future patches seem to use enable == YES a lot...
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL_GL)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("OpenGL for SDL is not supported with this QEMU "
[...]
> @@ -8122,10 +8127,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> virBufferAddLit(&opt, "seamless-migration=on,");
> }
>
> + /* OpenGL magic */
> + if (graphics->gl &&
> + graphics->gl->enable == VIR_TRISTATE_BOOL_YES &&
> + qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, &opt, qemuCaps) < 0)
> + goto error;
> +
Not that it matters, but this is a change of order w/
"seamless-migration=on,"... No code uses both now and I don't imagine
order matters.
With recommended adjustments,
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
> virBufferTrim(&opt, ",", -1);
>
> virCommandAddArg(cmd, "-spice");
> virCommandAddArgBuffer(cmd, &opt);
> +
> if (graphics->data.spice.keymap)
> virCommandAddArgList(cmd, "-k",
> graphics->data.spice.keymap, NULL);
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
