From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 11443936CD for ; Fri, 7 Apr 2023 14:25:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EFF8A1A06B for ; Fri, 7 Apr 2023 14:25:57 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 7 Apr 2023 14:25:56 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 80CDD464C8; Fri, 7 Apr 2023 14:25:56 +0200 (CEST) Message-ID: Date: Fri, 7 Apr 2023 14:25:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Markus Frank References: <20230314110942.279643-1-m.frank@proxmox.com> <20230314110942.279643-2-m.frank@proxmox.com> From: Dominik Csapak In-Reply-To: <20230314110942.279643-2-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.040 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server v4 1/5] enable clipboard parameter in vga_fmt X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Apr 2023 12:25:58 -0000 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 > --- > 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) { .... } 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);