[pve-devel] [PATCH v2 qemu-server] Fix #2041, #2272 Add Spice enhancements

[Dominik Csapak]

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.lauterer at 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
>