[libvirt] [PATCH 0/7] SPICE autoport improvements
This series improves handling of automatic port allocation for spice consoles. Peter Krempa (7): qemu: Split out code to generate SPICE command line qemu: Split out code to generate VNC command line qemu: Use switch instead of ifs in qemuBuildGraphicsCommandLine qemu: Split out SPICE port allocation into a separate function conf: spice: Do more automation if autoport is requested qemu: Do sensible auto allocation of SPICE port numbers qemu: Improve handling of channels when generating SPICE command line docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 - src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 582 +++++++++++---------- src/qemu/qemu_process.c | 117 +++-- .../qemuxml2argv-controller-order.args | 2 +- .../qemuxml2argv-graphics-spice-timeout.args | 2 +- 7 files changed, 408 insertions(+), 304 deletions(-) -- 1.8.2.1
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
On Tue, Apr 23, 2013 at 03:46:08PM +0200, Peter Krempa wrote:
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(-)
ACK 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 :|
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:
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(-)
ACK 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 03:46:09PM +0200, Peter Krempa wrote:
Decrease size of qemuBuildGraphicsCommandLine() by splitting out spice-related code into qemuBuildGraphicsVNCCommandLine().
I guess you mean "VNC-related code" here. Christophe
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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/25/13 12:34, Christophe Fergeau wrote:
On Tue, Apr 23, 2013 at 03:46:09PM +0200, Peter Krempa wrote:
Decrease size of qemuBuildGraphicsCommandLine() by splitting out spice-related code into qemuBuildGraphicsVNCCommandLine().
I guess you mean "VNC-related code" here.
Ah, yes. I borrowed the commit message from the patch moving the spice code and forgot to update that. Unfortunately I already pushed this patch.
Christophe
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(-)
Peter
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
On Tue, Apr 23, 2013 at 03:46:10PM +0200, Peter Krempa wrote:
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(-)
ACK 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 :|
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:11PM +0200, Peter Krempa wrote:
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(-)
ACK Danie -- |: 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/13 18:16, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 03:46:11PM +0200, Peter Krempa wrote:
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(-)
ACK
Danie
Thanks for the reviews. I've pushed patches 1-4. Peter
With autoport enabled, both ports were alocated. With enabling defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..bb75943 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null while <code>tlsPort</code> gives an alternative secure port number. The <code>autoport</code> attribute is the new preferred syntax for indicating autoallocation of - both port numbers. The <code>listen</code> attribute is + needed port numbers. The <code>listen</code> attribute is an IP address for the server to listen on. The <code>passwd</code> attribute provides a SPICE password in clear text. The <code>keymap</code> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc0ecaa..86a444c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7595,11 +7595,6 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(defaultMode); } - if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { - /* Legacy compat syntax, used -1 for auto-port */ - def->data.spice.autoport = true; - } - if (def->data.spice.autoport && (flags & VIR_DOMAIN_XML_INACTIVE)) { def->data.spice.port = 0; def->data.spice.tlsPort = 0; -- 1.8.2.1
On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote:
With autoport enabled, both ports were alocated. With enabling defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..bb75943 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null while <code>tlsPort</code> gives an alternative secure port number. The <code>autoport</code> attribute is the new preferred syntax for indicating autoallocation of - both port numbers. The <code>listen</code> attribute is + needed port numbers. The <code>listen</code> attribute is an IP address for the server to listen on. The <code>passwd</code> attribute provides a SPICE password in clear text. The <code>keymap</code> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc0ecaa..86a444c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7595,11 +7595,6 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(defaultMode); }
- if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { - /* Legacy compat syntax, used -1 for auto-port */ - def->data.spice.autoport = true; - }
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 04/23/13 18:21, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote:
With autoport enabled, both ports were alocated. With enabling defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..bb75943 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null while <code>tlsPort</code> gives an alternative secure port number. The <code>autoport</code> attribute is the new preferred syntax for indicating autoallocation of - both port numbers. The <code>listen</code> attribute is + needed port numbers. The <code>listen</code> attribute is an IP address for the server to listen on. The <code>passwd</code> attribute provides a SPICE password in clear text. The <code>keymap</code> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc0ecaa..86a444c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7595,11 +7595,6 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(defaultMode); }
- if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { - /* Legacy compat syntax, used -1 for auto-port */ - def->data.spice.autoport = true; - }
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?
Later on this will slightly change semantics: <graphics port='-1' tlsPort='-1' autoport='no'/> Will allocate both ports every time, even if one isn't needed because of other configuration (eg defaultMode="insecure") whereas <graphics autoport='yes'/> will automatically determine which ports are needed according to configuration and allocate just those. (only the insecure port in case of the example above)
Daniel
Peter
On Tue, Apr 23, 2013 at 08:57:10PM +0200, Peter Krempa wrote:
On 04/23/13 18:21, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote:
With autoport enabled, both ports were alocated. With enabling defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..bb75943 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null while <code>tlsPort</code> gives an alternative secure port number. The <code>autoport</code> attribute is the new preferred syntax for indicating autoallocation of - both port numbers. The <code>listen</code> attribute is + needed port numbers. The <code>listen</code> attribute is an IP address for the server to listen on. The <code>passwd</code> attribute provides a SPICE password in clear text. The <code>keymap</code> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc0ecaa..86a444c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7595,11 +7595,6 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(defaultMode); }
- if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { - /* Legacy compat syntax, used -1 for auto-port */ - def->data.spice.autoport = true; - }
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?
Later on this will slightly change semantics:
<graphics port='-1' tlsPort='-1' autoport='no'/>
Will allocate both ports every time, even if one isn't needed because of other configuration (eg defaultMode="insecure")
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.
whereas
<graphics autoport='yes'/>
will automatically determine which ports are needed according to configuration and allocate just those. (only the insecure port in case of the example above)
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/24/13 10:19, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 08:57:10PM +0200, Peter Krempa wrote:
On 04/23/13 18:21, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote:
With autoport enabled, both ports were alocated. With enabling defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..bb75943 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null while <code>tlsPort</code> gives an alternative secure port number. The <code>autoport</code> attribute is the new preferred syntax for indicating autoallocation of - both port numbers. The <code>listen</code> attribute is + needed port numbers. The <code>listen</code> attribute is an IP address for the server to listen on. The <code>passwd</code> attribute provides a SPICE password in clear text. The <code>keymap</code> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc0ecaa..86a444c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7595,11 +7595,6 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(defaultMode); }
- if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { - /* Legacy compat syntax, used -1 for auto-port */ - def->data.spice.autoport = true; - }
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?
Later on this will slightly change semantics:
<graphics port='-1' tlsPort='-1' autoport='no'/>
Will allocate both ports every time, even if one isn't needed because of other configuration (eg defaultMode="insecure")
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.
Okay, that is fair enough. In that case, is it okay not to allocate both ports if the configuration doesn't require it even if we did so before? Or do we need to have an option to force allocation of both TLS and non-tls port even if it's not needed? Peter
On Wed, Apr 24, 2013 at 10:38:26AM +0200, Peter Krempa wrote:
On 04/24/13 10:19, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 08:57:10PM +0200, Peter Krempa wrote:
On 04/23/13 18:21, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote:
With autoport enabled, both ports were alocated. With enabling defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..bb75943 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null while <code>tlsPort</code> gives an alternative secure port number. The <code>autoport</code> attribute is the new preferred syntax for indicating autoallocation of - both port numbers. The <code>listen</code> attribute is + needed port numbers. The <code>listen</code> attribute is an IP address for the server to listen on. The <code>passwd</code> attribute provides a SPICE password in clear text. The <code>keymap</code> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dc0ecaa..86a444c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7595,11 +7595,6 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(defaultMode); }
- if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { - /* Legacy compat syntax, used -1 for auto-port */ - def->data.spice.autoport = true; - }
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?
Later on this will slightly change semantics:
<graphics port='-1' tlsPort='-1' autoport='no'/>
Will allocate both ports every time, even if one isn't needed because of other configuration (eg defaultMode="insecure")
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.
Okay, that is fair enough.
In that case, is it okay not to allocate both ports if the configuration doesn't require it even if we did so before? Or do we need to have an option to force allocation of both TLS and non-tls port even if it's not needed?
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/24/13 10:42, Daniel P. Berrange wrote:
On Wed, Apr 24, 2013 at 10:38:26AM +0200, Peter Krempa wrote:
On 04/24/13 10:19, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 08:57:10PM +0200, Peter Krempa wrote:
On 04/23/13 18:21, Daniel P. Berrange wrote:
On Tue, Apr 23, 2013 at 03:46:12PM +0200, Peter Krempa wrote:
With autoport enabled, both ports were alocated. With enabling defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..bb75943 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3470,7 +3470,7 @@ qemu-kvm -net nic,model=? /dev/null while <code>tlsPort</code> gives an alternative secure port number. The <code>autoport</code> attribute is the new preferred syntax for indicating autoallocation of - both port numbers. The <code>listen</code> attribute is + needed port numbers. The <code>listen</code> attribute is an IP address for the server to listen on. The <code>passwd</code> attribute provides a SPICE password in clear text. The <code>keymap</code>
...
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.
Okay, that is fair enough.
In that case, is it okay not to allocate both ports if the configuration doesn't require it even if we did so before? Or do we need to have an option to force allocation of both TLS and non-tls port even if it's not needed?
Sure, we don't need to allocate both ports, if TLS is disabled in libvirtd, or if the configuration does not otherwise require it
Great, in that case I'll incorporate the doc hunk from this patch into the next one in this series as that patch makes this change and I'll drop this one. Peter
On 04/23/2013 07:46 AM, Peter Krempa wrote:
With autoport enabled, both ports were alocated. With enabling
s/alocated/allocated/
defaultMode or setting separate channel modes one of the ports may not be needed. This will allow later on doing this kind of change. --- docs/formatdomain.html.in | 2 +- src/conf/domain_conf.c | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
With this patch, if the autoport attribute is used, the code will sensibly auto allocate the ports only if needed. --- src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 20978e0..db80626 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3275,44 +3275,82 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, virDomainGraphicsDefPtr graphics) { - int ret = -1; unsigned short port = 0; unsigned short tlsPort; + int i; + int defaultMode = graphics->data.spice.defaultMode; + + bool needTLSPort = false; + bool needPort = false; + + if (graphics->data.spice.autoport) { + /* check if tlsPort or port are needed to be alocated */ + 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: + needTLSPort = true; + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + needPort = true; + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: + switch (defaultMode) { + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + needTLSPort = true; + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + needPort = true; + break; + + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: + needTLSPort = true; + needPort = true; + break; + } + break; + } + } + } - if (graphics->data.spice.autoport || - graphics->data.spice.port == -1) { + if (needPort || graphics->data.spice.port == -1) { if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) - goto cleanup; + goto error; if (port == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find an unused port for SPICE")); - goto cleanup; + goto error; } graphics->data.spice.port = port; } if (cfg->spiceTLS && - (graphics->data.spice.autoport || - graphics->data.spice.tlsPort == -1)) { + (needTLSPort || graphics->data.spice.tlsPort == -1)) { if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0) - goto cleanup; + goto error; if (tlsPort == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused port for SPICE TLS")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to find an unused port for SPICE TLS")); virPortAllocatorRelease(driver->remotePorts, port); - goto cleanup; + goto error; } graphics->data.spice.tlsPort = tlsPort; } - ret = 0; + return 0; -cleanup: - return ret; +error: + if (port) + virPortAllocatorRelease(driver->remotePorts, port); + return -1; } -- 1.8.2.1
On 04/23/2013 07:46 AM, Peter Krempa wrote:
With this patch, if the autoport attribute is used, the code will sensibly auto allocate the ports only if needed. --- src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 52 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 20978e0..db80626 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3275,44 +3275,82 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, virDomainGraphicsDefPtr graphics) { - int ret = -1; unsigned short port = 0; unsigned short tlsPort; + int i; + int defaultMode = graphics->data.spice.defaultMode; + + bool needTLSPort = false; + bool needPort = false; + + if (graphics->data.spice.autoport) { + /* check if tlsPort or port are needed to be alocated */
s/are needed to be alocated/need allocation/ ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/24/13 01:07, Eric Blake wrote:
On 04/23/2013 07:46 AM, Peter Krempa wrote:
With this patch, if the autoport attribute is used, the code will sensibly auto allocate the ports only if needed. --- src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 52 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 20978e0..db80626 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3275,44 +3275,82 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, virDomainGraphicsDefPtr graphics) { - int ret = -1; unsigned short port = 0; unsigned short tlsPort; + int i; + int defaultMode = graphics->data.spice.defaultMode; + + bool needTLSPort = false; + bool needPort = false; + + if (graphics->data.spice.autoport) { + /* check if tlsPort or port are needed to be alocated */
s/are needed to be alocated/need allocation/
I fixed the wording/spelling you pointed out and squashed in the docs change from the previous patch that was dropped and pushed this patch. Thanks for the review. Peter
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
On 04/23/2013 07:46 AM, Peter Krempa wrote:
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(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/24/13 01:10, Eric Blake wrote:
On 04/23/2013 07:46 AM, Peter Krempa wrote:
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(-)
ACK.
Pushed, thanks! Peter
participants (4)
-
Christophe Fergeau -
Daniel P. Berrange -
Eric Blake -
Peter Krempa