10:20 a.m.
Hey,
I've been trying out the recently adding virgl support, which does
not need any TCP port/address to be set (it's directly opening a local unix
fd).
While looking at QEMU command line in configs such as
<graphics type="spice" autoport="no"/>, I've noticed that
the QEMU command line
gets some 'useless' arguments such as
-spice port=0
or
-spice addr=127.0.0.1
(without a port defined).
This series changes the spice command line generation so that we don't add these
arguments
when not needed.
Christophe
10:20 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: Make all SPICE command-line args optional
The goal is to not add -spice port=0,addr=127.0.0.1 to QEMU command line
when no SPICE port is specified in libvirt XML.
Before this change, we could rely on port or tlsPort to always be
present, so subsequent args could be unconditionally appended with a
leading ','. Now that it's no longer the case, we need to always check
whether the arg we are about to append is the first one or not.
---
src/qemu/qemu_command.c | 98 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 73 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 32d32b1..84db056 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7039,13 +7039,16 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
" but TLS is disabled in qemu.conf"));
goto error;
}
- if (port > 0)
+
+ if (virBufferUse(&opt) != 0)
virBufferAddChar(&opt, ',');
virBufferAsprintf(&opt, "tls-port=%u", tlsPort);
}
if (cfg->spiceSASL) {
- virBufferAddLit(&opt, ",sasl");
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAddLit(&opt, "sasl");
if (cfg->spiceSASLdir)
virCommandAddEnvPair(cmd, "SASL_CONF_PATH",
@@ -7084,18 +7087,25 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
if (!listenAddr)
listenAddr = cfg->spiceListen;
- if (listenAddr)
- virBufferAsprintf(&opt, ",addr=%s", listenAddr);
+ if (listenAddr) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ 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");
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAddLit(&opt, "agent-mouse=off");
break;
case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT:
- virBufferAddLit(&opt, ",agent-mouse=on");
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAddLit(&opt, "agent-mouse=on");
break;
default:
break;
@@ -7106,18 +7116,25 @@ 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) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAddLit(&opt, "disable-ticketing");
+ }
if (tlsPort > 0)
virBufferAsprintf(&opt, ",x509-dir=%s",
cfg->spiceTLSx509certdir);
switch (defaultMode) {
case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
- virBufferAddLit(&opt, ",tls-channel=default");
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAddLit(&opt, "tls-channel=default");
break;
case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
- virBufferAddLit(&opt, ",plaintext-channel=default");
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAddLit(&opt, "plaintext-channel=default");
break;
case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY:
/* nothing */
@@ -7175,30 +7192,50 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
}
}
- if (graphics->data.spice.image)
- virBufferAsprintf(&opt, ",image-compression=%s",
+ if (graphics->data.spice.image) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ 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) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ 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) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ 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) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ 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) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ 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) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ 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");
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAddLit(&opt, "disable-agent-file-xfer");
}
}
@@ -7209,20 +7246,31 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
goto error;
}
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
/* 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));
}
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
/* 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");
+ virBufferAddLit(&opt, "seamless-migration=on");
}
+ /* 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)
+ virBufferAddLit(&opt, "port=0");
+
virCommandAddArg(cmd, "-spice");
virCommandAddArgBuffer(cmd, &opt);
if (graphics->data.spice.keymap)
--
2.5.0
12:16 p.m.
New subject: [libvirt] [PATCH 1/3] qemu: Make all SPICE command-line args optional
On Tue, Mar 15, 2016 at 10:20:48AM +0100, Christophe Fergeau wrote:
The goal is to not add -spice port=0,addr=127.0.0.1 to QEMU command
line
when no SPICE port is specified in libvirt XML.
Misleading, even after the series port=0 is generated.
Before this change, we could rely on port or tlsPort to always be
present, so subsequent args could be unconditionally appended with a
leading ','. Now that it's no longer the case, we need to always check
whether the arg we are about to append is the first one or not.
---
src/qemu/qemu_command.c | 98 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 73 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 32d32b1..84db056 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7039,13 +7039,16 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
" but TLS is disabled in qemu.conf"));
goto error;
}
- if (port > 0)
+
+ if (virBufferUse(&opt) != 0)
virBufferAddChar(&opt, ',');
virBufferAsprintf(&opt, "tls-port=%u", tlsPort);
Instead of all these conditions, we can convert all the arguments to add
a trailing comma and remove the last one by calling
virBufferTrim(&opt, ",", -1);
at the end.
}
+ /* 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)
+ virBufferAddLit(&opt, "port=0");
+
I would rather add this change to the commit that disables the first
port=0.
Also, there is no need to go through the buffer, the string can be added
directly to the virCommand.
Jan
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
9:43 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: Make all SPICE command-line args optional
Hey,
On Tue, Mar 15, 2016 at 12:16:10PM +0100, Ján Tomko wrote:
On Tue, Mar 15, 2016 at 10:20:48AM +0100, Christophe Fergeau wrote:
> The goal is to not add -spice port=0,addr=127.0.0.1 to QEMU command line
> when no SPICE port is specified in libvirt XML.
Misleading, even after the series port=0 is generated.
Indeed, I'll reword this.
> Before this change, we could rely on port or tlsPort to always be
> present, so subsequent args could be unconditionally appended with a
> leading ','. Now that it's no longer the case, we need to always check
> whether the arg we are about to append is the first one or not.
Even that part is not correct before the last patch in the series, I'll
reword it too.
> ---
> src/qemu/qemu_command.c | 98 ++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 73 insertions(+), 25 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 32d32b1..84db056 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7039,13 +7039,16 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr
cfg,
> " but TLS is disabled in qemu.conf"));
> goto error;
> }
> - if (port > 0)
> +
> + if (virBufferUse(&opt) != 0)
> virBufferAddChar(&opt, ',');
> virBufferAsprintf(&opt, "tls-port=%u", tlsPort);
Instead of all these conditions, we can convert all the arguments to add
a trailing comma and remove the last one by calling
virBufferTrim(&opt, ",", -1);
at the end.
Ah indeed, I knew there had to be a better way than adding these checks
all over the place. Thanks a lot, I'll change it :)
> }
>
> + /* 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)
> + virBufferAddLit(&opt, "port=0");
> +
I would rather add this change to the commit that disables the first
port=0.
Ok, I was torn between adding it here or in the commit you mention, I'll
move it :)
Also, there is no need to go through the buffer, the string can be added
directly to the virCommand.
Ah ok, thanks.
I'll send a v2!
Christophe
10:20 a.m.
New subject: [libvirt] [PATCH 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.
---
This one is more readable with git show -w, it's just
if (port > 0 || tlsPort > 0) {
<indentation change>
}
Christophe
src/qemu/qemu_command.c | 65 +++++++++++-----------
.../qemuxml2argv-controller-order.args | 2 +-
2 files changed, 34 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 84db056..5d37d28 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7057,44 +7057,45 @@ 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;
-
- 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) {
+ if (virBufferUse(&opt) != 0)
+ virBufferAddChar(&opt, ',');
+ virBufferAsprintf(&opt, "addr=%s", listenAddr);
+ }
- if (!listenAddr)
- listenAddr = cfg->spiceListen;
- if (listenAddr) {
- if (virBufferUse(&opt) != 0)
- virBufferAddChar(&opt, ',');
- virBufferAsprintf(&opt, "addr=%s", listenAddr);
+ VIR_FREE(netAddr);
}
- 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
12:17 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: Omit SPICE address if no port is specified
On Tue, Mar 15, 2016 at 10:20:49AM +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.
---
This one is more readable with git show -w, it's just
if (port > 0 || tlsPort > 0) {
<indentation change>
}
Christophe
src/qemu/qemu_command.c | 65 +++++++++++-----------
.../qemuxml2argv-controller-order.args | 2 +-
2 files changed, 34 insertions(+), 33 deletions(-)
ACK
Jan
10:20 a.m.
New subject: [libvirt] [PATCH 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.
---
src/qemu/qemu_command.c | 2 +-
tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5d37d28..a4e6f84 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) {
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
--
2.5.0
3108
days inactive
3109
days old
6 comments
2 participants
participants (2)
-
Christophe Fergeau -
Ján Tomko