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

[Thomas Lamprecht]

Am 7/29/19 um 10:59 AM schrieb Aaron Lauterer:
> 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)"
> +    },
> +};
> +
>  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
> +    },
>  };
>  
>  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' : '';

for me it's a bit strange that we add the "," here already, this adds
tight coupling between this method and the caller below, which I really
do not like. It seems a bit like an misguided effort to have not
everything in config_to_command, which itself is appreciated, but adds
so much coupling that now we're even worse off, as it suggests this
is a independent function, but it's not. It's very dependent on how
the caller uses its result in even a positional way, cannot be reused
and doesn't abstract one full step or device of the config-to-command
procedure away, only parts of that..

So either I'd:
* move the whole device pushing in it's own method, e.g., called "assemble_spice_devices",
  which gets just the config and the $pciaddr (the latter to avoid passing many
  more parameters) as params and let it return a list of "devices" we then can
  just pust to the @$devices list in config_to_command
* omit this, as the single "real" thing it does it a call to "parse_property_string",
  do that below and have less lines added in total and better code locality, and
  the addition to config_to_command shouldn't be much bigger than now (2 - 3 lines)

> +
> +	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);

I wrote above after I wrote this here, but I keep it maybe the comments help,
or at least allow to get the next version of this in shape faster :)

can we please try to keep naming conventions a little bit in sync
spice_enhancements (to match spice_ports)

or move the port allocation there too and name it $spice, so $spice->{port},
$spice->{foldersharing}, ... could be used? Just an idea, but could make things
slightly more readable (and move some lines away from "config_to_command")

There are a few other slightly strange things about the surrounding code,
e.g., why us $spice_port declared outside the "if ($qxlnum)" branch if it's
not used anywhere outside, why $localhost is added if we already have a module-
wide $nodename available, ... So if you want to add a separate patch clearing that
a bit up, it could be nice.

>  
> -	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
>