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

[Aaron Lauterer]

On 8/6/19 1:41 PM, Dominik Csapak wrote:
> 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

Valid point regarding the descriptions phrasing.

Funnily enough the CPU load on the host is lower with video streaming 
and without it. Sending frames as PNGs vs. video. Though with 
compression there will be visible artifacts in the right conditions.

But yes, the phrasing should be made simpler.

> 
>> +    },
>> +};
>> +
>>   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 ''
> 
> ?

Current behavior is a config line without any options. Starting a VM 
with it seems to work fine.
Are there possible problems down the road with this, besides it not 
being pretty?
Is there another sane way to store these settings? Storing it in `vga` 
property string if spice/qxl is selected does seem a bit wrong to me.
Also see further below to the next comment.

> 
>> +    },
>>   };
>>   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?

First idea was to keep it simple. If enum is used we can use all 3 
possible values off|filter|all and set off as the default.

Then we either attach the `streaming-video=` part to the `-spice` 
parameter if needed or have it always present with the selected value.

This could also help with the only optional elements discussed above.

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