[pve-devel] [PATCH qemu-server, manager v6 0/4] fix #1926 autodetect xtermjs or novnc for VM console

[pve-devel] [PATCH qemu-server, manager v6 0/4] fix #1926 autodetect xtermjs or novnc for VM console

[Aaron Lauterer]

We add a new property in the VM status/current API result that includes
the display configurtion of the VM. This way we can check in the
frontend what to do with it.

I chose a nested return value, as that makes it easier to add/move
additional display properties into it.

Patch 1/4 makes two functions in QemuServer.pm public.


Patch 2/4 adds the new display property. If not explicitly set in the VM
config, it will return the default 'std' value.

Patch 3/4 implements the changes in the UI. The final result isn't
really a lot simpler on the UI side than in V4 where we had the extra
API call to the VM's config directly. Because we still need to wait for
the API call to finish when initially navigating to the VM. But we have
one fewer call.

Patch 4/4 then introduces some changes to make loading of the console
faster if we just navigate in the submenu of a VM itself where we
already have the current status of a VM already cached.

Changes from v5: implement suggestions:

* use get_vga_properties for default VGA
* UI: use helper to determine if serial display

qemu-server: Aaron Lauterer (2):
  QemuServer: make get_vga_properties and extract_version public
  api: status/current: add display property

 PVE/API2/Qemu.pm  | 21 +++++++++++++++++++++
 PVE/QemuServer.pm |  4 ++--
 2 files changed, 23 insertions(+), 2 deletions(-)


manager: Aaron Lauterer (2):
  fix #1926 ui: vm console: autodetect novnc or xtermjs
  ui: console: check on activate if display info for VMs is present

 www/manager6/Utils.js       |  4 +++
 www/manager6/VNCConsole.js  | 60 ++++++++++++++++++++++++++-----------
 www/manager6/qemu/Config.js |  7 ++++-
 3 files changed, 53 insertions(+), 18 deletions(-)


-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server v6 1/4] QemuServer: make get_vga_properties and extract_version public

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
newly introduced with v6

 PVE/QemuServer.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ccdceed..b7ef69b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1159,7 +1159,7 @@ sub kvm_version {
     return $kvm_api_version;
 }
 
-my sub extract_version {
+sub extract_version {
     my ($machine_type, $version) = @_;
     $version = kvm_user_version() if !defined($version);
     return PVE::QemuServer::Machine::extract_version($machine_type, $version)
@@ -3414,7 +3414,7 @@ my sub print_ovmf_drive_commandlines {
     return ("if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code", $var_drive_str);
 }
 
-my sub get_vga_properties {
+sub get_vga_properties {
     my ($conf, $arch, $machine_version, $winversion) = @_;
 
     my $vga = parse_vga($conf->{vga});
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server v6 1/4] QemuServer: make get_vga_properties and extract_version public

[Fiona Ebner]

Am 08.04.25 um 12:37 schrieb Aaron Lauterer:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> newly introduced with v6
> 
>  PVE/QemuServer.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ccdceed..b7ef69b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1159,7 +1159,7 @@ sub kvm_version {
>      return $kvm_api_version;
>  }
>  
> -my sub extract_version {
> +sub extract_version {

This is named too generally to make public. And there is no need to make
it public if you use PVE::QemuServer::Machine::extract_version() for the
new caller in the next patch. We could think about merging this helper
with PVE::QemuServer::Machine::extract_version(), but totally orthogonal
to this series.

>      my ($machine_type, $version) = @_;
>      $version = kvm_user_version() if !defined($version);
>      return PVE::QemuServer::Machine::extract_version($machine_type, $version)
> @@ -3414,7 +3414,7 @@ my sub print_ovmf_drive_commandlines {
>      return ("if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code", $var_drive_str);
>  }
>  
> -my sub get_vga_properties {
> +sub get_vga_properties {
>      my ($conf, $arch, $machine_version, $winversion) = @_;
>  
>      my $vga = parse_vga($conf->{vga});



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server v6 2/4] api: status/current: add display property

[Aaron Lauterer]

This new property returns the configured or default display for a VM.

Instead of a flat property, we use a nested 'type' object that contains
the actual information. This way we can add other properties that belong
to a VM's display in the future without much hassle, to have them all in
one place.
Candidates to be moved into the display property are for example the
spice and clipboard property.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
Getting all the parameters for get_vga_properties added a bit of
boilerplate. Should be get another external call site to it we could
think about moving the boilerplate into get_vga_properties to handle
missing parameters. But for now I would rather not change the
get_vga_properties itself.

changes since v5:
* use QemuServer::get_vga_properties to determine the default
* properties.

 PVE/API2/Qemu.pm | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 626cce4..cfe483c 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3074,6 +3074,16 @@ __PACKAGE__->register_method({
 		enum => ['vnc'],
 		optional => 1,
 	    },
+	    display => {
+		description => "Display settings",
+		type => 'object',
+		properties => {
+		    type => {
+			description => "Display type configured",
+			type => 'string',
+		    },
+		},
+	    },
 	},
     },
     code => sub {
@@ -3087,8 +3097,19 @@ __PACKAGE__->register_method({
 
 	$status->{ha} = PVE::HA::Config::get_service_status("vm:$param->{vmid}");
 
+	my $arch = PVE::QemuServer::Helpers::get_vm_arch($conf);
+	my $kvm_binary = PVE::QemuServer::Helpers::get_command_for_arch($arch);
+	my $kvmver = PVE::QemuServer::Helpers::kvm_user_version($kvm_binary);
+	my $machine_type = PVE::QemuServer::Machine::get_vm_machine($conf, undef, $arch);
+	my $machine_version = PVE::QemuServer::extract_version($machine_type, $kvmver);
+	my $winversion = PVE::QemuServer::Helpers::windows_version($conf->{ostype});
+	my ($default_display, undef) = PVE::QemuServer::get_vga_properties($conf, $arch, $machine_version, $winversion);
+
+	$status->{display}->{type} = $default_display->{type};
 	if ($conf->{vga}) {
 	    my $vga = PVE::QemuServer::parse_vga($conf->{vga});
+	    $status->{display}->{type} = $vga->{type} if defined($vga->{type});
+
 	    my $spice = defined($vga->{type}) && $vga->{type} =~ /^virtio/;
 	    $spice ||= PVE::QemuServer::vga_conf_has_spice($conf->{vga});
 	    $status->{spice} = 1 if $spice;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server v6 2/4] api: status/current: add display property

[Fiona Ebner]

Am 08.04.25 um 12:37 schrieb Aaron Lauterer:
> This new property returns the configured or default display for a VM.
> 
> Instead of a flat property, we use a nested 'type' object that contains
> the actual information. This way we can add other properties that belong
> to a VM's display in the future without much hassle, to have them all in
> one place.
> Candidates to be moved into the display property are for example the
> spice and clipboard property.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> Getting all the parameters for get_vga_properties added a bit of
> boilerplate. Should be get another external call site to it we could
> think about moving the boilerplate into get_vga_properties to handle
> missing parameters. But for now I would rather not change the
> get_vga_properties itself.

Yeah, that's not too nice. FWIW, machine versions 2.X will get dropped
with QEMU 10.0, so at least we'll not require the machine version
anymore then.

> @@ -3087,8 +3097,19 @@ __PACKAGE__->register_method({
>  
>  	$status->{ha} = PVE::HA::Config::get_service_status("vm:$param->{vmid}");
>  
> +	my $arch = PVE::QemuServer::Helpers::get_vm_arch($conf);
> +	my $kvm_binary = PVE::QemuServer::Helpers::get_command_for_arch($arch);
> +	my $kvmver = PVE::QemuServer::Helpers::kvm_user_version($kvm_binary);
> +	my $machine_type = PVE::QemuServer::Machine::get_vm_machine($conf, undef, $arch);
> +	my $machine_version = PVE::QemuServer::extract_version($machine_type, $kvmver);

Please use the PVE::QemuServer::Machine::extract_version() helper here.

> +	my $winversion = PVE::QemuServer::Helpers::windows_version($conf->{ostype});
> +	my ($default_display, undef) = PVE::QemuServer::get_vga_properties($conf, $arch, $machine_version, $winversion);

That's not necessarily the default display. It's only the default if
there is no $conf->{vga}. It /would/ already be the effectively
configured one, except...the helper re-maps 'qxl2', 'qxl3' and 'qxl4' to
'qxl' :/ And because of that re-mapping you'll still need the line below
after parsing.

(The qxl ones shouldn't have been separate types in hindsight, but
rather type=qxl,qxl-display-count=N, but too late for that).

I guess we do want a separate helper get_deafult_vga_type() for just
getting the default and use that here and within get_vga_properties().

> +
> +	$status->{display}->{type} = $default_display->{type};
>  	if ($conf->{vga}) {
>  	    my $vga = PVE::QemuServer::parse_vga($conf->{vga});
> +	    $status->{display}->{type} = $vga->{type} if defined($vga->{type});
> +
>  	    my $spice = defined($vga->{type}) && $vga->{type} =~ /^virtio/;
>  	    $spice ||= PVE::QemuServer::vga_conf_has_spice($conf->{vga});
>  	    $status->{spice} = 1 if $spice;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager v6 3/4] fix #1926 ui: vm console: autodetect novnc or xtermjs

[Aaron Lauterer]

Some users configure their VMs to use serial as their display. The big
benefit is that in combination with the xtermjs remote console, copy &
paste works a lot better than via novnc.

While the console button in the top right allows to manually choose the
console type, the Console in the main submenu of a VM does not.

This patch changes the behavior to not load the console right away if
set to 'kvm'. Instead, the callback from the VM's status/current call
will run the final steps to load the console. Because then we know if it
should be noVNC or xtermjs.

That means that in the VNCConsole component we need to keep track if the
console has been fully set up and is running. The actual code that loads
the console into the iframe is moved into a function, so we cann call it
from the API callback.

One result is that the behavior changes. Loading the console on a VM
will take a little bit longer:
* When then Console is the selected and one switches between VMs, a new
  status/current call is made and on a good connection, the console
  should load just a few moments later.
* When one switches to the console from another submenu of the VM, the
  console will load once the next API call to status/current finished.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes since:
v5:
* introduce PVE.Utils.isSerialDisplay helper
* avoid 'rec' assignment in callback when setting the 'xtermjs' variable
v4:
* use new status/current display property

v3:
* fixed spacing issues
* add 'current' parameter when fetching config as the pending might have
  a different display set

v2:
* change approach and do it in the UI alone by fetching the VM
  config before deciding which console to use

v1:
* set 'autodetect' to always true in 'VNCConsole.js'
* add additional checks in pveproxy
  * only if autodetect is enabled and console is set to 'kvm'
  * username exists and has VM.Console permissions for the guest

 www/manager6/Utils.js       |  4 +++
 www/manager6/VNCConsole.js  | 52 +++++++++++++++++++++++++------------
 www/manager6/qemu/Config.js |  7 ++++-
 3 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index a1bcbe7e..d6fd6a9e 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1986,6 +1986,10 @@ Ext.define('PVE.Utils', {
 	);
 	return Ext.htmlEncode(description);
     },
+
+    isSerialDisplay: function(display) {
+	return display?.startsWith('serial') ?? false;
+    },
 },
 
     singleton: true,
diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
index 9057e447..3371c923 100644
--- a/www/manager6/VNCConsole.js
+++ b/www/manager6/VNCConsole.js
@@ -12,6 +12,36 @@ Ext.define('PVE.noVncConsole', {
     layout: 'fit',
     border: false,
 
+    setupDone: false, // initial setup is done and the session is running
+
+    loadConsole: function(xtermjs, consoleType) {
+	let me = this;
+	if (me.setupDone) {
+	    return;
+	}
+
+	let type = xtermjs ? 'xtermjs' : 'novnc';
+	let sp = Ext.state.Manager.getProvider();
+	if (Ext.isFunction(me.beforeLoad)) {
+	    me.beforeLoad();
+	}
+	let queryDict = {
+	    console: consoleType, // kvm, lxc, upgrade or shell
+	    vmid: me.vmid,
+	    node: me.nodename,
+	    cmd: me.cmd,
+	    'cmd-opts': me.cmdOpts,
+	    resize: sp.get('novnc-scaling', 'scale'),
+	};
+	let box = this.down('[itemid=vncconsole]');
+	queryDict[type] = 1;
+	PVE.Utils.cleanEmptyObjectKeys(queryDict);
+	let url = '/?' + Ext.Object.toQueryString(queryDict);
+	Proxmox.Utils.setErrorMask(me);
+	box.load(url);
+	me.setupDone = true;
+    },
+
     initComponent: function() {
 	var me = this;
 
@@ -29,37 +59,25 @@ Ext.define('PVE.noVncConsole', {
 
 	// always use same iframe, to avoid running several noVnc clients
 	// at same time (to avoid performance problems)
-	var box = Ext.create('Ext.ux.IFrame', { itemid: "vncconsole" });
+	let box = Ext.create('Ext.ux.IFrame', { itemid: "vncconsole" });
 
-	var type = me.xtermjs ? 'xtermjs' : 'novnc';
 	Ext.apply(me, {
 	    items: box,
 	    listeners: {
 		activate: function() {
-		    let sp = Ext.state.Manager.getProvider();
-		    if (Ext.isFunction(me.beforeLoad)) {
-			me.beforeLoad();
+		    if (me.consoleType !== 'kvm') {
+			me.loadConsole(me.xtermjs, me.consoleType);
 		    }
-		    let queryDict = {
-			console: me.consoleType, // kvm, lxc, upgrade or shell
-			vmid: me.vmid,
-			node: me.nodename,
-			cmd: me.cmd,
-			'cmd-opts': me.cmdOpts,
-			resize: sp.get('novnc-scaling', 'scale'),
-		    };
-		    queryDict[type] = 1;
-		    PVE.Utils.cleanEmptyObjectKeys(queryDict);
-		    var url = '/?' + Ext.Object.toQueryString(queryDict);
-		    box.load(url);
 		},
 	    },
 	});
 
+
 	me.callParent();
 
 	me.on('afterrender', function() {
 	    me.focus();
+	    Proxmox.Utils.setErrorMask(me, true);
 	});
     },
 
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index 0699175e..1598adf8 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -5,6 +5,8 @@ Ext.define('PVE.qemu.Config', {
     onlineHelp: 'chapter_virtual_machines',
     userCls: 'proxmox-tags-full',
 
+    referenceHolder: true,
+
     initComponent: function() {
         var me = this;
 	var vm = me.pveSelNode.data;
@@ -283,6 +285,7 @@ Ext.define('PVE.qemu.Config', {
 		vmid: vmid,
 		consoleType: 'kvm',
 		nodename: nodename,
+		reference: 'submenuConsoleBtn',
 	    });
 	}
 
@@ -444,7 +447,7 @@ Ext.define('PVE.qemu.Config', {
 		lock = rec ? rec.data.value : undefined;
 
 		spice = !!s.data.get('spice');
-		xtermjs = !!s.data.get('serial');
+		xtermjs = PVE.Utils.isSerialDisplay(s.data.get('display')?.data.value.type);
 	    }
 
 	    rec = s.data.get('tags');
@@ -467,6 +470,8 @@ Ext.define('PVE.qemu.Config', {
 	    consoleBtn.setEnableSpice(spice);
 	    consoleBtn.setEnableXtermJS(xtermjs);
 
+	    me.lookup('submenuConsoleBtn')?.loadConsole(xtermjs, 'kvm');
+
 	    statusTxt.update({ lock: lock });
 
 	    let guest_running = status === 'running' &&
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager v6 4/4] ui: console: check on activate if display info for VMs is present

[Aaron Lauterer]

If we already have the display information for a VM, we can proceed
loading the correct console (noVNC or xtermjs).

This way, we don't need to wait for the callback of the VM's
status/current API call to finish setting up the console.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v5:
* use new helper to check if serial

 www/manager6/VNCConsole.js | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
index 3371c923..200145c0 100644
--- a/www/manager6/VNCConsole.js
+++ b/www/manager6/VNCConsole.js
@@ -67,6 +67,14 @@ Ext.define('PVE.noVncConsole', {
 		activate: function() {
 		    if (me.consoleType !== 'kvm') {
 			me.loadConsole(me.xtermjs, me.consoleType);
+		    } else {
+			let display = me.up().statusStore.getById('display');
+			if (PVE.Utils.isSerialDisplay(display?.data.value.type)) {
+			    me.xtermjs = true;
+			}
+			if (display) {
+			    me.loadConsole(me.xtermjs, me.consoleType);
+			}
 		    }
 		},
 	    },
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server, manager v6 0/4] fix #1926 autodetect xtermjs or novnc for VM console

[Aaron Lauterer]

sent a v7 
https://lore.proxmox.com/pve-devel/20250408122710.1164112-2-a.lauterer@proxmox.com/T/#m54aae8f7595a1478748c2b61180c46d0cf52e38b

On  2025-04-08  12:37, Aaron Lauterer wrote:
> We add a new property in the VM status/current API result that includes
> the display configurtion of the VM. This way we can check in the
> frontend what to do with it.
> 
> I chose a nested return value, as that makes it easier to add/move
> additional display properties into it.
> 
> Patch 1/4 makes two functions in QemuServer.pm public.
> 
> 
> Patch 2/4 adds the new display property. If not explicitly set in the VM
> config, it will return the default 'std' value.
> 
> Patch 3/4 implements the changes in the UI. The final result isn't
> really a lot simpler on the UI side than in V4 where we had the extra
> API call to the VM's config directly. Because we still need to wait for
> the API call to finish when initially navigating to the VM. But we have
> one fewer call.
> 
> Patch 4/4 then introduces some changes to make loading of the console
> faster if we just navigate in the submenu of a VM itself where we
> already have the current status of a VM already cached.
> 
> Changes from v5: implement suggestions:
> 
> * use get_vga_properties for default VGA
> * UI: use helper to determine if serial display
> 
> qemu-server: Aaron Lauterer (2):
>    QemuServer: make get_vga_properties and extract_version public
>    api: status/current: add display property
> 
>   PVE/API2/Qemu.pm  | 21 +++++++++++++++++++++
>   PVE/QemuServer.pm |  4 ++--
>   2 files changed, 23 insertions(+), 2 deletions(-)
> 
> 
> manager: Aaron Lauterer (2):
>    fix #1926 ui: vm console: autodetect novnc or xtermjs
>    ui: console: check on activate if display info for VMs is present
> 
>   www/manager6/Utils.js       |  4 +++
>   www/manager6/VNCConsole.js  | 60 ++++++++++++++++++++++++++-----------
>   www/manager6/qemu/Config.js |  7 ++++-
>   3 files changed, 53 insertions(+), 18 deletions(-)
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel