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

[Dominik Csapak]

Looks ok overall, some comments/questions inline

On 7/29/19 10:59 AM, Aaron Lauterer wrote:
> This adds a new config option called `spice_enhancements` and two
> settings for it:
> 
> * videostreaming
> * foldersharing
> 
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
> 
> Feedback is appreciated, especially regarding:
> 
> * naming of the new config option
> * design of the `conf_has_spice_enhancements` method
>    * general structure with different returns
>    * ternary operator
> 
> PVE/QemuServer.pm | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 8c519b5..693c33b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -295,6 +295,19 @@ my $audio_fmt = {
>       },
>   };
>   
> +my $spice_enhancements_fmt = {
> +    foldersharing => {
> +	type => 'boolean',
> +	optional => 1,
> +	description =>  "Enable folder sharing via SPICE."
> +    },
> +    videostreaming =>  {
> +	type => 'boolean',
> +	optional => 1,
> +	description => "Enable video streaming. (Better performance but compression)"

This description sounds weird for me.. why is compression bad? normally 
compression is bad because performance is lower due to higher cpu load,
but it also says better performance, so... no downsides?

i think something like:

'uses compression and encoding for (detected) video streams'

without mentioning performance is better

> +    },
> +};
> +
>   my $confdesc = {
>       onboot => {
>   	optional => 1,
> @@ -672,6 +685,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

we have an optional propertystring with only optional elements...
what happens if i set it to the empty value?
e.g.

qm set ID -spiceenhancements ''

?

> +    },
>   };
>   
>   my $cicustom_fmt = {
> @@ -3454,6 +3473,26 @@ sub vga_conf_has_spice {
>       return $1 || 1;
>   }
>   
> +sub conf_has_spice_enhancements {
> +    my ($conf) = @_;
> +
> +    if ($conf->{spice_enhancements}) {
> +	my $enhancements = PVE::JSONSchema::parse_property_string($spice_enhancements_fmt, $conf->{spice_enhancements});
> +
> +	my $videostreaming = ($enhancements->{videostreaming}) ? ',streaming-video=all' : '';

why not let videostreaming be an enum with filter|all ? so that we can 
let the user choose the mode?

> +
> +	return {
> +	    videostreaming => $videostreaming,
> +	    foldersharing => $enhancements->{foldersharing},
> +	}
> +    } else {
> +	return {
> +	    videostreaming => '',
> +	    foldersharing => 0,
> +	};
> +    }
> +}
> +
>   my $host_arch; # FIXME: fix PVE::Tools::get_host_arch
>   sub get_host_arch() {
>       $host_arch = (POSIX::uname())[4] if !$host_arch;
> @@ -3982,12 +4021,18 @@ sub config_to_command {
>   	die "failed to get an ip address of type $pfamily for 'localhost'\n" if !@nodeaddrs;
>   	my $localhost = PVE::Network::addr_to_ip($nodeaddrs[0]->{addr});
>   	$spice_port = PVE::Tools::next_spice_port($pfamily, $localhost);
> +	my $spiceenhancements = conf_has_spice_enhancements($conf);
>   
> -	push @$devices, '-spice', "tls-port=${spice_port},addr=$localhost,tls-ciphers=HIGH,seamless-migration=on";
> +	push @$devices, '-spice', "tls-port=${spice_port},addr=$localhost,tls-ciphers=HIGH,seamless-migration=on$spiceenhancements->{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 ($spiceenhancements->{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
>