[PATCH qemu-server] fix #7173: config: do not write out an empty tag list

[PATCH qemu-server] fix #7173: config: do not write out an empty tag list

[Lucas Barros]

Removing the last tag of a VM, for example in the web UI, sets 'tags'
to an empty string, which then ends up as an empty 'tags:' line in the
VM config and in the output of 'qm config'. This carries no
information, is inconsistent with how unset options are handled and
confuses tools that consume the config, as they need to special case
an empty tag list.

Drop the key when writing the config if the tag list is empty, which
also cleans up configs that already contain such a line on the next
write. The pending section is left alone, as an empty value there is
still needed to apply the change.

Signed-off-by: Lucas Barros <lucasbarros.kam@gmail.com>
---
A matching patch for pve-container is sent separately.

Only the main section is touched. In [PENDING] the empty value is still
needed so that applying the pending change removes the tags, and it is
dropped from the main section on the write that follows.

The new parse-config test input contains the 'tags: ' line with the
trailing space that the writer produces today, so 'git am' warns about
trailing whitespace there. Without the trailing space the line would
not match the config parser at all.

An alternative would be to turn an empty 'tags' value into a deletion
in the update API, but that would not cover other paths that end up
with an empty value (create, clone, restore, 'qm set --tags ""').

Tested on a PVE 9 node (qemu-server 9.2.15, pve-container 6.1.18),
adding and then removing the only tag of a VM and of a container:

without the patches:
  $ qm set 9901 --tags foo; pvesh set /nodes/<node>/qemu/9901/config --tags ''
  $ grep -n tags /etc/pve/qemu-server/9901.conf | cat -A
  6:tags:  $
  $ pct set 9902 --tags foo; pct set 9902 --tags ''
  $ grep -n tags /etc/pve/lxc/9902.conf | cat -A
  7:tags: $
  $ pvesh get /nodes/<node>/qemu/9901/config --output-format json | jq -c '{tags}'
  {"tags":" "}

with the patches:
  $ grep -c tags /etc/pve/qemu-server/9901.conf /etc/pve/lxc/9902.conf
  /etc/pve/qemu-server/9901.conf:0
  /etc/pve/lxc/9902.conf:0
  $ pvesh get /nodes/<node>/qemu/9901/config --output-format json | jq -c '{tags}'
  {"tags":null}

An existing empty 'tags:' line goes away on the next write of the
config, removing the tag of a running VM leaves no line and no
[PENDING] entry, and 'tags: a;b' is still written as before.

'make test' (run with 16 CPUs in /proc/cpuinfo, the cfg2cmd tests need
more vCPUs than the test machine has) passes before and after; the
parse-config tests go from 20 to 22, and the new empty-tags case fails
without the change to QemuServer.pm. proxmox-perltidy reports no
changes.

This patch was prepared with the help of an LLM based coding assistant;
I reviewed the change and ran the tests mentioned above myself.

 src/PVE/QemuServer.pm                          |  3 +++
 src/test/parse-config-expected/empty-tags.conf | 11 +++++++++++
 src/test/parse-config-input/empty-tags.conf    | 12 ++++++++++++
 src/test/run_parse_config_tests.pl             |  2 +-
 4 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 src/test/parse-config-expected/empty-tags.conf
 create mode 100644 src/test/parse-config-input/empty-tags.conf

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 8045c07e..22621e63 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -2067,6 +2067,9 @@ sub write_vm_config {
         delete $conf->{smp};
     }
 
+    # an empty tag list carries no information, so do not write out the key
+    delete $conf->{tags} if defined($conf->{tags}) && $conf->{tags} =~ m/^[\s,;]*$/;
+
     my $used_volids = {};
 
     my $cleanup_config = sub {
diff --git a/src/test/parse-config-expected/empty-tags.conf b/src/test/parse-config-expected/empty-tags.conf
new file mode 100644
index 00000000..a5b02f2c
--- /dev/null
+++ b/src/test/parse-config-expected/empty-tags.conf
@@ -0,0 +1,11 @@
+# VM with an empty tag list left behind after removing the last tag
+bootdisk: scsi0
+cores: 1
+memory: 512
+name: apache
+ostype: l26
+scsi0: mydir:142/vm-142-disk-0.qcow2,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+vmgenid: 0
diff --git a/src/test/parse-config-input/empty-tags.conf b/src/test/parse-config-input/empty-tags.conf
new file mode 100644
index 00000000..a37a9c2d
--- /dev/null
+++ b/src/test/parse-config-input/empty-tags.conf
@@ -0,0 +1,12 @@
+# VM with an empty tag list left behind after removing the last tag
+bootdisk: scsi0
+cores: 1
+memory: 512
+name: apache
+ostype: l26
+scsi0: mydir:142/vm-142-disk-0.qcow2,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+tags: 
+vmgenid: 0
diff --git a/src/test/run_parse_config_tests.pl b/src/test/run_parse_config_tests.pl
index f68abc06..c30aad99 100755
--- a/src/test/run_parse_config_tests.pl
+++ b/src/test/run_parse_config_tests.pl
@@ -25,7 +25,7 @@ my $INPUT_DIR = './parse-config-input';
 my $EXPECTED_DIR = './parse-config-expected';
 
 # NOTE update when you add/remove tests
-plan tests => 2 * 10;
+plan tests => 2 * 11;
 
 sub run_tests {
     my ($strict) = @_;
-- 
2.47.3