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

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

comments inline:

On 3/14/23 12:09, 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 | 47 ++++++++++++++++++++++++++++++++++++++---------
>   2 files changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..747eb62 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::clipboard_check_compatibility($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::clipboard_check_compatibility($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,
>   	    },
> +	    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->{clipboard} = $vga->{clipboard};
>   	}
>   	$status->{agent} = 1 if PVE::QemuServer::get_qga_key($conf, 'enabled');
>   
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 40be44d..461fe8f 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -193,8 +193,16 @@ my $vga_fmt = {
>   	minimum => 4,
>   	maximum => 512,
>       },
> +    clipboard => {
> +	description => "enable clipboard (requires spice tools in the guest)",
> +	type => 'boolean',
> +	optional => 1,
> +	default => 0
> +    }
>   };
>   
> +my $clipboardregex = qr/^(std|cirrus|vmware|virtio|qxl)/;
> +
>   my $ivshmem_fmt = {
>       size => {
>   	type => 'integer',
> @@ -1405,6 +1413,16 @@ sub pve_verify_hotplug_features {
>       die "unable to parse hotplug option\n";
>   }
>   
> +sub clipboard_check_compatibility {
> +    my ($vga) = @_;
> +
> +    if ($vga->{clipboard} && $vga->{type} !~ $clipboardregex) {
> +	die "vga type $vga->{type} is not compatible with clipboard\n";
> +    }
> +
> +    return $vga->{type} =~ $clipboardregex;
> +}

not really a fan of this function. it basically does two things at once
in a non-obvious way:

1. it asserts that the vga config is valid
2. it returns if the type would be compatible with the clipboard setting

my suggestion would be to have 2 methods here (disregard the names, were
just the first ones that came to mind, we should find better ones)

----8<----
# returns if the vga type can have the clipboard setting on
sub can_have_clipboard {
my ($type) = @_;
...
}

# dies if the vga setting regarding clipboard is invalid
sub assert_valid_clipboard_setting {
my ($vga) = @_;
...
}
---->8----

the assert method can ofc call the first one to check for that

> +
>   sub scsi_inquiry {
>       my($fh, $noerr) = @_;
>   
> @@ -3933,6 +3951,21 @@ sub config_to_command {
>   	push @$devices, '-device', "virtio-rng-pci,rng=rng0$limiter_str$rng_addr";
>       }
>   
> +    my $clipboardable = clipboard_check_compatibility($vga);

since you use that only one time, you could inline it in the if condition

> +    my $spicedevices = [];
> +
> +    if (($vga->{clipboard} && $clipboardable)
> +	|| ($vga->{type} =~ /^virtio/ || $qxlnum)) {
> +	my $pciaddr = print_pci_addr("spice", $bridges, $arch, $machine_type);
> +	push @$spicedevices, '-device', "virtio-serial,id=spice$pciaddr";
> +	if ($vga->{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";
> +    }

i get why you did it this way (i assume to keep the order of the devices the same)
but i think there is a better way

if the code enters the below block, it will always have entered above block

so if we modify the condition to the block below to include
'$vga->{clipboard} && $clipboardable', then we only have to put the few spice related things
into their own 'if (qxlnum....' block and we can scrape the spicedevices list and
simply push into the devices one

so i'd imagine the finished code to look like this:

if ($qxlnum || $vga... || ($vga->{clipboard} && $clipboardable) {
     if  ($qxlnum > 1) {
         ....
     }

    <code from above block here>

     if (qxlnum || $vga...) {
         ....
     }
)

in that case we might want to have a seperate function to check for
spice, then we don't have to have 2 conditions in sync

> +
>       my $spice_port;
>   
>       if ($qxlnum || $vga->{type} =~ /^virtio/) {
> @@ -3954,33 +3987,29 @@ sub config_to_command {
>   	    }
>   	}
>   
> -	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 @$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";
> -
>   	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";
> +	    push @$spicedevices, '-chardev', "spiceport,id=foldershare,name=org.spice-space.webdav.0";
> +	    push @$spicedevices, '-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};
>   
> -	push @$devices, '-spice', "$spice_opts";
> +	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);