looks good, but 2 comments inline
On 8/20/19 6:02 PM, Aaron Lauterer wrote:
This adds a new config option called `spice_enhancements` with two
optional settings:
* videostreaming
* foldersharing
Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
v1 -> v2:
format changes suggested by dominik:
* changed descriptions
* changed `videostreaming` to enum
refactoring suggested by thomas:
I went with moving the `has_spice_enhancements` to the
`config_to_command` function and reducing the LOC.
Added a `spice` hash to store spice settings. Intended to be used in
further cleanup patches in this part of the code.
An empty `spice_enhancements` in the config file can be set (thanks
Dominik for pointing it out). That case is now handled to avoid errors.
The `streaming-video` option is always present now in the `-spice`
device. My tests showed no problems when migrating from a PVE node
without this patch to one with it. Migrating from newer PVE to an older
version without the patch resultet in a "resume failed" error.
normally we do not put commit changelogs in the commit message
---
but here <---
this way, they do not show up in the git history
PVE/QemuServer.pm | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 9f5bf56..4701193 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -295,6 +295,20 @@ my $audio_fmt = {
},
};
+my $spice_enhancements_fmt = {
+ foldersharing => {
+ type => 'boolean',
+ optional => 1,
+ description => "Enable folder sharing via SPICE. Needs Spice-WebDAV daemon
installed in the VM."
+ },
+ videostreaming => {
+ type => 'string',
+ enum => ['off', 'all', 'filter'],
+ optional => 1,
+ description => "Enable video streaming. Uses compression for detected video
streams."
+ },
+};
+
my $confdesc = {
onboot => {
optional => 1,
@@ -672,6 +686,12 @@ EODESCR
description => "Configure a audio device, useful in combination with
QXL/Spice.",
optional => 1
},
+ spice_enhancements => {
+ type => 'string',
+ format => $spice_enhancements_fmt,
+ description => "Configure additional enhancements for SPICE.",
+ optional => 1
+ },
};
my $cicustom_fmt = {
@@ -3992,11 +4012,21 @@ sub config_to_command {
my $localhost = PVE::Network::addr_to_ip($nodeaddrs[0]->{addr});
$spice_port = PVE::Tools::next_spice_port($pfamily, $localhost);
- push @$devices, '-spice', "tls-port=${spice_port},addr=$localhost,tls-ciphers=HIGH,seamless-migration=on";
+ my $spice = {};
+ my $spice_enhancements = $conf->{spice_enhancements} ?
PVE::JSONSchema::parse_property_string($spice_enhancements_fmt,
$conf->{spice_enhancements}) : {} ;
+ $spice->{videostreaming} = $spice_enhancements->{videostreaming} //
'off';
i would prefer to keep it backwards compatible, like so:
$spice->{videostreaming} = $spice_enhancements->{videostreaming} ?
",streaming-video=$spice_enhancements->{videostreaming}" : "";
and simply add this string to the '-spice' parameters
+ $spice->{foldersharing} = $spice_enhancements->{foldersharing} // 0;
the '// 0' is unecessary, but it does not really hurt...
+
+ push @$devices, '-spice',
"tls-port=${spice_port},addr=$localhost,tls-ciphers=HIGH,seamless-migration=on,streaming-video=$spice->{videostreaming}";
push @$devices, '-device', "virtio-serial,id=spice$pciaddr";
push @$devices, '-chardev', "spicevmc,id=vdagent,name=vdagent";
push @$devices, '-device',
"virtserialport,chardev=vdagent,name=com.redhat.spice.0";
+
+ if ($spice_enhancements->{foldersharing}) {
+ push @$devices, '-chardev',
"spiceport,id=foldershare,name=org.spice-space.webdav.0";
+ push @$devices, '-device',
"virtserialport,chardev=foldershare,name=org.spice-space.webdav.0";
+ }
}
# enable balloon by default, unless explicitly disabled
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
