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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 32A9D99ACD for ; Wed, 3 May 2023 09:11:20 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0B238371FF for ; Wed, 3 May 2023 09:10:50 +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 ; Wed, 3 May 2023 09:10:48 +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 ADF97470EB; Wed, 3 May 2023 09:10:48 +0200 (CEST) Message-ID: <1682dcc4-3852-d5be-dabd-30655e5f9d8a@proxmox.com> Date: Wed, 3 May 2023 09:10:47 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Markus Frank References: <20230421100009.98063-1-m.frank@proxmox.com> <20230421100009.98063-2-m.frank@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20230421100009.98063-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.015 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemu.pm, qemuserver.pm] Subject: Re: [pve-devel] [PATCH qemu-server v7 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: Wed, 03 May 2023 07:11:20 -0000 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 > --- > 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);