[libvirt] [PATCH 0/3] Cosmetic changes to SPICE QEMU commandline
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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
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
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
participants (2)
-
Christophe Fergeau -
Ján Tomko