Decrease size of qemuBuildGraphicsCommandLine() by splitting out spice-related code into qemuBuildGraphicsSPICECommandLine(). This patch also fixes 2 possible memory leaks on error path in the code that was split-out. The buffer containing the already generated options and a listen address string could be leaked. Also break a few very long lines. --- src/qemu/qemu_command.c | 336 +++++++++++++++++++++++++----------------------- 1 file changed, 176 insertions(+), 160 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 05c12b2..d6d782c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5417,6 +5417,181 @@ cleanup: return ret; } + +static int +qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, + virCommandPtr cmd, + virQEMUCapsPtr qemuCaps, + virDomainGraphicsDefPtr graphics) +{ + virBuffer opt = VIR_BUFFER_INITIALIZER; + const char *listenNetwork; + const char *listenAddr = NULL; + char *netAddr = NULL; + int ret; + int defaultMode = graphics->data.spice.defaultMode; + int port = graphics->data.spice.port; + int tlsPort = graphics->data.spice.tlsPort; + int i; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("spice graphics are not supported with this QEMU")); + goto error; + } + + if (port > 0 || tlsPort <= 0) + virBufferAsprintf(&opt, "port=%u", port); + + if (tlsPort > 0) { + if (!cfg->spiceTLS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("spice TLS port set in XML configuration," + " but TLS is disabled in qemu.conf")); + goto error; + } + if (port > 0) + virBufferAddChar(&opt, ','); + virBufferAsprintf(&opt, "tls-port=%u", tlsPort); + } + + switch (virDomainGraphicsListenGetType(graphics, 0)) { + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS: + listenAddr = virDomainGraphicsListenGetAddress(graphics, 0); + break; + + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK: + listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); + if (!listenNetwork) + break; + ret = networkGetNetworkAddress(listenNetwork, &netAddr); + if (ret <= -2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("network-based listen not possible, " + "network driver not present")); + goto error; + } + if (ret < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("listen network '%s' had no usable address"), + listenNetwork); + goto error; + } + listenAddr = netAddr; + /* store the address we found in the <graphics> element so it will + * show up in status. */ + if (virDomainGraphicsListenSetAddress(graphics, 0, + listenAddr, -1, false) < 0) + goto error; + break; + } + + if (!listenAddr) + listenAddr = cfg->spiceListen; + if (listenAddr) + virBufferAsprintf(&opt, ",addr=%s", listenAddr); + + VIR_FREE(netAddr); + + if (graphics->data.spice.mousemode) { + switch (graphics->data.spice.mousemode) { + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER: + virBufferAsprintf(&opt, ",agent-mouse=off"); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: + virBufferAsprintf(&opt, ",agent-mouse=on"); + break; + default: + break; + } + } + + /* In the password case we set it via monitor command, to avoid + * making it visible on CLI, so there's no use of password=XXX + * in this bit of the code */ + if (!graphics->data.spice.auth.passwd && + !cfg->spicePassword) + virBufferAddLit(&opt, ",disable-ticketing"); + + if (cfg->spiceTLS) + virBufferAsprintf(&opt, ",x509-dir=%s", + cfg->spiceTLSx509certdir); + + switch (defaultMode) { + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + virBufferAsprintf(&opt, ",tls-channel=default"); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + virBufferAsprintf(&opt, ",plaintext-channel=default"); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: + /* nothing */ + break; + } + + for (i = 0 ; i < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST ; i++) { + int mode = graphics->data.spice.channels[i]; + switch (mode) { + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + if (!cfg->spiceTLS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("spice secure channels set in XML configuration, " + "but TLS is disabled in qemu.conf")); + goto error; + } + virBufferAsprintf(&opt, ",tls-channel=%s", + virDomainGraphicsSpiceChannelNameTypeToString(i)); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + virBufferAsprintf(&opt, ",plaintext-channel=%s", + virDomainGraphicsSpiceChannelNameTypeToString(i)); + break; + } + } + if (graphics->data.spice.image) + virBufferAsprintf(&opt, ",image-compression=%s", + virDomainGraphicsSpiceImageCompressionTypeToString(graphics->data.spice.image)); + if (graphics->data.spice.jpeg) + virBufferAsprintf(&opt, ",jpeg-wan-compression=%s", + virDomainGraphicsSpiceJpegCompressionTypeToString(graphics->data.spice.jpeg)); + if (graphics->data.spice.zlib) + virBufferAsprintf(&opt, ",zlib-glz-wan-compression=%s", + virDomainGraphicsSpiceZlibCompressionTypeToString(graphics->data.spice.zlib)); + if (graphics->data.spice.playback) + virBufferAsprintf(&opt, ",playback-compression=%s", + virDomainGraphicsSpicePlaybackCompressionTypeToString(graphics->data.spice.playback)); + if (graphics->data.spice.streaming) + virBufferAsprintf(&opt, ",streaming-video=%s", + virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming)); + if (graphics->data.spice.copypaste == VIR_DOMAIN_GRAPHICS_SPICE_CLIPBOARD_COPYPASTE_NO) + virBufferAddLit(&opt, ",disable-copy-paste"); + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { + /* If qemu supports seamless migration turn it + * unconditionally on. If migration destination + * doesn't support it, it fallbacks to previous + * migration algorithm silently. */ + virBufferAddLit(&opt, ",seamless-migration=on"); + } + + virCommandAddArg(cmd, "-spice"); + virCommandAddArgBuffer(cmd, &opt); + if (graphics->data.spice.keymap) + virCommandAddArgList(cmd, "-k", + graphics->data.spice.keymap, NULL); + /* SPICE includes native support for tunnelling audio, so we + * set the audio backend to point at SPICE's own driver + */ + virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=spice"); + + return 0; + +error: + VIR_FREE(netAddr); + virBufferFreeAndReset(&opt); + return -1; +} + static int qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virCommandPtr cmd, @@ -5424,8 +5599,6 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virQEMUCapsPtr qemuCaps, virDomainGraphicsDefPtr graphics) { - int i; - if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { virBuffer opt = VIR_BUFFER_INITIALIZER; @@ -5577,165 +5750,8 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virCommandAddArg(cmd, "-sdl"); } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - virBuffer opt = VIR_BUFFER_INITIALIZER; - const char *listenNetwork; - const char *listenAddr = NULL; - char *netAddr = NULL; - int ret; - int defaultMode = graphics->data.spice.defaultMode; - int port = graphics->data.spice.port; - int tlsPort = graphics->data.spice.tlsPort; - - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("spice graphics are not supported with this QEMU")); + if (qemuBuildGraphicsSPICECommandLine(cfg, cmd, qemuCaps, graphics) < 0) goto error; - } - - if (port > 0 || tlsPort <= 0) - virBufferAsprintf(&opt, "port=%u", port); - - if (tlsPort > 0) { - if (!cfg->spiceTLS) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("spice TLS port set in XML configuration," - " but TLS is disabled in qemu.conf")); - goto error; - } - if (port > 0) - virBufferAddChar(&opt, ','); - virBufferAsprintf(&opt, "tls-port=%u", tlsPort); - } - - switch (virDomainGraphicsListenGetType(graphics, 0)) { - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS: - listenAddr = virDomainGraphicsListenGetAddress(graphics, 0); - break; - - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK: - listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); - if (!listenNetwork) - break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); - if (ret <= -2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("network-based listen not possible, " - "network driver not present")); - goto error; - } - if (ret < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("listen network '%s' had no usable address"), - listenNetwork); - goto error; - } - listenAddr = netAddr; - /* store the address we found in the <graphics> element so it will - * show up in status. */ - if (virDomainGraphicsListenSetAddress(graphics, 0, - listenAddr, -1, false) < 0) - goto error; - break; - } - - if (!listenAddr) - listenAddr = cfg->spiceListen; - if (listenAddr) - virBufferAsprintf(&opt, ",addr=%s", listenAddr); - - VIR_FREE(netAddr); - - int mm = graphics->data.spice.mousemode; - if (mm) { - switch (mm) { - case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER: - virBufferAsprintf(&opt, ",agent-mouse=off"); - break; - case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: - virBufferAsprintf(&opt, ",agent-mouse=on"); - break; - default: - break; - } - } - - /* In the password case we set it via monitor command, to avoid - * making it visible on CLI, so there's no use of password=XXX - * in this bit of the code */ - if (!graphics->data.spice.auth.passwd && - !cfg->spicePassword) - virBufferAddLit(&opt, ",disable-ticketing"); - - if (cfg->spiceTLS) - virBufferAsprintf(&opt, ",x509-dir=%s", - cfg->spiceTLSx509certdir); - - switch (defaultMode) { - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - virBufferAsprintf(&opt, ",tls-channel=default"); - break; - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - virBufferAsprintf(&opt, ",plaintext-channel=default"); - break; - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: - /* nothing */ - break; - } - - for (i = 0 ; i < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST ; i++) { - int mode = graphics->data.spice.channels[i]; - switch (mode) { - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - if (!cfg->spiceTLS) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("spice secure channels set in XML configuration, but TLS is disabled in qemu.conf")); - goto error; - } - virBufferAsprintf(&opt, ",tls-channel=%s", - virDomainGraphicsSpiceChannelNameTypeToString(i)); - break; - case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - virBufferAsprintf(&opt, ",plaintext-channel=%s", - virDomainGraphicsSpiceChannelNameTypeToString(i)); - break; - } - } - if (graphics->data.spice.image) - virBufferAsprintf(&opt, ",image-compression=%s", - virDomainGraphicsSpiceImageCompressionTypeToString(graphics->data.spice.image)); - if (graphics->data.spice.jpeg) - virBufferAsprintf(&opt, ",jpeg-wan-compression=%s", - virDomainGraphicsSpiceJpegCompressionTypeToString(graphics->data.spice.jpeg)); - if (graphics->data.spice.zlib) - virBufferAsprintf(&opt, ",zlib-glz-wan-compression=%s", - virDomainGraphicsSpiceZlibCompressionTypeToString(graphics->data.spice.zlib)); - if (graphics->data.spice.playback) - virBufferAsprintf(&opt, ",playback-compression=%s", - virDomainGraphicsSpicePlaybackCompressionTypeToString(graphics->data.spice.playback)); - if (graphics->data.spice.streaming) - virBufferAsprintf(&opt, ",streaming-video=%s", - virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming)); - if (graphics->data.spice.copypaste == VIR_DOMAIN_GRAPHICS_SPICE_CLIPBOARD_COPYPASTE_NO) - virBufferAddLit(&opt, ",disable-copy-paste"); - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { - /* If qemu supports seamless migration turn it - * unconditionally on. If migration destination - * doesn't support it, it fallbacks to previous - * migration algorithm silently. */ - virBufferAddLit(&opt, ",seamless-migration=on"); - } - - virCommandAddArg(cmd, "-spice"); - virCommandAddArgBuffer(cmd, &opt); - if (graphics->data.spice.keymap) - virCommandAddArgList(cmd, "-k", - graphics->data.spice.keymap, NULL); - /* SPICE includes native support for tunnelling audio, so we - * set the audio backend to point at SPICE's own driver - */ - virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=spice"); - } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported graphics type '%s'"), -- 1.8.2.1

Decrease size of qemuBuildGraphicsCommandLine() by splitting out spice-related code into qemuBuildGraphicsVNCCommandLine(). This patch also fixes 2 possible memory leaks on error path in the code that was split-out. The buffer containing the already generated options and a listen address string could be leaked. Also break a few very long lines and reflow code that fits now. --- src/qemu/qemu_command.c | 244 +++++++++++++++++++++++++----------------------- 1 file changed, 125 insertions(+), 119 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6d782c..66b2ba8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5419,6 +5419,130 @@ cleanup: static int +qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, + virCommandPtr cmd, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainGraphicsDefPtr graphics) +{ + virBuffer opt = VIR_BUFFER_INITIALIZER; + const char *listenNetwork; + const char *listenAddr = NULL; + char *netAddr = NULL; + bool escapeAddr; + int ret; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("vnc graphics are not supported with this QEMU")); + goto error; + } + + if (graphics->data.vnc.socket || cfg->vncAutoUnixSocket) { + if (!graphics->data.vnc.socket && + virAsprintf(&graphics->data.vnc.socket, + "%s/%s.vnc", cfg->libDir, def->name) == -1) + goto no_memory; + + virBufferAsprintf(&opt, "unix:%s", graphics->data.vnc.socket); + + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) { + switch (virDomainGraphicsListenGetType(graphics, 0)) { + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS: + listenAddr = virDomainGraphicsListenGetAddress(graphics, 0); + break; + + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK: + listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); + if (!listenNetwork) + break; + ret = networkGetNetworkAddress(listenNetwork, &netAddr); + if (ret <= -2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("network-based listen not possible, " + "network driver not present")); + goto error; + } + if (ret < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("listen network '%s' had no usable address"), + listenNetwork); + goto error; + } + listenAddr = netAddr; + /* store the address we found in the <graphics> element so it will + * show up in status. */ + if (virDomainGraphicsListenSetAddress(graphics, 0, + listenAddr, -1, false) < 0) + goto error; + break; + } + + if (!listenAddr) + listenAddr = cfg->vncListen; + + escapeAddr = strchr(listenAddr, ':') != NULL; + if (escapeAddr) + virBufferAsprintf(&opt, "[%s]", listenAddr); + else + virBufferAdd(&opt, listenAddr, -1); + virBufferAsprintf(&opt, ":%d", + graphics->data.vnc.port - 5900); + + VIR_FREE(netAddr); + } else { + virBufferAsprintf(&opt, "%d", + graphics->data.vnc.port - 5900); + } + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) { + if (graphics->data.vnc.auth.passwd || cfg->vncPassword) + virBufferAddLit(&opt, ",password"); + + if (cfg->vncTLS) { + virBufferAddLit(&opt, ",tls"); + if (cfg->vncTLSx509verify) + virBufferAsprintf(&opt, ",x509verify=%s", cfg->vncTLSx509certdir); + else + virBufferAsprintf(&opt, ",x509=%s", cfg->vncTLSx509certdir); + } + + if (cfg->vncSASL) { + virBufferAddLit(&opt, ",sasl"); + + if (cfg->vncSASLdir) + virCommandAddEnvPair(cmd, "SASL_CONF_DIR", cfg->vncSASLdir); + + /* TODO: Support ACLs later */ + } + } + + virCommandAddArg(cmd, "-vnc"); + virCommandAddArgBuffer(cmd, &opt); + if (graphics->data.vnc.keymap) + virCommandAddArgList(cmd, "-k", graphics->data.vnc.keymap, NULL); + + /* Unless user requested it, set the audio backend to none, to + * prevent it opening the host OS audio devices, since that causes + * security issues and might not work when using VNC. + */ + if (cfg->vncAllowHostAudio) + virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV"); + else + virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); + + return 0; + +no_memory: + virReportOOMError(); +error: + VIR_FREE(netAddr); + virBufferFreeAndReset(&opt); + return -1; +} + + +static int qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, virCommandPtr cmd, virQEMUCapsPtr qemuCaps, @@ -5600,124 +5724,8 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virDomainGraphicsDefPtr graphics) { if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - virBuffer opt = VIR_BUFFER_INITIALIZER; - - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("vnc graphics are not supported with this QEMU")); + if (qemuBuildGraphicsVNCCommandLine(cfg, cmd, def, qemuCaps, graphics) < 0) goto error; - } - - if (graphics->data.vnc.socket || - cfg->vncAutoUnixSocket) { - - if (!graphics->data.vnc.socket && - virAsprintf(&graphics->data.vnc.socket, - "%s/%s.vnc", cfg->libDir, def->name) == -1) { - goto no_memory; - } - - virBufferAsprintf(&opt, "unix:%s", - graphics->data.vnc.socket); - - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) { - const char *listenNetwork; - const char *listenAddr = NULL; - char *netAddr = NULL; - bool escapeAddr; - int ret; - - switch (virDomainGraphicsListenGetType(graphics, 0)) { - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS: - listenAddr = virDomainGraphicsListenGetAddress(graphics, 0); - break; - - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK: - listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); - if (!listenNetwork) - break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); - if (ret <= -2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("network-based listen not possible, " - "network driver not present")); - goto error; - } - if (ret < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("listen network '%s' had no usable address"), - listenNetwork); - goto error; - } - listenAddr = netAddr; - /* store the address we found in the <graphics> element so it will - * show up in status. */ - if (virDomainGraphicsListenSetAddress(graphics, 0, - listenAddr, -1, false) < 0) - goto error; - break; - } - - if (!listenAddr) - listenAddr = cfg->vncListen; - - escapeAddr = strchr(listenAddr, ':') != NULL; - if (escapeAddr) - virBufferAsprintf(&opt, "[%s]", listenAddr); - else - virBufferAdd(&opt, listenAddr, -1); - virBufferAsprintf(&opt, ":%d", - graphics->data.vnc.port - 5900); - - VIR_FREE(netAddr); - } else { - virBufferAsprintf(&opt, "%d", - graphics->data.vnc.port - 5900); - } - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC_COLON)) { - if (graphics->data.vnc.auth.passwd || - cfg->vncPassword) - virBufferAddLit(&opt, ",password"); - - if (cfg->vncTLS) { - virBufferAddLit(&opt, ",tls"); - if (cfg->vncTLSx509verify) { - virBufferAsprintf(&opt, ",x509verify=%s", - cfg->vncTLSx509certdir); - } else { - virBufferAsprintf(&opt, ",x509=%s", - cfg->vncTLSx509certdir); - } - } - - if (cfg->vncSASL) { - virBufferAddLit(&opt, ",sasl"); - - if (cfg->vncSASLdir) - virCommandAddEnvPair(cmd, "SASL_CONF_DIR", - cfg->vncSASLdir); - - /* TODO: Support ACLs later */ - } - } - - virCommandAddArg(cmd, "-vnc"); - virCommandAddArgBuffer(cmd, &opt); - if (graphics->data.vnc.keymap) { - virCommandAddArgList(cmd, "-k", graphics->data.vnc.keymap, - NULL); - } - - /* Unless user requested it, set the audio backend to none, to - * prevent it opening the host OS audio devices, since that causes - * security issues and might not work when using VNC. - */ - if (cfg->vncAllowHostAudio) { - virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV"); - } else { - virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); - } } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_0_10) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL)) { @@ -5761,8 +5769,6 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, return 0; -no_memory: - virReportOOMError(); error: return -1; } -- 1.8.2.1

On Tue, Apr 23, 2013 at 03:46:09PM +0200, Peter Krempa wrote: I guess you mean "VNC-related code" here. Christophe

Switch the function from a bunch of ifs to a switch statement with correct type and reflow some code. Also fix comment in enum describing possible graphics types --- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 37 ++++++++++++++++++------------------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f1f01fa..5b069b6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1230,7 +1230,7 @@ struct _virDomainVideoDef { virDomainDeviceInfo info; }; -/* 3 possible graphics console modes */ +/* graphics console modes */ enum virDomainGraphicsType { VIR_DOMAIN_GRAPHICS_TYPE_SDL, VIR_DOMAIN_GRAPHICS_TYPE_VNC, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 66b2ba8..ed8e73e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5723,24 +5723,19 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, virQEMUCapsPtr qemuCaps, virDomainGraphicsDefPtr graphics) { - if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - if (qemuBuildGraphicsVNCCommandLine(cfg, cmd, def, qemuCaps, graphics) < 0) - goto error; - } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { + switch ((enum virDomainGraphicsType) graphics->type) { + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_0_10) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("sdl not supported by '%s'"), - def->emulator); - goto error; + _("sdl not supported by '%s'"), def->emulator); + return -1; } if (graphics->data.sdl.xauth) - virCommandAddEnvPair(cmd, "XAUTHORITY", - graphics->data.sdl.xauth); + virCommandAddEnvPair(cmd, "XAUTHORITY", graphics->data.sdl.xauth); if (graphics->data.sdl.display) - virCommandAddEnvPair(cmd, "DISPLAY", - graphics->data.sdl.display); + virCommandAddEnvPair(cmd, "DISPLAY", graphics->data.sdl.display); if (graphics->data.sdl.fullscreen) virCommandAddArg(cmd, "-full-screen"); @@ -5757,20 +5752,24 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL)) virCommandAddArg(cmd, "-sdl"); - } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - if (qemuBuildGraphicsSPICECommandLine(cfg, cmd, qemuCaps, graphics) < 0) - goto error; - } else { + break; + + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: + return qemuBuildGraphicsVNCCommandLine(cfg, cmd, def, qemuCaps, graphics); + + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + return qemuBuildGraphicsSPICECommandLine(cfg, cmd, qemuCaps, graphics); + + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported graphics type '%s'"), virDomainGraphicsTypeToString(graphics->type)); - goto error; + return -1; } return 0; - -error: - return -1; } /* -- 1.8.2.1

Later on this function will be used to do more sophisticated checks and determination if port allocation is needed. --- src/qemu/qemu_process.c | 79 ++++++++++++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4988d9b..20978e0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3269,6 +3269,53 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) return ret; } + +static int +qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virDomainGraphicsDefPtr graphics) +{ + int ret = -1; + unsigned short port = 0; + unsigned short tlsPort; + + if (graphics->data.spice.autoport || + graphics->data.spice.port == -1) { + if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) + goto cleanup; + + if (port == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to find an unused port for SPICE")); + goto cleanup; + } + + graphics->data.spice.port = port; + } + + if (cfg->spiceTLS && + (graphics->data.spice.autoport || + graphics->data.spice.tlsPort == -1)) { + if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0) + goto cleanup; + + if (tlsPort == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unable to find an unused port for SPICE TLS")); + virPortAllocatorRelease(driver->remotePorts, port); + goto cleanup; + } + + graphics->data.spice.tlsPort = tlsPort; + } + + ret = 0; + +cleanup: + return ret; +} + + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3403,36 +3450,8 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; graphics->data.vnc.port = port; } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - unsigned short port = 0; - if (graphics->data.spice.autoport || - graphics->data.spice.port == -1) { - if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) - goto cleanup; - - if (port == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused port for SPICE")); - goto cleanup; - } - - graphics->data.spice.port = port; - } - if (cfg->spiceTLS && - (graphics->data.spice.autoport || - graphics->data.spice.tlsPort == -1)) { - unsigned short tlsPort; - if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0) - goto cleanup; - - if (tlsPort == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused port for SPICE TLS")); - virPortAllocatorRelease(driver->remotePorts, port); - goto cleanup; - } - - graphics->data.spice.tlsPort = tlsPort; - } + if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics) < 0) + goto cleanup; } if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC || -- 1.8.2.1

On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote: I'm not clear why this is safe. The idea is that if the user sends XML <graphics port='-1' tlsPort='-1'/> then libvirt would turn it into <graphics port='-1' tlsPort='-1' autoport='yes'/> with this removed, won't we be instead outputting <graphics port='-1' tlsPort='-1' autoport='no'/> despite the fact that it is auto-allocating the ports? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Tue, Apr 23, 2013 at 08:57:10PM +0200, Peter Krempa wrote: That is certainly not right. If we're allocating ports then we *must* be setting autoport='yes'. Having port='1' and tlsPort='-1' and autoport='no' is a non-sensical configuration. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Apr 24, 2013 at 10:38:26AM +0200, Peter Krempa wrote: Sure, we don't need to allocate both ports, if TLS is disabled in libvirtd, or if the configuration does not otherwise require it Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 04/23/2013 07:46 AM, Peter Krempa wrote: s/are needed to be alocated/need allocation/ ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Improve error reporting and generating of SPICE command line arguments according to the need to enable TLS. If TLS is disabled, there's no need to pass the certificate dir to qemu. This patch resolves: https://bugzilla.redhat.com/show_bug.cgi?id=953126 --- src/qemu/qemu_command.c | 45 ++++++++++++++++++---- .../qemuxml2argv-controller-order.args | 2 +- .../qemuxml2argv-graphics-spice-timeout.args | 2 +- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ed8e73e..dbb0892 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5637,9 +5637,8 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, !cfg->spicePassword) virBufferAddLit(&opt, ",disable-ticketing"); - if (cfg->spiceTLS) - virBufferAsprintf(&opt, ",x509-dir=%s", - cfg->spiceTLSx509certdir); + if (tlsPort > 0) + virBufferAsprintf(&opt, ",x509-dir=%s", cfg->spiceTLSx509certdir); switch (defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: @@ -5654,24 +5653,56 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } for (i = 0 ; i < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST ; i++) { - int mode = graphics->data.spice.channels[i]; - switch (mode) { + switch (graphics->data.spice.channels[i]) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - if (!cfg->spiceTLS) { + if (tlsPort <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("spice secure channels set in XML configuration, " - "but TLS is disabled in qemu.conf")); + "but TLS port is not provided")); goto error; } virBufferAsprintf(&opt, ",tls-channel=%s", virDomainGraphicsSpiceChannelNameTypeToString(i)); break; + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + if (port <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("spice insecure channels set in XML " + "configuration, but plain port is not provided")); + goto error; + } virBufferAsprintf(&opt, ",plaintext-channel=%s", virDomainGraphicsSpiceChannelNameTypeToString(i)); break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: + switch (defaultMode) { + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + if (tlsPort <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("spice defaultMode secure requested in XML " + "configuration but TLS port not provided")); + goto error; + } + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + if (port <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("spice defaultMode insecure requested in XML " + "configuration but plain port not provided")); + goto error; + } + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: + /* don't care */ + break; + } } } + if (graphics->data.spice.image) virBufferAsprintf(&opt, ",image-compression=%s", virDomainGraphicsSpiceImageCompressionTypeToString(graphics->data.spice.image)); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args index ec70c87..e6ed47f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args @@ -21,7 +21,7 @@ isa-serial,chardev=charserial0,id=serial0 -chardev \ spicevmc,id=charchannel0,name=vdagent -device \ virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 \ -device usb-tablet,id=input0 -spice \ -port=0,addr=0.0.0.0,x509-dir=/etc/pki/libvirt-spice -device \ +port=0,addr=0.0.0.0 -device \ intel-hda,id=sound0,bus=pci.0,addr=0x4 -device \ hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device \ usb-host,hostbus=14,hostaddr=6,id=hostdev0 -device \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args index 397ee4d..9df0eb1 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-timeout.args @@ -10,6 +10,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ -device rtl8139,vlan=0,id=net0,mac=52:54:00:71:70:89,bus=pci.0,addr=0x7 \ -net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 -serial pty \ -device usb-tablet,id=input0 \ --spice port=5900,x509-dir=/etc/pki/libvirt-spice -vga std \ +-spice port=5900 -vga std \ -device AC97,id=sound0,bus=pci.0,addr=0x3 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 -- 1.8.2.1