[libvirt] [PATCH v2 0/3] Cosmetic changes to SPICE QEMU commandline
Hey, This is a v2 of the series I previously sent. Changes since v1: - much nicer cmdline generation in patch 1/3 - improved commit log in 1/3 - moved part of patch 1/3 to 3/3 - added test case in 3/3 Christophe
The end goal is to avoid adding -spice port=0,addr=127.0.0.1 to QEMU command line when no SPICE port is specified in libvirt XML. Currently, the code relies on port=xx to always be present, so subsequent args can be unconditionally appended with a leading ','. Since port=0 will no longer be added in a subsequent commit, we append a ',' to every arg instead of prepending, and remove the last one before adding it to the arg list. --- src/qemu/qemu_command.c | 68 +++++++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 32d32b1..cd20a16 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7030,7 +7030,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } if (port > 0 || tlsPort <= 0) - virBufferAsprintf(&opt, "port=%u", port); + virBufferAsprintf(&opt, "port=%u,", port); if (tlsPort > 0) { if (!cfg->spiceTLS) { @@ -7039,13 +7039,12 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, " but TLS is disabled in qemu.conf")); goto error; } - if (port > 0) - virBufferAddChar(&opt, ','); - virBufferAsprintf(&opt, "tls-port=%u", tlsPort); + + virBufferAsprintf(&opt, "tls-port=%u,", tlsPort); } if (cfg->spiceSASL) { - virBufferAddLit(&opt, ",sasl"); + virBufferAddLit(&opt, "sasl,"); if (cfg->spiceSASLdir) virCommandAddEnvPair(cmd, "SASL_CONF_PATH", @@ -7085,17 +7084,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, if (!listenAddr) listenAddr = cfg->spiceListen; if (listenAddr) - virBufferAsprintf(&opt, ",addr=%s", 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: - virBufferAddLit(&opt, ",agent-mouse=off"); + virBufferAddLit(&opt, "agent-mouse=off,"); break; case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: - virBufferAddLit(&opt, ",agent-mouse=on"); + virBufferAddLit(&opt, "agent-mouse=on,"); break; default: break; @@ -7106,18 +7105,19 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, * 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"); + !cfg->spicePassword) { + virBufferAddLit(&opt, "disable-ticketing,"); + } if (tlsPort > 0) - virBufferAsprintf(&opt, ",x509-dir=%s", cfg->spiceTLSx509certdir); + virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir); switch (defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - virBufferAddLit(&opt, ",tls-channel=default"); + virBufferAddLit(&opt, "tls-channel=default,"); break; case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - virBufferAddLit(&opt, ",plaintext-channel=default"); + virBufferAddLit(&opt, "plaintext-channel=default,"); break; case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: /* nothing */ @@ -7133,7 +7133,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, "but TLS port is not provided")); goto error; } - virBufferAsprintf(&opt, ",tls-channel=%s", + virBufferAsprintf(&opt, "tls-channel=%s,", virDomainGraphicsSpiceChannelNameTypeToString(i)); break; @@ -7144,7 +7144,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, "configuration, but plain port is not provided")); goto error; } - virBufferAsprintf(&opt, ",plaintext-channel=%s", + virBufferAsprintf(&opt, "plaintext-channel=%s,", virDomainGraphicsSpiceChannelNameTypeToString(i)); break; @@ -7175,30 +7175,36 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } } - if (graphics->data.spice.image) - virBufferAsprintf(&opt, ",image-compression=%s", + 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", + } + 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", + } + 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", + } + if (graphics->data.spice.playback) { + virBufferAsprintf(&opt, "playback-compression=%s,", virTristateSwitchTypeToString(graphics->data.spice.playback)); - if (graphics->data.spice.streaming) - virBufferAsprintf(&opt, ",streaming-video=%s", + } + if (graphics->data.spice.streaming) { + virBufferAsprintf(&opt, "streaming-video=%s,", virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming)); - if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) - virBufferAddLit(&opt, ",disable-copy-paste"); + } + if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) { + virBufferAddLit(&opt, "disable-copy-paste,"); + } if (graphics->data.spice.filetransfer == VIR_TRISTATE_BOOL_NO) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This QEMU can't disable file transfers through spice")); goto error; } else { - virBufferAddLit(&opt, ",disable-agent-file-xfer"); + virBufferAddLit(&opt, "disable-agent-file-xfer,"); } } @@ -7211,7 +7217,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, /* spice.gl is a TristateBool, but qemu expects on/off: use * TristateSwitch helper */ - virBufferAsprintf(&opt, ",gl=%s", + virBufferAsprintf(&opt, "gl=%s,", virTristateSwitchTypeToString(graphics->data.spice.gl)); } @@ -7220,9 +7226,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, * unconditionally on. If migration destination * doesn't support it, it fallbacks to previous * migration algorithm silently. */ - virBufferAddLit(&opt, ",seamless-migration=on"); + virBufferAddLit(&opt, "seamless-migration=on,"); } + virBufferTrim(&opt, ",", -1); + virCommandAddArg(cmd, "-spice"); virCommandAddArgBuffer(cmd, &opt); if (graphics->data.spice.keymap) -- 2.5.0
On Wed, Mar 16, 2016 at 05:45:03PM +0100, Christophe Fergeau wrote:
The end goal is to avoid adding -spice port=0,addr=127.0.0.1 to QEMU command line when no SPICE port is specified in libvirt XML.
Currently, the code relies on port=xx to always be present, so subsequent args can be unconditionally appended with a leading ','. Since port=0 will no longer be added in a subsequent commit, we append a ',' to every arg instead of prepending, and remove the last one before adding it to the arg list. --- src/qemu/qemu_command.c | 68 +++++++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 30 deletions(-)
ACK
- if (graphics->data.spice.playback) - virBufferAsprintf(&opt, ",playback-compression=%s", + } + if (graphics->data.spice.playback) { + virBufferAsprintf(&opt, "playback-compression=%s,", virTristateSwitchTypeToString(graphics->data.spice.playback)); - if (graphics->data.spice.streaming) - virBufferAsprintf(&opt, ",streaming-video=%s", + } + if (graphics->data.spice.streaming) { + virBufferAsprintf(&opt, "streaming-video=%s,", virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming)); - if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) - virBufferAddLit(&opt, ",disable-copy-paste"); + } + if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) { + virBufferAddLit(&opt, "disable-copy-paste,"); + }
This breaks make syntax-check: Curly brackets around single-line body: src/qemu/qemu_command.c:7559-7561: if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) { virBufferAddLit(&opt, "disable-copy-paste,"); } Jan
On Fri, Mar 18, 2016 at 10:25:58AM +0100, Ján Tomko wrote:
- if (graphics->data.spice.playback) - virBufferAsprintf(&opt, ",playback-compression=%s", + } + if (graphics->data.spice.playback) { + virBufferAsprintf(&opt, "playback-compression=%s,", virTristateSwitchTypeToString(graphics->data.spice.playback)); - if (graphics->data.spice.streaming) - virBufferAsprintf(&opt, ",streaming-video=%s", + } + if (graphics->data.spice.streaming) { + virBufferAsprintf(&opt, "streaming-video=%s,", virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming)); - if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) - virBufferAddLit(&opt, ",disable-copy-paste"); + } + if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) { + virBufferAddLit(&opt, "disable-copy-paste,"); + }
This breaks make syntax-check:
Curly brackets around single-line body: src/qemu/qemu_command.c:7559-7561: if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) { virBufferAddLit(&opt, "disable-copy-paste,"); }
Ah thanks, I'll make sure to fix these before pushing. Totally forgot to rerun it before sending v2. Christophe
On Fri, Mar 18, 2016 at 11:09:25AM +0100, Christophe Fergeau wrote:
On Fri, Mar 18, 2016 at 10:25:58AM +0100, Ján Tomko wrote:
This breaks make syntax-check:
Curly brackets around single-line body: src/qemu/qemu_command.c:7559-7561: if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) { virBufferAddLit(&opt, "disable-copy-paste,"); }
Ah thanks, I'll make sure to fix these before pushing. Totally forgot to rerun it before sending v2.
I've now pushed the series after fixing make syntax-check and the comment. Christophe
Hey, After this patch series, the QEMU command line may not contain port nor tls-port if they both were set to '0'. However, QEMU versions older than 2.3.0 will error out because they don't have this commit: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0 I assume we want to keep supporting older QEMU binaries, and that this needs to be fixed on libvirt side? Christophe On Wed, Mar 16, 2016 at 05:45:03PM +0100, Christophe Fergeau wrote:
The end goal is to avoid adding -spice port=0,addr=127.0.0.1 to QEMU command line when no SPICE port is specified in libvirt XML.
Currently, the code relies on port=xx to always be present, so subsequent args can be unconditionally appended with a leading ','. Since port=0 will no longer be added in a subsequent commit, we append a ',' to every arg instead of prepending, and remove the last one before adding it to the arg list. --- src/qemu/qemu_command.c | 68 +++++++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 32d32b1..cd20a16 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7030,7 +7030,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, }
if (port > 0 || tlsPort <= 0) - virBufferAsprintf(&opt, "port=%u", port); + virBufferAsprintf(&opt, "port=%u,", port);
if (tlsPort > 0) { if (!cfg->spiceTLS) { @@ -7039,13 +7039,12 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, " but TLS is disabled in qemu.conf")); goto error; } - if (port > 0) - virBufferAddChar(&opt, ','); - virBufferAsprintf(&opt, "tls-port=%u", tlsPort); + + virBufferAsprintf(&opt, "tls-port=%u,", tlsPort); }
if (cfg->spiceSASL) { - virBufferAddLit(&opt, ",sasl"); + virBufferAddLit(&opt, "sasl,");
if (cfg->spiceSASLdir) virCommandAddEnvPair(cmd, "SASL_CONF_PATH", @@ -7085,17 +7084,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, if (!listenAddr) listenAddr = cfg->spiceListen; if (listenAddr) - virBufferAsprintf(&opt, ",addr=%s", 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: - virBufferAddLit(&opt, ",agent-mouse=off"); + virBufferAddLit(&opt, "agent-mouse=off,"); break; case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT: - virBufferAddLit(&opt, ",agent-mouse=on"); + virBufferAddLit(&opt, "agent-mouse=on,"); break; default: break; @@ -7106,18 +7105,19 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, * 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"); + !cfg->spicePassword) { + virBufferAddLit(&opt, "disable-ticketing,"); + }
if (tlsPort > 0) - virBufferAsprintf(&opt, ",x509-dir=%s", cfg->spiceTLSx509certdir); + virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir);
switch (defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: - virBufferAddLit(&opt, ",tls-channel=default"); + virBufferAddLit(&opt, "tls-channel=default,"); break; case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: - virBufferAddLit(&opt, ",plaintext-channel=default"); + virBufferAddLit(&opt, "plaintext-channel=default,"); break; case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: /* nothing */ @@ -7133,7 +7133,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, "but TLS port is not provided")); goto error; } - virBufferAsprintf(&opt, ",tls-channel=%s", + virBufferAsprintf(&opt, "tls-channel=%s,", virDomainGraphicsSpiceChannelNameTypeToString(i)); break;
@@ -7144,7 +7144,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, "configuration, but plain port is not provided")); goto error; } - virBufferAsprintf(&opt, ",plaintext-channel=%s", + virBufferAsprintf(&opt, "plaintext-channel=%s,", virDomainGraphicsSpiceChannelNameTypeToString(i)); break;
@@ -7175,30 +7175,36 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, } }
- if (graphics->data.spice.image) - virBufferAsprintf(&opt, ",image-compression=%s", + 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", + } + 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", + } + 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", + } + if (graphics->data.spice.playback) { + virBufferAsprintf(&opt, "playback-compression=%s,", virTristateSwitchTypeToString(graphics->data.spice.playback)); - if (graphics->data.spice.streaming) - virBufferAsprintf(&opt, ",streaming-video=%s", + } + if (graphics->data.spice.streaming) { + virBufferAsprintf(&opt, "streaming-video=%s,", virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming)); - if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) - virBufferAddLit(&opt, ",disable-copy-paste"); + } + if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) { + virBufferAddLit(&opt, "disable-copy-paste,"); + } if (graphics->data.spice.filetransfer == VIR_TRISTATE_BOOL_NO) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This QEMU can't disable file transfers through spice")); goto error; } else { - virBufferAddLit(&opt, ",disable-agent-file-xfer"); + virBufferAddLit(&opt, "disable-agent-file-xfer,"); } }
@@ -7211,7 +7217,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
/* spice.gl is a TristateBool, but qemu expects on/off: use * TristateSwitch helper */ - virBufferAsprintf(&opt, ",gl=%s", + virBufferAsprintf(&opt, "gl=%s,", virTristateSwitchTypeToString(graphics->data.spice.gl)); }
@@ -7220,9 +7226,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, * unconditionally on. If migration destination * doesn't support it, it fallbacks to previous * migration algorithm silently. */ - virBufferAddLit(&opt, ",seamless-migration=on"); + virBufferAddLit(&opt, "seamless-migration=on,"); }
+ virBufferTrim(&opt, ",", -1); + virCommandAddArg(cmd, "-spice"); virCommandAddArgBuffer(cmd, &opt); if (graphics->data.spice.keymap) -- 2.5.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/17/2016 06:11 AM, Christophe Fergeau wrote:
Hey,
After this patch series, the QEMU command line may not contain port nor tls-port if they both were set to '0'. However, QEMU versions older than 2.3.0 will error out because they don't have this commit: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0
I assume we want to keep supporting older QEMU binaries, and that this needs to be fixed on libvirt side?
Yes we will want to keep older qemu working. However I think Pavel's patches address this issue? - Cole
On Tue, May 17, 2016 at 08:03:11AM -0400, Cole Robinson wrote:
On 05/17/2016 06:11 AM, Christophe Fergeau wrote:
Hey,
After this patch series, the QEMU command line may not contain port nor tls-port if they both were set to '0'. However, QEMU versions older than 2.3.0 will error out because they don't have this commit: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0
I assume we want to keep supporting older QEMU binaries, and that this needs to be fixed on libvirt side?
Yes we will want to keep older qemu working. However I think Pavel's patches address this issue?
Actually as Ján pointed out, older QEMU behaviour before/after these patches should be unchanged. Only difference is that before QEMU was always adding port=0/tls-port=0, after the are not present, but QEMU will default to 0 when they are missing. In both cases (present and set to 0, and not present and default to 0), we will be getting an error. I don't think Pavel's patches are going to help there though, but I don't think that's a problem. Christophe
On Tue, May 17, 2016 at 12:11:38PM +0200, Christophe Fergeau wrote:
Hey,
After this patch series, the QEMU command line may not contain port nor tls-port if they both were set to '0'. However, QEMU versions older than 2.3.0 will error out because they don't have this commit: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0
From that snippet of code it seems omitting port completely and setting it to 0 were equivalent.
I assume we want to keep supporting older QEMU binaries, and that this needs to be fixed on libvirt side?
It seems this never worked with older QEMUs, regardless of this series. Jan
On Tue, May 17, 2016 at 02:10:16PM +0200, Ján Tomko wrote:
On Tue, May 17, 2016 at 12:11:38PM +0200, Christophe Fergeau wrote:
Hey,
After this patch series, the QEMU command line may not contain port nor tls-port if they both were set to '0'. However, QEMU versions older than 2.3.0 will error out because they don't have this commit: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0
From that snippet of code it seems omitting port completely and setting it to 0 were equivalent.
I assume we want to keep supporting older QEMU binaries, and that this needs to be fixed on libvirt side?
It seems this never worked with older QEMUs, regardless of this series.
Ah good point, I did not pay enough attention to the commit ;) Christophe
Currently -spice addr=127.0.0.1 is generated, but spice-server is going to ignore this as no port is specified. --- src/qemu/qemu_command.c | 60 +++++++++++----------- .../qemuxml2argv-controller-order.args | 2 +- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cd20a16..bcc8cd6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7053,41 +7053,43 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, /* TODO: Support ACLs later */ } - switch (virDomainGraphicsListenGetType(graphics, 0)) { - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS: - listenAddr = virDomainGraphicsListenGetAddress(graphics, 0); - break; + if (port > 0 || tlsPort > 0) { + 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) + goto error; - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK: - listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); - if (!listenNetwork) + 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; - 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) - goto error; + if (!listenAddr) + listenAddr = cfg->spiceListen; + if (listenAddr) { + virBufferAsprintf(&opt, "addr=%s,", listenAddr); + } - 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; + VIR_FREE(netAddr); } - 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: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args index 89c7fd8..b47193a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-controller-order.args @@ -37,7 +37,7 @@ media=cdrom,id=drive-ide0-1-0 \ -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 \ +-spice port=0 \ -vga cirrus \ -device intel-hda,id=sound0,bus=pci.0,addr=0x4 \ -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 \ -- 2.5.0
On Wed, Mar 16, 2016 at 05:45:04PM +0100, Christophe Fergeau wrote:
Currently -spice addr=127.0.0.1 is generated, but spice-server is going to ignore this as no port is specified. --- src/qemu/qemu_command.c | 60 +++++++++++----------- .../qemuxml2argv-controller-order.args | 2 +- 2 files changed, 32 insertions(+), 30 deletions(-)
ACK
- goto error; + if (!listenAddr) + listenAddr = cfg->spiceListen; + if (listenAddr) { + virBufferAsprintf(&opt, "addr=%s,", listenAddr); + }
This also fails syntax check. Jan
If a <graphics type='spice'> has no port nor tlsPort set, the generated QEMU command line will contain -spice port=0. This is later going to be ignored by spice-server, but it's better not to add it at all in this situation. As an empty -spice is not allowed, we still need to append port=0 if we did not add any other argument. --- src/qemu/qemu_command.c | 11 +++++++++-- .../qemuxml2argv-graphics-spice-no-args.args | 21 +++++++++++++++++++++ .../qemuxml2argv-graphics-spice-no-args.xml | 22 ++++++++++++++++++++++ .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +- tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bcc8cd6..e69e873 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7029,7 +7029,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, goto error; } - if (port > 0 || tlsPort <= 0) + if (port > 0) virBufferAsprintf(&opt, "port=%u,", port); if (tlsPort > 0) { @@ -7234,7 +7234,14 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, virBufferTrim(&opt, ",", -1); virCommandAddArg(cmd, "-spice"); - virCommandAddArgBuffer(cmd, &opt); + /* If we did not add any SPICE arguments, add a dummy 'port=0' one + * as -spice alone is not allowed on QEMU command line and will be + * ignored by libvirt + */ + if (virBufferUse(&opt) == 0) + virCommandAddArg(cmd, "port=0"); + else + virCommandAddArgBuffer(cmd, &opt); if (graphics->data.spice.keymap) virCommandAddArgList(cmd, "-k", graphics->data.spice.keymap, NULL); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.args new file mode 100644 index 0000000..05ce743 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.args @@ -0,0 +1,21 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=spice \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-spice port=0 \ +-vga cirrus \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.xml new file mode 100644 index 0000000..9d49feb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.xml @@ -0,0 +1,22 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice' autoport='no'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args index edecca1..b80ad16 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args @@ -19,6 +19,6 @@ QEMU_AUDIO_DRV=spice \ -drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ id=drive-ide0-0-0,cache=none \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --spice port=0,gl=on \ +-spice gl=on \ -device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e15da37..4fac77d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -949,6 +949,8 @@ mymain(void) QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_SPICE_FILE_XFER_DISABLE); + DO_TEST("graphics-spice-no-args", + QEMU_CAPS_SPICE); driver.config->spiceSASL = 1; ignore_value(VIR_STRDUP(driver.config->spiceSASLdir, "/root/.sasl2")); DO_TEST("graphics-spice-sasl", -- 2.5.0
On Wed, Mar 16, 2016 at 05:45:05PM +0100, Christophe Fergeau wrote:
If a <graphics type='spice'> has no port nor tlsPort set, the generated QEMU command line will contain -spice port=0. This is later going to be ignored by spice-server, but it's better not to add it at all in this situation. As an empty -spice is not allowed, we still need to append port=0 if we did not add any other argument. --- src/qemu/qemu_command.c | 11 +++++++++-- .../qemuxml2argv-graphics-spice-no-args.args | 21 +++++++++++++++++++++ .../qemuxml2argv-graphics-spice-no-args.xml | 22 ++++++++++++++++++++++ .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +- tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-no-args.xml
ACK
@@ -7234,7 +7234,14 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, virBufferTrim(&opt, ",", -1);
virCommandAddArg(cmd, "-spice"); - virCommandAddArgBuffer(cmd, &opt); + /* If we did not add any SPICE arguments, add a dummy 'port=0' one + * as -spice alone is not allowed on QEMU command line and will be + * ignored by libvirt
I am confused by the "will be ignored by libvirt" part. Jan
+ */ + if (virBufferUse(&opt) == 0) + virCommandAddArg(cmd, "port=0"); + else + virCommandAddArgBuffer(cmd, &opt); if (graphics->data.spice.keymap)
On Fri, Mar 18, 2016 at 10:28:10AM +0100, Ján Tomko wrote:
@@ -7234,7 +7234,14 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, virBufferTrim(&opt, ",", -1);
virCommandAddArg(cmd, "-spice"); - virCommandAddArgBuffer(cmd, &opt); + /* If we did not add any SPICE arguments, add a dummy 'port=0' one + * as -spice alone is not allowed on QEMU command line and will be + * ignored by libvirt
I am confused by the "will be ignored by libvirt" part.
I'm confused too :) I'll drop this before pushing. Maybe I meant that 'port=0' is now ignored (not added) by libvirt, so we may end up with an empty -spice parameter list, so we need to make sure it's not empty. Better to drop this. Christophe
participants (3)
-
Christophe Fergeau -
Cole Robinson -
Ján Tomko