[pve-devel] [RFC manager 0/5] GUI: Hardware comments

[pve-devel] [RFC manager 0/5] GUI: Hardware comments

[Matthias Heiserer]

This series is a first attempt to fix 2672 by implementing editable comments
that are displayed in the GUI and stored in the qemu config file. 

It works, but there are several questions:

How should comments be displayed? (GUI)
    I added a third column. IMO, this looks cleaner than the alternative of
    appending the comment to the corresponding config values. However, it 
    requires more space and some special logic.
    
What to do when there is not enough space? (GUI)
    Currently, this case is not handled. A possible solution would be to
    wrap overflowing lines, but I'm not sure what opinions are on that.
    
Should comments of ineditable fields (EFI Disk) be editable? (GUI)
    I suppose yes, but currently they are not.

Currently missing are:
    GUI: Comments in the wizard, i.e. when creating a VM, no comment can be set.
    Detaching disks voids their comment. Probably shouldn't happen, but seems
        a tad complicated.
    The GUI assumes comments (except memory, socket, bios,  machine, scsihw)
        to be URIencoded. This could be verified server-side.

Generally, I tried to keep the code consistent, but didnt always succeed, partly
because of the variations in the original code.


Matthias Heiserer (5):
  GUI: Parser: add comment support
  GUI: Utils: add comment renderer and field provider
  GUI: QEMU Hardware: add comment column
  GUI: QEMU Hardware: multikey support for comments
  GUI: QEMU Hardware: add comment fields to rows

 www/manager6/Parser.js             | 22 +++++++++++++++
 www/manager6/Utils.js              | 39 +++++++++++++++++++++++++++
 www/manager6/qemu/AudioEdit.js     |  3 ++-
 www/manager6/qemu/CDEdit.js        |  7 ++---
 www/manager6/qemu/CIDriveEdit.js   |  2 ++
 www/manager6/qemu/DisplayEdit.js   |  2 +-
 www/manager6/qemu/HDEdit.js        |  4 +++
 www/manager6/qemu/HDEfi.js         |  3 +++
 www/manager6/qemu/HDTPM.js         |  2 ++
 www/manager6/qemu/HardwareView.js  | 43 +++++++++++++++++++++++++++++-
 www/manager6/qemu/MachineEdit.js   | 24 ++++++++++-------
 www/manager6/qemu/MemoryEdit.js    |  3 +++
 www/manager6/qemu/NetworkEdit.js   |  7 +++--
 www/manager6/qemu/PCIEdit.js       |  1 +
 www/manager6/qemu/ProcessorEdit.js |  1 +
 www/manager6/qemu/QemuBiosEdit.js  |  1 +
 www/manager6/qemu/RNGEdit.js       |  1 +
 www/manager6/qemu/ScsiHwEdit.js    | 15 ++++++-----
 www/manager6/qemu/SerialEdit.js    |  6 ++---
 www/manager6/qemu/USBEdit.js       | 13 ++++++++-
 20 files changed, 171 insertions(+), 28 deletions(-)

-- 
2.30.2

[pve-devel] [RFC manager 1/5] GUI: Parser: add comment support

[Matthias Heiserer]

Comments are automatically (en|de)coded in QemuNetwork,
PropertyString or QemuDrive. For fields not using these,
(en|de)coding utility functions are provided.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/Parser.js | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
index 9f7b2c84..e52c7688 100644
--- a/www/manager6/Parser.js
+++ b/www/manager6/Parser.js
@@ -60,6 +60,9 @@ Ext.define('PVE.Parser', {
 	    value.split(',').forEach(property => {
 		let [k, v] = property.split('=', 2);
 		if (Ext.isDefined(v)) {
+		    if (k === 'comment') {
+			v = PVE.Parser.parseComment(v);
+		    }
 		    res[k] = v;
 		} else if (Ext.isDefined(defaultKey)) {
 		    if (Ext.isDefined(res[defaultKey])) {
@@ -84,6 +87,9 @@ Ext.define('PVE.Parser', {
 	    defaultKeyVal;
 
 	Ext.Object.each(data, function(key, value) {
+	    if (key === 'comment') {
+		value = PVE.Parser.encodeComment(value);
+	    }
 	    if (defaultKey !== undefined && key === defaultKey) {
 		gotDefaultKeyVal = true;
 		defaultKeyVal = value;
@@ -135,6 +141,8 @@ Ext.define('PVE.Parser', {
 		res.trunks = match_res[1];
 	    } else if ((match_res = p.match(/^mtu=(\d+)$/)) !== null) {
 		res.mtu = match_res[1];
+	    } else if ((match_res = p.match(/^comment=(.*)$/)) !== null) {
+		res.comment = PVE.Parser.parseComment(match_res[1]);
 	    } else {
 		errors = true;
 		return false; // break
@@ -178,6 +186,9 @@ Ext.define('PVE.Parser', {
 	if (net.mtu) {
 	    netstr += ",mtu=" + net.mtu;
 	}
+	if (net.comment) {
+	    netstr += ",comment=" + PVE.Parser.encodeComment(net.comment);
+	}
 	return netstr;
     },
 
@@ -239,6 +250,9 @@ Ext.define('PVE.Parser', {
 	var drivestr = drive.file;
 
 	Ext.Object.each(drive, function(key, value) {
+	    if (key === 'comment') {
+		value = PVE.Parser.encodeComment(value);
+	    }
 	    if (!Ext.isDefined(value) || key === 'file' ||
 		key === 'index' || key === 'interface') {
 		return; // continue
@@ -601,5 +615,13 @@ Ext.define('PVE.Parser', {
 	});
 	return [res, extradata];
     },
+
+    parseComment: function(comment) {
+	return decodeURIComponent(comment ?? '');
+    },
+
+    encodeComment: function(comment) {
+	return encodeURIComponent(comment);
+    },
 },
 });
-- 
2.30.2

[pve-devel] [RFC qemu-server 1/2] QEMU: add comment helper

[Matthias Heiserer]

Makes creation of comment fields centralized and simple

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 PVE/QemuServer/Helpers.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
index c10d842..b753b1c 100644
--- a/PVE/QemuServer/Helpers.pm
+++ b/PVE/QemuServer/Helpers.pm
@@ -12,6 +12,7 @@ use base 'Exporter';
 our @EXPORT_OK = qw(
 min_version
 config_aware_timeout
+make_comment_fmt
 );
 
 my $nodename = PVE::INotify::nodename();
@@ -161,4 +162,14 @@ sub config_aware_timeout {
     return $timeout;
 }
 
+sub make_comment_fmt {
+    # GUI assumes this to be URIComponent encoded
+     return {
+	type => 'string',
+	description => 'user-supplied comment',
+	format_description => 'URIcomponent-encoded string',
+	optional => 1,
+    }
+}
+
 1;
-- 
2.30.2

[pve-devel] [RFC manager 2/5] GUI: Utils: add comment renderer and field provider

[Matthias Heiserer]

These render functions are used when displaying comments in a
separate column.
The `commentField` utility simplifies creation of comments.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/Utils.js | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index aafe359a..2d03f611 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1803,6 +1803,45 @@ Ext.define('PVE.Utils', {
 
 	return undefined;
     },
+
+    renderComment: function(key, metaData, rec, rowIndex, colIndex, store, pending) {
+	if (rec && rec.data && rec.data.value) {
+	    let properties = PVE.Parser.parsePropertyString(rec.data.value, rec.data.key);
+	    if (properties && properties.comment) {
+		return properties.comment;
+	    }
+	}
+	// Try to extract a comment from the separate fields
+	// This is needed for single value fields like bios and memory
+	if (rec && rec.id) {
+	    let hw_name = rec.id;
+	    let comment = this.getObjectValue(`${hw_name}_comment`, undefined);
+	    if (comment) {
+		return comment;
+	    }
+	}
+	return PVE.Utils.noneText;
+    },
+
+    renderValueWithoutComment: function(context) {
+	return function(value, ...args) {
+	    if (value && typeof value === "string") {
+		value = value.replace(/comment=[^,]*(,|$)/, "");
+	    }
+	    return this.renderValue(value, ...args);
+	}.bind(context);
+    },
+
+    commentField: function(name = 'comment') {
+	return {
+	    xtype: 'textfield',
+	    name: name,
+	    fieldLabel: gettext('comment'),
+	    allowBlank: true,
+	    emptyText: '',
+	    defaultValue: '',
+	};
+    },
 },
 
     singleton: true,
-- 
2.30.2

[pve-devel] [RFC qemu-server 2/2] QEMU: add comment fields

[Matthias Heiserer]

This patch adds comment fields to all values shown in the
QEMU/Hardware GUI.
As I tried to keep changes minimal, this is achieved in two
different ways:
 - adding `comment` as field. Easiest/simplest option,
    but not always possible, when data is stored as a single
    int (e.g memory). Although the UI stores the data as a
    URIComponent-encoded string, this is not yet checked in
    the perl code.

 - adding a separate `<name>_comment` field. Not that great,
    but it works. One advantage of this is that we don't have
    to encode/decode the data, but many new fields bloat the config.
    Changing this seems to require some more rewriting.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 PVE/QemuServer.pm           | 49 ++++++++++++++++++++++++++++++++++---
 PVE/QemuServer/CPUConfig.pm |  7 ++++--
 PVE/QemuServer/Drive.pm     |  6 ++++-
 PVE/QemuServer/PCI.pm       |  4 ++-
 4 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0071a06..3b2abfe 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -44,7 +44,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 
 use PVE::QMPClient;
 use PVE::QemuConfig;
-use PVE::QemuServer::Helpers qw(min_version config_aware_timeout);
+use PVE::QemuServer::Helpers qw(min_version config_aware_timeout make_comment_fmt);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
 use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
@@ -205,6 +205,7 @@ my $vga_fmt = {
 	minimum => 4,
 	maximum => 512,
     },
+    comment => make_comment_fmt(),
 };
 
 my $ivshmem_fmt = {
@@ -235,6 +236,7 @@ my $audio_fmt = {
 	optional => 1,
 	description => "Driver backend for the audio device."
     },
+    comment => make_comment_fmt(),
 };
 
 my $spice_enhancements_fmt = {
@@ -284,6 +286,7 @@ my $rng_fmt = {
 	optional => 1,
 	default => 1000,
     },
+    comment => make_comment_fmt(),
 };
 
 my $meta_info_fmt = {
@@ -728,6 +731,31 @@ EODESCR
 	description => "Some (read-only) meta-information about this guest.",
 	optional => 1,
     },
+    sockets_comment => {
+	type => 'string',
+	optional => 1,
+	description => 'Comment on cpu',
+    },
+    memory_comment => {
+	type => 'string',
+	optional => 1,
+	description => 'Comment on memory',
+    },
+    bios_comment => {
+	type => 'string',
+	optional => 1,
+	description => 'Comment on bios',
+    },
+    machine_comment => {
+	type => 'string',
+	optional => 1,
+	description => 'Comment on machine',
+    },
+    scsihw_comment => {
+	type => 'string',
+	optional => 1,
+	description => 'Comment on SCSI controller',
+    },
 };
 
 my $cicustom_fmt = {
@@ -977,6 +1005,7 @@ my $net_fmt = {
 	description => "Force MTU, for VirtIO only. Set to '1' to use the bridge MTU",
 	optional => 1,
     },
+    comment => make_comment_fmt(),
 };
 
 my $netdesc = {
@@ -1093,6 +1122,7 @@ EODESCR
 	description => "Specifies whether if given host option is a USB3 device or port.",
         default => 0,
     },
+    comment => make_comment_fmt(),
 };
 
 my $usbdesc = {
@@ -1102,12 +1132,23 @@ my $usbdesc = {
 };
 PVE::JSONSchema::register_standard_option("pve-qm-usb", $usbdesc);
 
-my $serialdesc = {
+my $serial_fmt = {
+    serial => {
+	default_key => 1,
 	optional => 1,
 	type => 'string',
 	pattern => '(/dev/.+|socket)',
-	description =>  "Create a serial device inside the VM (n is 0 to 3)",
-	verbose_description =>  <<EODESCR,
+	format_description => '(/dev/.+|socket)',
+    },
+    comment => make_comment_fmt(),
+};
+
+my $serialdesc = {
+    optional => 1,
+    type => 'string',
+    format => $serial_fmt,
+    description =>  "Create a serial device inside the VM (n is 0 to 3)",
+    verbose_description =>  <<EODESCR,
 Create a serial device inside the VM (n is 0 to 3), and pass through a
 host serial device (i.e. /dev/ttyS0), or create a unix socket on the
 host side (use 'qm terminal' to open a terminal connection).
diff --git a/PVE/QemuServer/CPUConfig.pm b/PVE/QemuServer/CPUConfig.pm
index b9981c8..d83c1e9 100644
--- a/PVE/QemuServer/CPUConfig.pm
+++ b/PVE/QemuServer/CPUConfig.pm
@@ -5,7 +5,7 @@ use warnings;
 
 use PVE::JSONSchema;
 use PVE::Cluster qw(cfs_register_file cfs_read_file);
-use PVE::QemuServer::Helpers qw(min_version);
+use PVE::QemuServer::Helpers qw(min_version make_comment_fmt);
 
 use base qw(PVE::SectionConfig Exporter);
 
@@ -160,6 +160,7 @@ my $cpu_fmt = {
 		     . " doing so will break live migration to CPUs with other values.",
 	optional => 1,
     },
+    comment => make_comment_fmt(),
 };
 
 PVE::JSONSchema::register_format('pve-phys-bits', \&parse_phys_bits);
@@ -368,7 +369,9 @@ sub print_cpu_device {
     my $current_core = ($id - 1) % $cores;
     my $current_socket = int(($id - 1 - $current_core)/$cores);
 
-    return "$cpu-x86_64-cpu,id=cpu$id,socket-id=$current_socket,core-id=$current_core,thread-id=0";
+    my $comment = $conf->{comment} || '';
+
+    return "$cpu-x86_64-cpu,id=cpu$id,socket-id=$current_socket,core-id=$current_core,thread-id=0,comment=$comment";
 }
 
 # Resolves multiple arrays of hashes representing CPU flags with metadata to a
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 7b82fb2..708d95e 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use PVE::Storage;
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::QemuServer::Helpers qw(make_comment_fmt);
 
 use base qw(Exporter);
 
@@ -146,7 +147,8 @@ my %drivedesc_base = (
 	verbose_description => "Mark this locally-managed volume as available on all nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is shared already!",
 	optional => 1,
 	default => 0,
-    }
+    },
+    comment => make_comment_fmt(),
 );
 
 my %iothread_fmt = ( iothread => {
@@ -353,6 +355,7 @@ my $efidisk_fmt = {
 	description => "Disk size. This is purely informational and has no effect.",
 	optional => 1,
     },
+    comment => make_comment_fmt(),
     %efitype_fmt,
 };
 
@@ -393,6 +396,7 @@ my $tpmstate_fmt = {
 	optional => 1,
     },
     %tpmversion_fmt,
+    comment => make_comment_fmt(),
 };
 my $tpmstate_desc = {
     optional => 1,
diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
index 70987d8..a961bda 100644
--- a/PVE/QemuServer/PCI.pm
+++ b/PVE/QemuServer/PCI.pm
@@ -6,6 +6,7 @@ use strict;
 use PVE::JSONSchema;
 use PVE::SysFSTools;
 use PVE::Tools;
+use PVE::QemuServer::Helpers qw(make_comment_fmt);
 
 use base 'Exporter';
 
@@ -105,7 +106,8 @@ EODESCR
 	format_description => 'hex id',
 	optional => 1,
 	description => "Override PCI subsystem device ID visible to guest"
-    }
+    },
+    comment => make_comment_fmt(),
 };
 PVE::JSONSchema::register_format('pve-qm-hostpci', $hostpci_fmt);
 
-- 
2.30.2

[pve-devel] [RFC manager 3/5] GUI: QEMU Hardware: add comment column

[Matthias Heiserer]

Adds a column in which comments can be displayed.
This column is to the right of the values column.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 6cea4287..60d662d5 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -787,6 +787,29 @@ Ext.define('PVE.qemu.HardwareView', {
 	    },
 	});
 
+	Ext.applyIf(me, {
+		columns: [
+		    {
+			header: gettext('Name'),
+			width: me.cwidth1 || 200,
+			dataIndex: 'key',
+			renderer: me.renderKey,
+		    },
+		    {
+			flex: 1,
+			header: gettext('Value'),
+			dataIndex: 'value',
+			renderer: PVE.Utils.renderValueWithoutComment(me),
+		    }, {
+			header: gettext('Comment'),
+			flex: 1,
+			dataIndex: 'comment',
+			renderer: PVE.Utils.renderComment,
+		    },
+		],
+	    });
+
+
 	me.callParent();
 
 	me.on('activate', me.rstore.startUpdate, me.rstore);
-- 
2.30.2

[pve-devel] [RFC manager 4/5] GUI: QEMU Hardware: multikey support for comments

[Matthias Heiserer]

Values in multikey need to be explicitly set `visible: false`,
otherwise they are not found.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 60d662d5..ca6dac76 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -98,7 +98,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		tdCls: 'pmx-itype-icon-processor',
 		group: 3,
 		defaultValue: '1',
-		multiKey: ['sockets', 'cpu', 'cores', 'numa', 'vcpus', 'cpulimit', 'cpuunits'],
+		multiKey: ['sockets', 'cpu', 'cores', 'numa', 'vcpus', 'cpulimit', 'cpuunits', 'cpucomment'],
 		renderer: function(value, metaData, record, rowIndex, colIndex, store, pending) {
 		    var sockets = me.getObjectValue('sockets', 1, pending);
 		    var model = me.getObjectValue('cpu', undefined, pending);
@@ -213,6 +213,21 @@ Ext.define('PVE.qemu.HardwareView', {
 	    ostype: {
 		visible: false,
 	    },
+	    sockets_comment: {
+		visible: false,
+	    },
+	    memory_comment: {
+		visible: false,
+	    },
+	    bios_comment: {
+		visible: false,
+	    },
+	    machine_comment: {
+		visible: false,
+	    },
+	    scsihw_comment: {
+		visible: false,
+	    },
 	};
 
 	PVE.Utils.forEachBus(undefined, function(type, id) {
@@ -283,6 +298,9 @@ Ext.define('PVE.qemu.HardwareView', {
 		never_delete: !caps.nodes['Sys.Console'],
 		header: gettext('Serial Port') + ' (' + confid + ')',
 	    };
+	    rows[confid + '_comment'] = {
+		visible: false,
+	    };
 	}
 	rows.audio0 = {
 	    group: 40,
-- 
2.30.2

[pve-devel] [RFC manager 5/5] GUI: QEMU Hardware: add comment fields to rows

[Matthias Heiserer]

This patch adds comment support to all qemu/hardware items.
Mostly, these are  property-string based fields.
As processor(sockets), memory, bios, machine, and scsihw are
multikey fields, comments haev to be sent in separate fields.

I tried to keep the number of changes minimal, which means that
the various files differ to some extent in how comments are
implemented (e.g. USBEdit). Streamlining them might be a good idea.

Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
 www/manager6/qemu/AudioEdit.js     |  3 ++-
 www/manager6/qemu/CDEdit.js        |  7 ++++---
 www/manager6/qemu/CIDriveEdit.js   |  2 ++
 www/manager6/qemu/DisplayEdit.js   |  2 +-
 www/manager6/qemu/HDEdit.js        |  4 ++++
 www/manager6/qemu/HDEfi.js         |  3 +++
 www/manager6/qemu/HDTPM.js         |  2 ++
 www/manager6/qemu/MachineEdit.js   | 24 ++++++++++++++----------
 www/manager6/qemu/MemoryEdit.js    |  3 +++
 www/manager6/qemu/NetworkEdit.js   |  7 +++++--
 www/manager6/qemu/PCIEdit.js       |  1 +
 www/manager6/qemu/ProcessorEdit.js |  1 +
 www/manager6/qemu/QemuBiosEdit.js  |  1 +
 www/manager6/qemu/RNGEdit.js       |  1 +
 www/manager6/qemu/ScsiHwEdit.js    | 15 +++++++++------
 www/manager6/qemu/SerialEdit.js    |  6 +++---
 www/manager6/qemu/USBEdit.js       | 13 ++++++++++++-
 17 files changed, 68 insertions(+), 27 deletions(-)

diff --git a/www/manager6/qemu/AudioEdit.js b/www/manager6/qemu/AudioEdit.js
index e7861ceb..f657f8f6 100644
--- a/www/manager6/qemu/AudioEdit.js
+++ b/www/manager6/qemu/AudioEdit.js
@@ -36,7 +36,8 @@ Ext.define('PVE.qemu.AudioInputPanel', {
 	    ['spice', 'SPICE'],
 	    ['none', `${Proxmox.Utils.NoneText} (${gettext('Dummy Device')})`],
 	],
-    }],
+    },
+    PVE.Utils.commentField()],
 });
 
 Ext.define('PVE.qemu.AudioEdit', {
diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js
index 72c01037..ec1f3388 100644
--- a/www/manager6/qemu/CDEdit.js
+++ b/www/manager6/qemu/CDEdit.js
@@ -17,7 +17,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	} else {
 	    me.drive.file = 'none';
 	}
-
+	PVE.Utils.propertyStringSet(me.drive, values.comment, 'comment');
 	var params = {};
 
 	params[confid] = PVE.Parser.printQemuDrive(me.drive);
@@ -49,9 +49,8 @@ Ext.define('PVE.qemu.CDInputPanel', {
 		values.cdimage = drive.file;
 	    }
 	}
-
+	values.comment = PVE.Parser.parseComment(drive.comment);
 	me.drive = drive;
-
 	me.setValues(values);
     },
 
@@ -140,6 +139,8 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	    boxLabel: gettext('Do not use any media'),
 	});
 
+	items.push(PVE.Utils.commentField());
+
 	me.items = items;
 
 	me.callParent();
diff --git a/www/manager6/qemu/CIDriveEdit.js b/www/manager6/qemu/CIDriveEdit.js
index 754b8353..1f5ee37b 100644
--- a/www/manager6/qemu/CIDriveEdit.js
+++ b/www/manager6/qemu/CIDriveEdit.js
@@ -13,6 +13,7 @@ Ext.define('PVE.qemu.CIDriveInputPanel', {
 	var params = {};
 	drive.file = values.hdstorage + ":cloudinit";
 	drive.format = values.diskformat;
+	drive.comment = values.comment;
 	params[values.controller + values.deviceid] = PVE.Parser.printQemuDrive(drive);
 	return params;
     },
@@ -48,6 +49,7 @@ Ext.define('PVE.qemu.CIDriveInputPanel', {
 		nodename: me.nodename,
 		hideSize: true,
 	    },
+	    PVE.Utils.commentField(),
 	];
 	me.callParent();
     },
diff --git a/www/manager6/qemu/DisplayEdit.js b/www/manager6/qemu/DisplayEdit.js
index 82e6777e..0f03101a 100644
--- a/www/manager6/qemu/DisplayEdit.js
+++ b/www/manager6/qemu/DisplayEdit.js
@@ -83,7 +83,7 @@ Ext.define('PVE.qemu.DisplayInputPanel', {
 	maxValue: 512,
 	step: 4,
 	name: 'memory',
-    }],
+    }, PVE.Utils.commentField()],
 });
 
 Ext.define('PVE.qemu.DisplayEdit', {
diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index c643ee73..bffdee48 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -93,6 +93,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	PVE.Utils.propertyStringSet(me.drive, values.readOnly, 'ro', 'on');
 	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
 	PVE.Utils.propertyStringSet(me.drive, values.aio, 'aio');
+	PVE.Utils.propertyStringSet(me.drive, values.comment, 'comment');
 
 	['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'].forEach(name => {
 	    let burst_name = `${name}_max`;
@@ -153,6 +154,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	values.iothread = PVE.Parser.parseBoolean(drive.iothread);
 	values.readOnly = PVE.Parser.parseBoolean(drive.ro);
 	values.aio = drive.aio || '__default__';
+	values.comment = PVE.Parser.parseComment(drive.comment) || '';
 
 	values.mbps_rd = drive.mbps_rd;
 	values.mbps_wr = drive.mbps_wr;
@@ -238,6 +240,8 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	    });
 	}
 
+	column1.push(PVE.Utils.commentField());
+
 	column2.push(
 	    {
 		xtype: 'CacheTypeSelector',
diff --git a/www/manager6/qemu/HDEfi.js b/www/manager6/qemu/HDEfi.js
index a8ca8f56..7d16540b 100644
--- a/www/manager6/qemu/HDEfi.js
+++ b/www/manager6/qemu/HDEfi.js
@@ -17,6 +17,8 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
 
 	var confid = 'efidisk0';
 
+	me.drive.comment = values.comment;
+
 	if (values.hdimage) {
 	    me.drive.file = values.hdimage;
 	} else {
@@ -76,6 +78,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
 		    'data-qtip': gettext('Use EFIvars image with standard distribution and Microsoft secure boot keys enrolled.'),
 		},
 	    },
+	    PVE.Utils.commentField(),
 	    {
 		xtype: 'label',
 		text: gettext("Warning: The VM currently does not uses 'OVMF (UEFI)' as BIOS."),
diff --git a/www/manager6/qemu/HDTPM.js b/www/manager6/qemu/HDTPM.js
index 87349aed..0b4a6431 100644
--- a/www/manager6/qemu/HDTPM.js
+++ b/www/manager6/qemu/HDTPM.js
@@ -13,6 +13,7 @@ Ext.define('PVE.qemu.TPMDiskInputPanel', {
 	}
 
 	var confid = 'tpmstate0';
+	me.drive.comment = values.comment;
 
 	if (values.hdimage) {
 	    me.drive.file = values.hdimage;
@@ -68,6 +69,7 @@ Ext.define('PVE.qemu.TPMDiskInputPanel', {
 		    ['v2.0', 'v2.0'],
 		],
 	    },
+	    PVE.Utils.commentField(),
 	];
 
 	me.callParent();
diff --git a/www/manager6/qemu/MachineEdit.js b/www/manager6/qemu/MachineEdit.js
index f928c80c..61540d55 100644
--- a/www/manager6/qemu/MachineEdit.js
+++ b/www/manager6/qemu/MachineEdit.js
@@ -69,16 +69,19 @@ Ext.define('PVE.qemu.MachineInputPanel', {
 	this.callParent(arguments);
     },
 
-    items: {
-	xtype: 'proxmoxKVComboBox',
-	name: 'machine',
-	reference: 'machine',
-	fieldLabel: gettext('Machine'),
-	comboItems: [
-	    ['__default__', PVE.Utils.render_qemu_machine('')],
-	    ['q35', 'q35'],
-	],
-    },
+    items: [
+	{
+	    xtype: 'proxmoxKVComboBox',
+	    name: 'machine',
+	    reference: 'machine',
+	    fieldLabel: gettext('Machine'),
+	    comboItems: [
+		['__default__', PVE.Utils.render_qemu_machine('')],
+		['q35', 'q35'],
+	    ],
+	},
+	PVE.Utils.commentField('machine_comment'),
+    ],
 
     advancedItems: [
 	{
@@ -137,6 +140,7 @@ Ext.define('PVE.qemu.MachineEdit', {
 		let conf = response.result.data;
 		let values = {
 		    machine: conf.machine || '__default__',
+		    machine_comment: conf.machine_comment,
 		};
 		values.isWindows = PVE.Utils.is_windows(conf.ostype);
 		me.setValues(values);
diff --git a/www/manager6/qemu/MemoryEdit.js b/www/manager6/qemu/MemoryEdit.js
index 5e91dc9b..aa5c6b4d 100644
--- a/www/manager6/qemu/MemoryEdit.js
+++ b/www/manager6/qemu/MemoryEdit.js
@@ -38,6 +38,7 @@ Ext.define('PVE.qemu.MemoryInputPanel', {
 
 	res.memory = values.memory;
 	res.balloon = values.balloon;
+	res.memory_comment = values.memory_comment;
 
 	if (!values.ballooning) {
 	    res.balloon = 0;
@@ -80,6 +81,7 @@ Ext.define('PVE.qemu.MemoryInputPanel', {
 		    },
 		},
 	    },
+	    PVE.Utils.commentField('memory_comment'),
 	];
 
 	me.advancedItems= [
@@ -183,6 +185,7 @@ Ext.define('PVE.qemu.MemoryEdit', {
 		    shares: data.shares,
 		    memory: data.memory || '512',
 		    balloon: data.balloon > 0 ? data.balloon : data.memory || '512',
+		    memory_comment: data.memory_comment,
 		};
 
 		ipanel.setValues(values);
diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
index b39cffdc..074d3006 100644
--- a/www/manager6/qemu/NetworkEdit.js
+++ b/www/manager6/qemu/NetworkEdit.js
@@ -19,7 +19,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
 	me.network.macaddr = values.macaddr;
 	me.network.disconnect = values.disconnect;
 	me.network.queues = values.queues;
-
+	me.network.comment = values.comment;
 	if (values.rate) {
 	    me.network.rate = values.rate;
 	} else {
@@ -140,7 +140,10 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
 		vtype: 'MacAddress',
 		allowBlank: true,
 		emptyText: 'auto',
-	    });
+	    },
+	    PVE.Utils.commentField(),
+	);
+
 	me.advancedColumn2 = [
 	    {
 		xtype: 'numberfield',
diff --git a/www/manager6/qemu/PCIEdit.js b/www/manager6/qemu/PCIEdit.js
index f505e34f..ac556c7a 100644
--- a/www/manager6/qemu/PCIEdit.js
+++ b/www/manager6/qemu/PCIEdit.js
@@ -151,6 +151,7 @@ Ext.define('PVE.qemu.PCIInputPanel', {
 		fieldLabel: gettext('All Functions'),
 		name: 'multifunction',
 	    },
+	    PVE.Utils.commentField(),
 	];
 
 	me.column2 = [
diff --git a/www/manager6/qemu/ProcessorEdit.js b/www/manager6/qemu/ProcessorEdit.js
index 539e3e7d..f7d1c2e2 100644
--- a/www/manager6/qemu/ProcessorEdit.js
+++ b/www/manager6/qemu/ProcessorEdit.js
@@ -121,6 +121,7 @@ Ext.define('PVE.qemu.ProcessorInputPanel', {
 		value: '{coreCount}',
 	    },
 	},
+	PVE.Utils.commentField('sockets_comment'),
     ],
 
     column2: [
diff --git a/www/manager6/qemu/QemuBiosEdit.js b/www/manager6/qemu/QemuBiosEdit.js
index 70731a71..228375c2 100644
--- a/www/manager6/qemu/QemuBiosEdit.js
+++ b/www/manager6/qemu/QemuBiosEdit.js
@@ -25,6 +25,7 @@ Ext.define('PVE.qemu.BiosEdit', {
 	    bind: '{bios}',
 	    fieldLabel: 'BIOS',
 	},
+	PVE.Utils.commentField('bios_comment'),
 	{
 	    xtype: 'displayfield',
 	    name: 'efidisk0',
diff --git a/www/manager6/qemu/RNGEdit.js b/www/manager6/qemu/RNGEdit.js
index e34e2c08..e9aa488f 100644
--- a/www/manager6/qemu/RNGEdit.js
+++ b/www/manager6/qemu/RNGEdit.js
@@ -77,6 +77,7 @@ Ext.define('PVE.qemu.RNGInputPanel', {
 	labelWidth: 130,
 	emptyText: '1000',
     },
+    PVE.Utils.commentField(),
     {
 	xtype: 'displayfield',
 	reference: 'sourceWarning',
diff --git a/www/manager6/qemu/ScsiHwEdit.js b/www/manager6/qemu/ScsiHwEdit.js
index 70f09c17..edd66808 100644
--- a/www/manager6/qemu/ScsiHwEdit.js
+++ b/www/manager6/qemu/ScsiHwEdit.js
@@ -6,12 +6,15 @@ Ext.define('PVE.qemu.ScsiHwEdit', {
 
 	Ext.applyIf(me, {
 	    subject: gettext('SCSI Controller Type'),
-	    items: {
-		xtype: 'pveScsiHwSelector',
-		name: 'scsihw',
-		value: '__default__',
-		fieldLabel: gettext('Type'),
-	    },
+	    items: [
+		{
+		    xtype: 'pveScsiHwSelector',
+		    name: 'scsihw',
+		    value: '__default__',
+		    fieldLabel: gettext('Type'),
+		},
+		PVE.Utils.commentField('scsihw_comment'),
+	    ],
 	});
 
 	me.callParent();
diff --git a/www/manager6/qemu/SerialEdit.js b/www/manager6/qemu/SerialEdit.js
index 6ce18ec2..730fe2c3 100644
--- a/www/manager6/qemu/SerialEdit.js
+++ b/www/manager6/qemu/SerialEdit.js
@@ -17,12 +17,11 @@ Ext.define('PVE.qemu.SerialnputPanel', {
     },
 
     onGetValues: function(values) {
-	var me = this;
-
 	var id = 'serial' + values.serialid;
 	delete values.serialid;
 	values[id] = 'socket';
-	return values;
+
+	return { [id]: PVE.Parser.printPropertyString(values, id) };
     },
 
     items: [
@@ -44,6 +43,7 @@ Ext.define('PVE.qemu.SerialnputPanel', {
 		return true;
 	    },
 	},
+	PVE.Utils.commentField(),
     ],
 });
 
diff --git a/www/manager6/qemu/USBEdit.js b/www/manager6/qemu/USBEdit.js
index a2204584..817693ec 100644
--- a/www/manager6/qemu/USBEdit.js
+++ b/www/manager6/qemu/USBEdit.js
@@ -45,6 +45,8 @@ Ext.define('PVE.qemu.USBInputPanel', {
 	    val += ',usb3=1';
 	}
 	values[me.confid] = val;
+	values[me.confid] += ',comment=' + PVE.Parser.encodeComment(values.comment);
+	delete values.comment;
 	return values;
     },
 
@@ -107,6 +109,7 @@ Ext.define('PVE.qemu.USBInputPanel', {
 		    reference: 'usb3',
 		    fieldLabel: gettext('Use USB3'),
 		},
+		PVE.Utils.commentField(),
 	    ],
 	},
     ],
@@ -145,7 +148,7 @@ Ext.define('PVE.qemu.USBEdit', {
 		}
 
 		var data = response.result.data[me.confid].split(',');
-		var port, hostdevice, usb3 = false;
+		var port, hostdevice, usb3, comment = false;
 		var type = 'spice';
 
 		for (let i = 0; i < data.length; i++) {
@@ -162,12 +165,20 @@ Ext.define('PVE.qemu.USBEdit', {
 		    if (/^usb3=(1|on|true)$/.test(data[i])) {
 			usb3 = true;
 		    }
+
+		    // FIXME: use PVE.Parser.parsePropertyString
+		    if (/comment=([^,]*)(,|$)/.test(data[i])) {
+			comment = data[i];
+			comment = comment.replace('comment=', '');
+			comment = PVE.Parser.parseComment(comment);
+		    }
 		}
 		var values = {
 		    usb: type,
 		    hostdevice: hostdevice,
 		    port: port,
 		    usb3: usb3,
+		    comment: comment,
 		};
 
 		ipanel.setValues(values);
-- 
2.30.2

Re: [pve-devel] [RFC manager 0/5] GUI: Hardware comments

[Thomas Lamprecht]

On 14.02.22 15:01, Matthias Heiserer wrote:
> This series is a first attempt to fix 2672 by implementing editable comments
> that are displayed in the GUI and stored in the qemu config file. 

I know Dominik's comment suggests this features is "ack'd" but I'm really not
sure about that I'd ack it, at least not in such a overly general form that adds
such things every where. We already have the guest notes for general comments about
bios/machine/... settings, bug 2672 is actually only asking for giving nic's and
disks some tag-like property to allow humans easier to determine what it's used for.

> 
> It works, but there are several questions:
> 
> How should comments be displayed? (GUI)
>     I added a third column. IMO, this looks cleaner than the alternative of
>     appending the comment to the corresponding config values. However, it 
>     requires more space and some special logic.

No, I really would not go that way for now, especially as I don't want such
comments on every level, if they're more restricted and tag like they can
just be rendered with existing infra.

>     
> What to do when there is not enough space? (GUI)
>     Currently, this case is not handled. A possible solution would be to
>     wrap overflowing lines, but I'm not sure what opinions are on that.

see above, no special handling required.

>     
> Should comments of ineditable fields (EFI Disk) be editable? (GUI)
>     I suppose yes, but currently they are not.

no, one can only have one efidisk, so the admin cannot be confused by multiple
efi disk entries, so they don't require a comment, the general guest section
is already more than enough for this-

> 
> Currently missing are:
>     GUI: Comments in the wizard, i.e. when creating a VM, no comment can be set.

for disks maybe ok, at least since we can add multiple disks from the start now
already, for all else no, please don't overload the user with inputs fields which
most people never need.

>     Detaching disks voids their comment. Probably shouldn't happen, but seems
>         a tad complicated.

that is unrelated of comments, detaching disks voids a lot of stuff which isn't
ideal and is separate from such a series.

>     The GUI assumes comments (except memory, socket, bios,  machine, scsihw)
>         to be URIencoded. This could be verified server-side.

just restrict them to a simpler format and be done..

Something along [\w-]{,32} (check what we already use elsewhere first)

What's missing: container support.


patch submit tipp: if you group the filenames of the format-patch exported patches
by repo before letting git send-email process them, they'd be also ordered/grouped
by that in the MTA's thread view per default, making it easier to review/apply.