Re: [libvirt] [PATCH v1 06/11] conf: Make graphics's GL a standalone structure

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@redhat.com> --- 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@redhat.com> John

virBufferTrim(&opt, ",", -1);

virCommandAddArg(cmd, "-spice"); virCommandAddArgBuffer(cmd, &opt); + if (graphics->data.spice.keymap) virCommandAddArgList(cmd, "-k", graphics->data.spice.keymap, NULL);

[...]