Re: [pve-devel] [PATCH qemu-server v7 1/5] enable clipboard parameter in vga_fmt

[Dominik Csapak <d.csapak@proxmox.com>]

hi,

a few comments inline, hopefully we're soon at the finish line ;)

On 4/21/23 12:00, Markus Frank wrote:
> added option to use the qemu vdagent implementation to enable the noVNC
> clipboard. When enabled with SPICE the spice-vdagent gets replaced with the qemu
> implementation.
> 
> This patch does not solve #1406, but does allow copy and paste with
> a running X-session, when spice-vdagent is installed on the guest.
> 
> added clipboard variable to return at status/current
> 
> By that noVNC is able to check if clipboard is active.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   PVE/API2/Qemu.pm  | 13 +++++++++
>   PVE/QemuServer.pm | 68 ++++++++++++++++++++++++++++++++---------------
>   2 files changed, 60 insertions(+), 21 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..267527a 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -970,6 +970,9 @@ __PACKAGE__->register_method({
>   			$conf->{boot} = PVE::QemuServer::print_bootorder($devs);
>   		    }
>   
> +		    my $vga = PVE::QemuServer::parse_vga($conf->{vga});
> +		    PVE::QemuServer::assert_vnc_clipboard_config($vga);
> +
>   		    # auto generate uuid if user did not specify smbios1 option
>   		    if (!$conf->{smbios1}) {
>   			$conf->{smbios1} = PVE::QemuServer::generate_smbios1_uuid();
> @@ -1760,6 +1763,10 @@ my $update_vm_api  = sub {
>   			die "only root can modify '$opt' config for real devices\n";
>   		    }
>   		    $conf->{pending}->{$opt} = $param->{$opt};
> +		} elsif ($opt eq 'vga') {
> +		    my $vga = PVE::QemuServer::parse_vga($param->{$opt});
> +		    PVE::QemuServer::assert_vnc_clipboard_config($vga);
> +		    $conf->{pending}->{$opt} = $param->{$opt};
>   		} elsif ($opt =~ m/^usb\d+/) {
>   		    if ((!defined($conf->{$opt}) || $conf->{$opt} =~ m/spice/) && $param->{$opt} =~ m/spice/) {
>   			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> @@ -2580,6 +2587,11 @@ __PACKAGE__->register_method({
>   		type => 'boolean',
>   		optional => 1,
>   	    },
> +	    vnc_clipboard => {
> +		description => "QEMU clipboard for noVNC is enabled in config.",
> +		type => 'boolean',
> +		optional => 1,
> +	    },
>   	},
>       },
>       code => sub {
> @@ -2598,6 +2610,7 @@ __PACKAGE__->register_method({
>   	    my $spice = defined($vga->{type}) && $vga->{type} =~ /^virtio/;
>   	    $spice ||= PVE::QemuServer::vga_conf_has_spice($conf->{vga});
>   	    $status->{spice} = 1 if $spice;
> +	    $status->{vnc_clipboard} = $vga->{vnc_clipboard};
>   	}
>   	$status->{agent} = 1 if PVE::QemuServer::get_qga_key($conf, 'enabled');
>   
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 40be44d..f9928b7 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -193,8 +193,16 @@ my $vga_fmt = {
>   	minimum => 4,
>   	maximum => 512,
>       },
> +    vnc_clipboard => {
> +	description => "enable VNC clipboard (requires spice tools in the guest)",
> +	type => 'boolean',
> +	optional => 1,
> +	default => 0
> +    }
>   };
>   
> +my $vnc_clipboard_regex = qr/^(std|cirrus|vmware|virtio|qxl)/;
> +
>   my $ivshmem_fmt = {
>       size => {
>   	type => 'integer',
> @@ -1405,6 +1413,14 @@ sub pve_verify_hotplug_features {
>       die "unable to parse hotplug option\n";
>   }
>   
> +sub assert_vnc_clipboard_config {
> +    my ($vga) = @_;
> +
> +    if ($vga->{vnc_clipboard} && $vga->{type} && $vga->{type} !~ $vnc_clipboard_regex) {
> +	die "vga type $vga->{type} is not compatible with VNC clipboard\n";
> +    }
> +}
> +
>   sub scsi_inquiry {
>       my($fh, $noerr) = @_;
>   
> @@ -3933,9 +3949,13 @@ sub config_to_command {
>   	push @$devices, '-device', "virtio-rng-pci,rng=rng0$limiter_str$rng_addr";
>       }
>   
> +    my $spicedevices = [];

i don't think this is necessary, you replaced all pushes to @$devices below
with @$spicedevices, and push them all after that into @$devices

you should be able to push directly into @$devices, or am i missing something?

>       my $spice_port;
>   
> -    if ($qxlnum || $vga->{type} =~ /^virtio/) {
> +    assert_vnc_clipboard_config($vga);
> +
> +    if ($qxlnum || $vga->{type} =~ /^virtio/
> +	|| ($vga->{vnc_clipboard} && $vga->{type} =~ $vnc_clipboard_regex)) {

isn't that second part of the clipboard check redundant since you call
the 'assert_vnc_clipboard_config' above?

so at this point the type must be a match or the assert would have triggered?
(would make the condition a bit more readable if we can omit the type check)

also if the type would be not set, this would generate a perl warning i guess?


>   	if ($qxlnum > 1) {
>   	    if ($winversion){
>   		for (my $i = 1; $i < $qxlnum; $i++){
> @@ -3953,34 +3973,40 @@ sub config_to_command {
>   		push @$cmd, '-global', "qxl-vga.vram_size=$vram";
>   	    }
>   	}
> -
>   	my $pciaddr = print_pci_addr("spice", $bridges, $arch, $machine_type);
>   
> -	my $pfamily = PVE::Tools::get_host_address_family($nodename);
> -	my @nodeaddrs = PVE::Tools::getaddrinfo_all('localhost', family => $pfamily);
> -	die "failed to get an ip address of type $pfamily for 'localhost'\n" if !@nodeaddrs;
> +	push @$spicedevices, '-device', "virtio-serial,id=spice$pciaddr";
> +	if ($vga->{vnc_clipboard}) {
> +	    push @$spicedevices, '-chardev', 'qemu-vdagent,id=vdagent,name=vdagent,clipboard=on';
> +	} elsif ($vga->{type} =~ /^virtio/ || $qxlnum) {
> +	    push @$spicedevices, '-chardev', 'spicevmc,id=vdagent,name=vdagent';
> +	}
> +	push @$spicedevices, '-device', "virtserialport,chardev=vdagent,name=com.redhat.spice.0";
>   

this looks a bit confusing:

we are in the case where we definitely have either a vnc clipboard or spice configured
so either condition must always be true? if i understood that correclty
it would make more sense imho to simply drop the condition in the else path

you even reference the chardev in the unconditional code afterwards so having such a condition
looks like there might be a way to have neither

also does the order of these devices/chardevs really matter (aside from our regression tests?)
if not, we could simply add the required devices up front, and reuse the
if (qxlnum || ..)
path below to add the spicevmc and keep only the if (vnc_clipboard) above




> -	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 ($qxlnum || $vga->{type} =~ /^virtio/) {

this check could maybe be factored out, since it's used at least twice and is rather
non-trivial (a regex that looks like it matches only 'virtio' but is only anchored at
the beginning so it matches more than one)

> +	    my $pfamily = PVE::Tools::get_host_address_family($nodename);
> +	    my @nodeaddrs = PVE::Tools::getaddrinfo_all('localhost', family => $pfamily);
> +	    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 $localhost = PVE::Network::addr_to_ip($nodeaddrs[0]->{addr});
> +	    $spice_port = PVE::Tools::next_spice_port($pfamily, $localhost);
>   
> -	my $spice_enhancement_str = $conf->{spice_enhancements} // '';
> -	my $spice_enhancement = parse_property_string($spice_enhancements_fmt, $spice_enhancement_str);
> -	if ($spice_enhancement->{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";
> -	}
> -
> -	my $spice_opts = "tls-port=${spice_port},addr=$localhost,tls-ciphers=HIGH,seamless-migration=on";
> -	$spice_opts .= ",streaming-video=$spice_enhancement->{videostreaming}"
> -	    if $spice_enhancement->{videostreaming};
> +	    my $spice_enhancement_str = $conf->{spice_enhancements} // '';
> +	    my $spice_enhancement = parse_property_string($spice_enhancements_fmt, $spice_enhancement_str);
> +	    if ($spice_enhancement->{foldersharing}) {
> +		push @$spicedevices, '-chardev', "spiceport,id=foldershare,name=org.spice-space.webdav.0";
> +		push @$spicedevices, '-device', "virtserialport,chardev=foldershare,name=org.spice-space.webdav.0";
> +	    }
>   
> -	push @$devices, '-spice', "$spice_opts";
> +	    my $spice_opts = "tls-port=${spice_port},addr=$localhost,tls-ciphers=HIGH,seamless-migration=on";
> +	    $spice_opts .= ",streaming-video=$spice_enhancement->{videostreaming}"
> +		if $spice_enhancement->{videostreaming};
> +	    push @$spicedevices, '-spice', "$spice_opts";
> +	}
>       }
>   
> +    push @$devices, @$spicedevices;
> +
>       # enable balloon by default, unless explicitly disabled
>       if (!defined($conf->{balloon}) || $conf->{balloon}) {
>   	my $pciaddr = print_pci_addr("balloon0", $bridges, $arch, $machine_type);