11:45 a.m.
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
11:45 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
4:25 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
5:09 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
5:11 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
5:11 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:03 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
7:38 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
7:10 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
7:36 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
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
11:45 a.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: Omit SPICE address if no port is specified
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
4:26 a.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: Omit SPICE address if no port is specified
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
11:45 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: Don't add -spice port=0 when no port is specified
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
4:28 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: Don't add -spice port=0 when no port is specified
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)
5:11 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: Don't add -spice port=0 when no port is specified
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
3108
days inactive
3170
days old
14 comments
3 participants
participants (3)
-
Christophe Fergeau -
Cole Robinson -
Ján Tomko