[pve-devel] [PATCH qemu-server/novnc/manager/docs v3 0/6] Feature noVNC-Clipboard

[pve-devel] [PATCH qemu-server/novnc/manager/docs v3 0/6] Feature noVNC-Clipboard

[Markus Frank]

This patch-series adds a parameter to vga_fmt, that enables the
noVNC-Clipboard and replaces the default SPICE-Clipboard if SPICE is
used.

changes v3:
* added hint to make clearer that the spice guest tools are required for
 the noVNC-clipboard
* Checkbox changes to ComboBox when a spice device is selected to make
 clear that only one clipboard can be used at a time.
* added 2 test-cases

changes v2:
* added pci address to virtio-serial-pci

proxmox-backup:

Markus Frank (3):
  enable clipboard parameter in vga_fmt
  added clipboard variable to return at status/current
  test cases for clipboard spice & std

 PVE/API2/Qemu.pm                            |  6 +++++
 PVE/QemuServer.pm                           | 19 ++++++++++++++-
 PVE/QemuServer/PCI.pm                       |  3 ++-
 test/cfg2cmd/noVNC-clipboard-spice.conf     |  1 +
 test/cfg2cmd/noVNC-clipboard-spice.conf.cmd | 27 +++++++++++++++++++++
 test/cfg2cmd/noVNC-clipboard-std.conf       |  1 +
 test/cfg2cmd/noVNC-clipboard-std.conf.cmd   | 27 +++++++++++++++++++++
 7 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 test/cfg2cmd/noVNC-clipboard-spice.conf
 create mode 100644 test/cfg2cmd/noVNC-clipboard-spice.conf.cmd
 create mode 100644 test/cfg2cmd/noVNC-clipboard-std.conf
 create mode 100644 test/cfg2cmd/noVNC-clipboard-std.conf.cmd

novnc-pve:

Markus Frank (1):
  added show clipboard button patch to series

 .../patches/0019-show-clipboard-button.patch  | 31 +++++++++++++++++++
 debian/patches/series                         |  1 +
 2 files changed, 32 insertions(+)
 create mode 100644 debian/patches/0019-show-clipboard-button.patch

pve-manager:

Markus Frank (1):
  added clipboard checkbox & combobox to DisplayEdit

 www/manager6/qemu/DisplayEdit.js | 62 +++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

pve-docs:

Markus Frank (1):
  added noVNC clipboard documentation

 qm.adoc | 11 +++++++++++
 1 file changed, 11 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH qemu-server v3 1/6] enable clipboard parameter in vga_fmt

[Markus Frank]

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.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 PVE/QemuServer.pm     | 19 ++++++++++++++++++-
 PVE/QemuServer/PCI.pm |  3 ++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c706653..333afc2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -190,6 +190,12 @@ my $vga_fmt = {
 	minimum => 4,
 	maximum => 512,
     },
+    clipboard => {
+	description => "enable clipboard (requires spice-vdagent)",
+	type => 'boolean',
+	optional => 1,
+	default => 0
+    }
 };
 
 my $ivshmem_fmt = {
@@ -3836,6 +3842,13 @@ sub config_to_command {
 	}
     }
 
+    if ($vga->{clipboard} && $vga->{type} =~ /^std|^cirrus|^vmware/) {
+	push @$devices, '-chardev', 'qemu-vdagent,id=vdagent,name=vdagent,clipboard=on';
+	my $pciaddr = print_pci_addr("clipboard", $bridges, $arch, $machine_type);
+	push @$devices, '-device', "virtio-serial-pci$pciaddr";
+	push @$devices, '-device', 'virtserialport,chardev=vdagent,name=com.redhat.spice.0';
+    }
+
     my $rng = $conf->{rng0} ? parse_rng($conf->{rng0}) : undef;
     if ($rng && $version_guard->(4, 1, 2)) {
 	check_rng_source($rng->{source});
@@ -3880,7 +3893,11 @@ sub config_to_command {
 	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";
+	if ($vga->{clipboard}) {
+	    push @$devices, '-chardev', 'qemu-vdagent,id=vdagent,name=vdagent,clipboard=on';
+	} else {
+	    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});
diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
index 3d0e70e..7ddabe0 100644
--- a/PVE/QemuServer/PCI.pm
+++ b/PVE/QemuServer/PCI.pm
@@ -138,7 +138,8 @@ sub get_pci_addr_map {
 	scsihw1 => { bus => 0, addr => 6 },
 	ahci0 => { bus => 0, addr => 7 },
 	qga0 => { bus => 0, addr => 8 },
-	spice => { bus => 0, addr => 9 },
+	spice => { bus => 0, addr => 9, conflict_ok => qw(clipboard) },
+	clipboard => { bus => 0, addr => 9, conflict_ok => qw(spice) }, # clipboard is used if spice is not running
 	virtio0 => { bus => 0, addr => 10 },
 	virtio1 => { bus => 0, addr => 11 },
 	virtio2 => { bus => 0, addr => 12 },
-- 
2.30.2

[pve-devel] [PATCH qemu-server v3 2/6] added clipboard variable to return at status/current

[Markus Frank]

By that noVNC is able to check if clipboard is active.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 PVE/API2/Qemu.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 99b426e..25f3a1d 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -2428,6 +2428,11 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		optional => 1,
 	    },
+	    clipboard => {
+		description => "Qemu clipboard enabled in config.",
+		type => 'boolean',
+		optional => 1,
+	    },
 	},
     },
     code => sub {
@@ -2446,6 +2451,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');
 
-- 
2.30.2

[pve-devel] [PATCH qemu-server v3 3/6] test cases for clipboard spice & std

[Markus Frank]

added one test case for a spice display and one for std

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 test/cfg2cmd/noVNC-clipboard-spice.conf     |  1 +
 test/cfg2cmd/noVNC-clipboard-spice.conf.cmd | 27 +++++++++++++++++++++
 test/cfg2cmd/noVNC-clipboard-std.conf       |  1 +
 test/cfg2cmd/noVNC-clipboard-std.conf.cmd   | 27 +++++++++++++++++++++
 4 files changed, 56 insertions(+)
 create mode 100644 test/cfg2cmd/noVNC-clipboard-spice.conf
 create mode 100644 test/cfg2cmd/noVNC-clipboard-spice.conf.cmd
 create mode 100644 test/cfg2cmd/noVNC-clipboard-std.conf
 create mode 100644 test/cfg2cmd/noVNC-clipboard-std.conf.cmd

diff --git a/test/cfg2cmd/noVNC-clipboard-spice.conf b/test/cfg2cmd/noVNC-clipboard-spice.conf
new file mode 100644
index 0000000..d9d933d
--- /dev/null
+++ b/test/cfg2cmd/noVNC-clipboard-spice.conf
@@ -0,0 +1 @@
+vga: qxl,clipboard=1
diff --git a/test/cfg2cmd/noVNC-clipboard-spice.conf.cmd b/test/cfg2cmd/noVNC-clipboard-spice.conf.cmd
new file mode 100644
index 0000000..f24cc7f
--- /dev/null
+++ b/test/cfg2cmd/noVNC-clipboard-spice.conf.cmd
@@ -0,0 +1,27 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'qxl-vga,id=vga,max_outputs=4,bus=pci.0,addr=0x2' \
+  -device 'virtio-serial,id=spice,bus=pci.0,addr=0x9' \
+  -chardev 'qemu-vdagent,id=vdagent,name=vdagent,clipboard=on' \
+  -device 'virtserialport,chardev=vdagent,name=com.redhat.spice.0' \
+  -spice 'tls-port=61000,addr=127.0.0.1,tls-ciphers=HIGH,seamless-migration=on' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -machine 'type=pc+pve0'
diff --git a/test/cfg2cmd/noVNC-clipboard-std.conf b/test/cfg2cmd/noVNC-clipboard-std.conf
new file mode 100644
index 0000000..ec84637
--- /dev/null
+++ b/test/cfg2cmd/noVNC-clipboard-std.conf
@@ -0,0 +1 @@
+vga: std,clipboard=1
diff --git a/test/cfg2cmd/noVNC-clipboard-std.conf.cmd b/test/cfg2cmd/noVNC-clipboard-std.conf.cmd
new file mode 100644
index 0000000..2a7e3f4
--- /dev/null
+++ b/test/cfg2cmd/noVNC-clipboard-std.conf.cmd
@@ -0,0 +1,27 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -chardev 'qemu-vdagent,id=vdagent,name=vdagent,clipboard=on' \
+  -device 'virtio-serial-pci,bus=pci.0,addr=0x9' \
+  -device 'virtserialport,chardev=vdagent,name=com.redhat.spice.0' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -machine 'type=pc+pve0'
-- 
2.30.2

[pve-devel] [PATCH novnc v3 4/6] added show clipboard button patch to series

[Markus Frank]

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 .../patches/0019-show-clipboard-button.patch  | 31 +++++++++++++++++++
 debian/patches/series                         |  1 +
 2 files changed, 32 insertions(+)
 create mode 100644 debian/patches/0019-show-clipboard-button.patch

diff --git a/debian/patches/0019-show-clipboard-button.patch b/debian/patches/0019-show-clipboard-button.patch
new file mode 100644
index 0000000..9075f4b
--- /dev/null
+++ b/debian/patches/0019-show-clipboard-button.patch
@@ -0,0 +1,31 @@
+From 338b94a5d7b3ec65ce3f4b9a91420ee5f155077e Mon Sep 17 00:00:00 2001
+From: Markus Frank <m.frank@proxmox.com>
+Date: Fri, 28 Oct 2022 13:57:57 +0200
+Subject: [PATCH] show clipboard button
+
+show button when clipboard at status/current is true
+
+Signed-off-by: Markus Frank <m.frank@proxmox.com>
+---
+ app/pve.js | 5 +++++
+ 1 file changed, 5 insertions(+)
+
+diff --git a/app/pve.js b/app/pve.js
+index 287615f..639e598 100644
+--- a/app/pve.js
++++ b/app/pve.js
+@@ -411,6 +411,11 @@ PVEUI.prototype = {
+ 			document.getElementById('pve_start_dlg')
+ 			    .classList.add("noVNC_open");
+ 		    }
++		    let clipboard = result.data.clipboard;
++		    if (clipboard) {
++			document.getElementById('noVNC_clipboard_button')
++			    .classList.remove('pve_hidden');
++		    }
+ 		},
+ 		failure: function(msg, code) {
+ 		    if (code === 403) {
+-- 
+2.36.1
+
diff --git a/debian/patches/series b/debian/patches/series
index ef9e9df..1eb50db 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -17,3 +17,4 @@ extra/0001-Ignore-ResizeObserver-errors.patch
 0016-hide-fullscreen-button-on-isFullscreen-get-variable.patch
 0017-make-error-hideable.patch
 0018-show-start-button-on-not-running-vm-ct.patch
+0019-show-clipboard-button.patch
-- 
2.36.1

[pve-devel] [PATCH manager v3 5/6] added clipboard checkbox & combobox to DisplayEdit

[Markus Frank]

If display is set to spice the checkbox gets replaced by a combobox to
show the available clipboard options.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 www/manager6/qemu/DisplayEdit.js | 62 +++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/DisplayEdit.js b/www/manager6/qemu/DisplayEdit.js
index 9bb1763e..2cef5ad2 100644
--- a/www/manager6/qemu/DisplayEdit.js
+++ b/www/manager6/qemu/DisplayEdit.js
@@ -33,22 +33,54 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
 		    return;
 		}
 		let memoryfield = this.up('panel').down('field[name=memory]');
+		let clipboardBox = this.up('panel').down('field[itemId=clipboardBox]');
+		let clipboardDrop = this.up('panel').down('field[itemId=clipboardDrop]');
+		let vdagentHint = this.up('panel').down('field[name=vdagentHint]');
 		let disableMemoryField = false;
+		let spice = false;
+		let showClipboardAndHint = true;
 
 		if (val === "cirrus") {
 		    memoryfield.setEmptyText("4");
-		} else if (val === "std" || val.match(/^qxl\d?$/) || val === "vmware") {
+		} else if (val === "std" || val === "vmware") {
 		    memoryfield.setEmptyText("16");
+		} else if (val.match(/^qxl\d?$/)) {
+		    memoryfield.setEmptyText("16");
+		    spice = true;
 		} else if (val.match(/^virtio/)) {
 		    memoryfield.setEmptyText("256");
+		    spice = true;
 		} else if (val.match(/^(serial\d|none)$/)) {
 		    memoryfield.setEmptyText("N/A");
 		    disableMemoryField = true;
+		    showClipboardAndHint = false;
 		} else {
 		    console.debug("unexpected display type", val);
 		    memoryfield.setEmptyText(Proxmox.Utils.defaultText);
 		}
 		memoryfield.setDisabled(disableMemoryField);
+		vdagentHint.setVisible(showClipboardAndHint);
+		if (showClipboardAndHint) {
+		    // switch from Checkbox to ComboBox and vice versa
+		    clipboardBox.setDisabled(spice);
+		    clipboardDrop.setDisabled(!spice);
+		    clipboardBox.setVisible(!spice);
+		    clipboardDrop.setVisible(spice);
+		    // reset value when changing to spice,
+		    // so that you have to actively change to noVNC Clipboard
+		    if (spice) {
+			clipboardDrop.setValue('__default__');
+		    }
+		} else {
+		    // reset to default
+		    clipboardBox.setValue(false);
+		    clipboardDrop.setValue('__default__');
+		    // show only the disabled Checkbox
+		    clipboardBox.setDisabled(true);
+		    clipboardDrop.setDisabled(true);
+		    clipboardBox.setVisible(true);
+		    clipboardDrop.setVisible(false);
+		}
 	    },
 	},
     },
@@ -60,6 +92,34 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
 	maxValue: 512,
 	step: 4,
 	name: 'memory',
+    },
+    {
+	xtype: 'proxmoxcheckbox',
+	fieldLabel: gettext('noVNC Clipboard'),
+	name: 'clipboard',
+	itemId: 'clipboardBox',
+    },
+    {
+	name: 'clipboard',
+	itemId: 'clipboardDrop',
+	xtype: 'proxmoxKVComboBox',
+	value: '__default__',
+	deleteEmpty: false,
+	fieldLabel: gettext('Clipboard'),
+	comboItems: [
+	    ['__default__', 'SPICE-Clipboard'],
+	    ['1', 'noVNC-Clipboard'],
+	],
+	disabled: true,
+	hidden: true,
+    },
+    {
+	itemId: 'vdagentHint',
+	name: 'vdagentHint',
+	xtype: 'displayfield',
+	userCls: 'pmx-hint',
+	value: 'Clipboard for noVNC requires spice-tools installed and ' +
+	    'enabled in the Guest-VM.',
     }],
 });
 
-- 
2.30.2

[pve-devel] [PATCH docs v3 6/6] added noVNC clipboard documentation

[Markus Frank]

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 qm.adoc | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index 4d0c7c4..3a575bc 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -693,6 +693,17 @@ Selecting `serialX` as display 'type' disables the VGA output, and redirects
 the Web Console to the selected serial port. A configured display 'memory'
 setting will be ignored in that case.
 
+You can enable the noVNC clipboard by setting 'clipboard' to 1.
+To use this, you need to install and enable spice-vdagent on the VM Guest: 
+`apt install spice-vdagent`
+
+Doing this will give you the ability to use the clipboard button of the novnc
+console. However, when using SPICE, you have to decide which clipboard you want
+to use, because the default SPICE clipboard implementation will be replaced by
+the qemu-vdagent implementation, which is used by noVNC. 
+This means you cannot simply copy and paste into a SPICE session and instead 
+need to use the noVNC button, when using SPICE with noVNC-clipboard.
+
 [[qm_usb_passthrough]]
 USB Passthrough
 ~~~~~~~~~~~~~~~
-- 
2.30.2

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

[Dominik Csapak]

first:
sorry for the late review ;)

high level comments:

i know why you're adding it to the vga format, but maybe that's not the way
to go? we couple the console method with the display quite a lot, but
maybe we don't want to further increase that coupling?

(imho it would be much nicer to be able to define the console method/options
independently to the virtual display hardware)

any thoughts to that anyone?


On 10/28/22 14:33, 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.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   PVE/QemuServer.pm     | 19 ++++++++++++++++++-
>   PVE/QemuServer/PCI.pm |  3 ++-
>   2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index c706653..333afc2 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -190,6 +190,12 @@ my $vga_fmt = {
>   	minimum => 4,
>   	maximum => 512,
>       },
> +    clipboard => {
> +	description => "enable clipboard (requires spice-vdagent)",
i'd expand/change that a bit, e.g. by saying it needs the spice tools
in the guest (spice-vdagent is rather linux specific)

> +	type => 'boolean',
> +	optional => 1,
> +	default => 0
> +    }
>   };
>   
>   my $ivshmem_fmt = {
> @@ -3836,6 +3842,13 @@ sub config_to_command {
>   	}
>       }
>   
> +    if ($vga->{clipboard} && $vga->{type} =~ /^std|^cirrus|^vmware/) {

that regex could be written as /^(std|cirrus|vmware)/, it's a bit clearer and
if one adds one, the chance to oversee/forget the '^' is mitigated

also, whats with serial ports? can it even work on that?

maybe it would be nicer to have some compatibility hash where we can simply
add the vga methods supported, and add it here only instead of in two places?
(just a thought, you don't have to go that way if it's not much better)

> +	push @$devices, '-chardev', 'qemu-vdagent,id=vdagent,name=vdagent,clipboard=on';
> +	my $pciaddr = print_pci_addr("clipboard", $bridges, $arch, $machine_type);
> +	push @$devices, '-device', "virtio-serial-pci$pciaddr";
> +	push @$devices, '-device', 'virtserialport,chardev=vdagent,name=com.redhat.spice.0';
> +    }
> +
>       my $rng = $conf->{rng0} ? parse_rng($conf->{rng0}) : undef;
>       if ($rng && $version_guard->(4, 1, 2)) {
>   	check_rng_source($rng->{source});
> @@ -3880,7 +3893,11 @@ sub config_to_command {
>   	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";
> +	if ($vga->{clipboard}) {
> +	    push @$devices, '-chardev', 'qemu-vdagent,id=vdagent,name=vdagent,clipboard=on';

like i wrote above, we add it in two different places (that cannot happen together AFAICS)

it would be much nicer if we'd only have a single point in the code where we add that

> +	} else {
> +	    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});
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 3d0e70e..7ddabe0 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -138,7 +138,8 @@ sub get_pci_addr_map {
>   	scsihw1 => { bus => 0, addr => 6 },
>   	ahci0 => { bus => 0, addr => 7 },
>   	qga0 => { bus => 0, addr => 8 },
> -	spice => { bus => 0, addr => 9 },
> +	spice => { bus => 0, addr => 9, conflict_ok => qw(clipboard) },
> +	clipboard => { bus => 0, addr => 9, conflict_ok => qw(spice) }, # clipboard is used if spice is not running
>   	virtio0 => { bus => 0, addr => 10 },
>   	virtio1 => { bus => 0, addr => 11 },
>   	virtio2 => { bus => 0, addr => 12 },

Re: [pve-devel] [PATCH qemu-server v3 2/6] added clipboard variable to return at status/current

[Dominik Csapak]

comment inline

On 10/28/22 14:33, Markus Frank wrote:
> By that noVNC is able to check if clipboard is active.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   PVE/API2/Qemu.pm | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 99b426e..25f3a1d 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2428,6 +2428,11 @@ __PACKAGE__->register_method({
>   		type => 'boolean',
>   		optional => 1,
>   	    },
> +	    clipboard => {
> +		description => "Qemu clipboard enabled in config.",

i'd expand a bit here, e.g. by mentioning novnc

> +		type => 'boolean',
> +		optional => 1,
> +	    },
>   	},
>       },
>       code => sub {
> @@ -2446,6 +2451,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');
>   

Re: [pve-devel] [PATCH manager v3 5/6] added clipboard checkbox & combobox to DisplayEdit

[Dominik Csapak]

high level comments:

i'd only show the hint when the checkbox is checked
or 'novnc' is selected in the combobox

also here it shows that the options does not really fit to the display at all,
at least i would not search the setting for the console clipboard under the
virtual display hardware...
(we could just have the gui seperate on the 'options' tab, and leave the option
itself on the display option...)

comments inline

On 10/28/22 14:33, Markus Frank wrote:
> If display is set to spice the checkbox gets replaced by a combobox to
> show the available clipboard options.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   www/manager6/qemu/DisplayEdit.js | 62 +++++++++++++++++++++++++++++++-
>   1 file changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/qemu/DisplayEdit.js b/www/manager6/qemu/DisplayEdit.js
> index 9bb1763e..2cef5ad2 100644
> --- a/www/manager6/qemu/DisplayEdit.js
> +++ b/www/manager6/qemu/DisplayEdit.js
> @@ -33,22 +33,54 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
>   		    return;
>   		}
>   		let memoryfield = this.up('panel').down('field[name=memory]');
> +		let clipboardBox = this.up('panel').down('field[itemId=clipboardBox]');
> +		let clipboardDrop = this.up('panel').down('field[itemId=clipboardDrop]');
> +		let vdagentHint = this.up('panel').down('field[name=vdagentHint]');

at this point, i'd really prefer to either pull the 'panel' into a variable only once
or even rewrite the component using a controller and using references

it's not a huge performance issue, but the it makes it much less readable

also clipboardDrop could be better written as clipboardDropDown

>   		let disableMemoryField = false;
> +		let spice = false;
> +		let showClipboardAndHint = true;
>   
>   		if (val === "cirrus") {
>   		    memoryfield.setEmptyText("4");
> -		} else if (val === "std" || val.match(/^qxl\d?$/) || val === "vmware") {
> +		} else if (val === "std" || val === "vmware") {
>   		    memoryfield.setEmptyText("16");
> +		} else if (val.match(/^qxl\d?$/)) {
> +		    memoryfield.setEmptyText("16");
> +		    spice = true;
>   		} else if (val.match(/^virtio/)) {
>   		    memoryfield.setEmptyText("256");
> +		    spice = true;
>   		} else if (val.match(/^(serial\d|none)$/)) {
>   		    memoryfield.setEmptyText("N/A");
>   		    disableMemoryField = true;
> +		    showClipboardAndHint = false;
>   		} else {
>   		    console.debug("unexpected display type", val);
>   		    memoryfield.setEmptyText(Proxmox.Utils.defaultText);
>   		}
>   		memoryfield.setDisabled(disableMemoryField);
> +		vdagentHint.setVisible(showClipboardAndHint);
> +		if (showClipboardAndHint) {
> +		    // switch from Checkbox to ComboBox and vice versa
> +		    clipboardBox.setDisabled(spice);
> +		    clipboardDrop.setDisabled(!spice);
> +		    clipboardBox.setVisible(!spice);
> +		    clipboardDrop.setVisible(spice);
> +		    // reset value when changing to spice,
> +		    // so that you have to actively change to noVNC Clipboard
> +		    if (spice) {
> +			clipboardDrop.setValue('__default__');
> +		    }
> +		} else {
> +		    // reset to default
> +		    clipboardBox.setValue(false);
> +		    clipboardDrop.setValue('__default__');
> +		    // show only the disabled Checkbox
> +		    clipboardBox.setDisabled(true);
> +		    clipboardDrop.setDisabled(true);
> +		    clipboardBox.setVisible(true);
> +		    clipboardDrop.setVisible(false);
> +		}

i find that whole section a bit too complicated, it took me a few glances
to see what it's actually doing

i'd propose a different approach:

use a 'showClipboardBox' and a 'showClipboardDropDown'
variable to track which you want to show and then only do a

clipboardBox.setDisabled(!showClipboardBox);
clipboardBox.setVisible(!showClipboardBox);

etc.

>   	    },
>   	},
>       },
> @@ -60,6 +92,34 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
>   	maxValue: 512,
>   	step: 4,
>   	name: 'memory',
> +    },
> +    {
> +	xtype: 'proxmoxcheckbox',
> +	fieldLabel: gettext('noVNC Clipboard'),
> +	name: 'clipboard',
> +	itemId: 'clipboardBox',
> +    },
> +    {
> +	name: 'clipboard',
> +	itemId: 'clipboardDrop',
> +	xtype: 'proxmoxKVComboBox',
> +	value: '__default__',
> +	deleteEmpty: false,
> +	fieldLabel: gettext('Clipboard'),
> +	comboItems: [
> +	    ['__default__', 'SPICE-Clipboard'],
> +	    ['1', 'noVNC-Clipboard'],
> +	],
> +	disabled: true,
> +	hidden: true,
> +    },
> +    {
> +	itemId: 'vdagentHint',
> +	name: 'vdagentHint',
> +	xtype: 'displayfield',
> +	userCls: 'pmx-hint',
> +	value: 'Clipboard for noVNC requires spice-tools installed and ' +
> +	    'enabled in the Guest-VM.',
>       }],
>   });
>   

Re: [pve-devel] [PATCH docs v3 6/6] added noVNC clipboard documentation

[Dominik Csapak]

comments inline:

On 10/28/22 14:33, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   qm.adoc | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/qm.adoc b/qm.adoc
> index 4d0c7c4..3a575bc 100644
> --- a/qm.adoc
> +++ b/qm.adoc
> @@ -693,6 +693,17 @@ Selecting `serialX` as display 'type' disables the VGA output, and redirects
>   the Web Console to the selected serial port. A configured display 'memory'
>   setting will be ignored in that case.
>   
> +You can enable the noVNC clipboard by setting 'clipboard' to 1.

please add an example how to do that with e.g. qm set

> +To use this, you need to install and enable spice-vdagent on the VM Guest:
> +`apt install spice-vdagent`

that should be a bit more generic, like 'install the spice guests tools'
as with the option description, spice-vdagent is linux (and probably even debian)
specific

you can ofc mention the package, but in general that sentens should be valid for
all guest os'

> +
> +Doing this will give you the ability to use the clipboard button of the novnc
> +console. However, when using SPICE, you have to decide which clipboard you want
> +to use, because the default SPICE clipboard implementation will be replaced by
> +the qemu-vdagent implementation, which is used by noVNC.
> +This means you cannot simply copy and paste into a SPICE session and instead
> +need to use the noVNC button, when using SPICE with noVNC-clipboard.

IMHO the sentences are too long, and a bit too technical. For an admin it should
be enough to say that if they use a display hardware where they can use the
SPICE console, only either the spice clipboard or novnc clipboard will work
and that they must decide. (how the device is named and what replaces what
is irrelevant for the admin, that's just an implementation detail and
could be changing at any point in time, at which point the docs are wrong)

> +
>   [[qm_usb_passthrough]]
>   USB Passthrough
>   ~~~~~~~~~~~~~~~