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

[Aaron Lauterer]

I finally found some time to look into this. Answers are inline.

On 8/12/19 5:34 PM, Thomas Lamprecht wrote:
> 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)
Thanks for the hint. I will send a v2 with a better implementation.

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

In my efforts to clean up this section of the code I ran into the 
following things:

Regarding $spice_port being declared outside the whole "if ($qlxnum)" 
branch: It is used in the return values of "config_to_command" if an 
array is returned. If we are going to leave it there we might want to 
add a small comment to let future us know right away why it is placed in 
this seemingly awkward location.

Regarding the "$nodename": The steps to get from $nodename to $localhost 
were added on purpose with commit 9115244. I do not want to touch this, 
therefore the easy part to clean up is to remove the definition of 
$nodename and rely on the module wide $nodename.



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