From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 8B9E01FF0B3 for ; Wed, 11 Nov 2026 10:53:34 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 53EDC2163C; Wed, 11 Nov 2026 10:53:33 +0100 (CET) Date: Wed, 11 Nov 2026 10:53:24 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH qemu-server] fix #7173: config: do not write out an empty tag list To: Lucas Barros , pve-devel@lists.proxmox.com References: <20261109135613.94692-1-lucasbarros.kam@gmail.com> In-Reply-To: <20261109135613.94692-1-lucasbarros.kam@gmail.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1794390075.ol4qipttrq.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794390807641 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.403 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NZA4NYIGTI4SPZCKGYMCGD5JSULDD3ZS X-Message-ID-Hash: NZA4NYIGTI4SPZCKGYMCGD5JSULDD3ZS X-MailFrom: f.gruenbichler@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On November 9, 2026 2:56 pm, Lucas Barros wrote: > 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. We could consider changing this and rejecting empty `-list` parameters, and instead require properly deleting? e.g., switch over to use `--delete tags` if the last to-be-set tag list is empty, make the parser ignore keys without a value, and then with 10.0 make empty (either by virtue of being an empty string, or just delimiters) list parameters invalid? It seems like these two patches just add a hacky workaround on top (with rather subtle semantics about adding the invalid value to pending but not to the `current` config).. > 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. >=20 > Signed-off-by: Lucas Barros > --- > A matching patch for pve-container is sent separately. >=20 > 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. >=20 > 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. >=20 > 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 ""'). >=20 > 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: >=20 > without the patches: > $ qm set 9901 --tags foo; pvesh set /nodes//qemu/9901/config --ta= gs '' > $ 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//qemu/9901/config --output-format json | jq -c= '{tags}' > {"tags":" "} >=20 > 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//qemu/9901/config --output-format json | jq -c= '{tags}' > {"tags":null} >=20 > 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. >=20 > '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. >=20 > This patch was prepared with the help of an LLM based coding assistant; > I reviewed the change and ran the tests mentioned above myself. >=20 > 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 >=20 > 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}; > } > =20 > + # an empty tag list carries no information, so do not write out the = key > + delete $conf->{tags} if defined($conf->{tags}) && $conf->{tags} =3D~= m/^[\s,;]*$/; nit: if we keep this approach, this should `split_list` and check that the result is empty, else it risks going out of sync (it already is w.r.t. \0-delimited lists). > + > my $used_volids =3D {}; > =20 > my $cleanup_config =3D sub { > diff --git a/src/test/parse-config-expected/empty-tags.conf b/src/test/pa= rse-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=3D4G > +scsihw: virtio-scsi-pci > +smbios1: uuid=3Dddf91b3f-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=3D4G > +scsihw: virtio-scsi-pci > +smbios1: uuid=3Dddf91b3f-a597-42be-9a7e-fb6421dcd5cd > +sockets: 1 > +tags:=20 > +vmgenid: 0 > diff --git a/src/test/run_parse_config_tests.pl b/src/test/run_parse_conf= ig_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 =3D './parse-config-input'; > my $EXPECTED_DIR =3D './parse-config-expected'; > =20 > # NOTE update when you add/remove tests > -plan tests =3D> 2 * 10; > +plan tests =3D> 2 * 11; > =20 > sub run_tests { > my ($strict) =3D @_; > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20