From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id D146E1FF13B for ; Wed, 22 Apr 2026 13:17:52 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 61A3F18E97; Wed, 22 Apr 2026 13:14:46 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v1 39/54] plugin: correct comment in get_subdir_files helper Date: Wed, 22 Apr 2026 13:13:05 +0200 Message-ID: <20260422111322.257380-40-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260422111322.257380-1-m.carrara@proxmox.com> References: <20260422111322.257380-1-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776856357521 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.083 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: DK2B4UVC567S52FBXJ445CGPZPVGPXFK X-Message-ID-Hash: DK2B4UVC567S52FBXJ445CGPZPVGPXFK X-MailFrom: m.carrara@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: The comment in the branch for 'backup' volume types in the `get_subdir_files()` helper states that the condition below checks for "false positives", apparently meaning that the VMID in the parent directory might have been matched. Challenge this claim by adding several new test cases that try to cause the parser to return a wrong result. As it turns out, the check the comment refers to is not (anymore) there to check for "false positives" or VMIDs appearing in the parent directory, but instead simply filters out backups that do not belong to the provided VMID. Note that if a backup has an arbitrary file name, that is, it's not named something like "vzdump-qemu-1337-$TIMESTAMP.vma", that backup is still returned in all cases. Therefore, also include test cases for a plain "some-backup.tar.gz" file and ensure that it is included in the expected output. Signed-off-by: Max R. Carrara --- src/PVE/Storage/Plugin.pm | 4 +- src/test/list_volumes_test.pm | 466 ++++++++++++++++++++++++++++++++++ 2 files changed, 468 insertions(+), 2 deletions(-) diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm index 3853682b..101e0b6d 100644 --- a/src/PVE/Storage/Plugin.pm +++ b/src/PVE/Storage/Plugin.pm @@ -1702,8 +1702,8 @@ my sub get_subdir_files { my $format = $parts->{ext}; my $volume_path = $parts->{path}; - # Check if parsed VMID matched provided VMID in order to avoid - # false positives (VMID in parent directory name) + # Check if parsed VMID matches provided VMID in order to avoid + # returning backups of other guests my $parsed_vmid = $parts->{vmid}; if (defined($vmid) && defined($parsed_vmid)) { return if $vmid ne $parsed_vmid; diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm index 455fb227..ce35c782 100644 --- a/src/test/list_volumes_test.pm +++ b/src/test/list_volumes_test.pm @@ -566,6 +566,284 @@ my $test_param_list = [ }, ], }, + { + description => 'VMID: none, backups of all guests', + storeid => $DEFAULT_STOREID, + scfg => $DEFAULT_SCFG, + vmid => undef, + vtypes => ['backup'], + cases => [ + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz", + expected => { + content => 'backup', + ctime => 1585602700, + format => 'vma.gz', + size => $DEFAULT_SIZE, + subtype => 'qemu', + vmid => '16110', + volid => 'local:backup/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo", + expected => { + content => 'backup', + ctime => 1585602765, + format => 'vma.lzo', + size => $DEFAULT_SIZE, + subtype => 'qemu', + vmid => '16110', + volid => 'local:backup/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', + }, + }, + { + file => "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma", + expected => { + content => 'backup', + ctime => 1585602835, + format => 'vma', + size => $DEFAULT_SIZE, + subtype => 'qemu', + vmid => '16110', + volid => 'local:backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo", + expected => { + content => 'backup', + ctime => 1585604370, + format => 'tar.lzo', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz", + expected => { + content => 'backup', + ctime => 1585604970, + format => 'tar.gz', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst", + expected => { + content => 'backup', + ctime => 1585604970, + format => 'tar.zst', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', + }, + }, + { + file => "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_59_30.tgz", + expected => { + content => 'backup', + ctime => 1585605570, + format => 'tgz', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", + expected => { + content => 'backup', + ctime => 1585604370, + format => 'tar.bz2', + size => $DEFAULT_SIZE, + subtype => 'openvz', + vmid => '16112', + volid => 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst", + expected => { + content => 'backup', + ctime => 1585602835, + format => 'vma.zst', + size => $DEFAULT_SIZE, + subtype => 'qemu', + vmid => '16110', + volid => 'local:backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-19253-2020_02_03-19_57_43.tar.gz", + expected => { + content => 'backup', + ctime => 1580759863, + format => 'tar.gz', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '19253', + volid => 'local:backup/vzdump-lxc-19253-2020_02_03-19_57_43.tar.gz', + }, + }, + { + file => "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-19254-2019_01_21-19_29_19.tar", + expected => { + content => 'backup', + ctime => 1548098959, + format => 'tar', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '19254', + volid => 'local:backup/vzdump-lxc-19254-2019_01_21-19_29_19.tar', + }, + }, + # Arbitrary backups are always included. + # Note that in this case, the 'vmid' key does not exist at all, + # instead of being set to undef. + { + file => "$DEFAULT_STORAGE_PATH/dump/some-backup.tar.gz", + expected => { + content => 'backup', + ctime => $DEFAULT_CTIME, + format => 'tar.gz', + size => $DEFAULT_SIZE, + subtype => 'unknown', + volid => 'local:backup/some-backup.tar.gz', + }, + }, + ], + }, + { + description => 'VMID: 16112, backups of specific guest', + storeid => $DEFAULT_STOREID, + scfg => $DEFAULT_SCFG, + vmid => 16112, + vtypes => ['backup'], + cases => [ + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz", + expected => undef, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo", + expected => undef, + }, + { + file => "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma", + expected => undef, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo", + expected => { + content => 'backup', + ctime => 1585604370, + format => 'tar.lzo', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz", + expected => { + content => 'backup', + ctime => 1585604970, + format => 'tar.gz', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst", + expected => { + content => 'backup', + ctime => 1585604970, + format => 'tar.zst', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', + }, + }, + { + file => "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-16112-2020_03_30-21_59_30.tgz", + expected => { + content => 'backup', + ctime => 1585605570, + format => 'tgz', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '16112', + volid => 'local:backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", + expected => { + content => 'backup', + ctime => 1585604370, + format => 'tar.bz2', + size => $DEFAULT_SIZE, + subtype => 'openvz', + vmid => '16112', + volid => 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', + }, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst", + expected => undef, + }, + { + file => + "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-19253-2020_02_03-19_57_43.tar.gz", + expected => undef, + }, + { + file => "$DEFAULT_STORAGE_PATH/dump/vzdump-lxc-19254-2019_01_21-19_29_19.tar", + expected => undef, + }, + # Arbitrary backups are always included. + # In this case the 'vmid' also gets set to the provided one. + { + file => "$DEFAULT_STORAGE_PATH/dump/some-backup.tar.gz", + expected => { + content => 'backup', + ctime => $DEFAULT_CTIME, + format => 'tar.gz', + size => $DEFAULT_SIZE, + subtype => 'unknown', + vmid => 16112, + volid => 'local:backup/some-backup.tar.gz', + }, + }, + ], + }, { description => 'VMID: none, parent, non-matching', storeid => $DEFAULT_STOREID, @@ -1020,6 +1298,194 @@ my $test_param_list = [ }, ]; +# Additional test cases that cannot be constructed within the list above +{ + my $file_name = "vzdump-qemu-16110-2020_03_30-21_13_55.vma"; + my $storage_path = File::Temp->newdir() . '/' . $file_name; + + my $backup_vmid_test_params = { + description => "VMID: 16110, file name in path of storage", + storeid => $DEFAULT_STOREID, + scfg => { + type => 'dir', + path => $storage_path, + shared => 0, + content => { + backup => 1, + }, + }, + vmid => 16110, + vtypes => ['backup'], + cases => [ + { + file => "$storage_path/dump/$file_name", + expected => { + content => 'backup', + ctime => 1585602835, + format => 'vma', + size => $DEFAULT_SIZE, + subtype => 'qemu', + vmid => '16110', + volid => "local:backup/$file_name", + }, + }, + ], + }; + + push($test_param_list->@*, $backup_vmid_test_params); +} + +{ + my $file_name = "vzdump-qemu-16110-2020_03_30-21_13_55.vma"; + my $storage_path = File::Temp->newdir(); + + my $backup_vmid_test_params = { + description => "VMID: 16110, file name in vtype subdir of 'backup' vtype", + storeid => $DEFAULT_STOREID, + scfg => { + type => 'dir', + path => $storage_path, + shared => 0, + content => { + backup => 1, + }, + 'content-dirs' => { + backup => $file_name, + }, + }, + vmid => 16110, + vtypes => ['backup'], + cases => [ + { + file => "$storage_path/$file_name/$file_name", + expected => { + content => 'backup', + ctime => 1585602835, + format => 'vma', + size => $DEFAULT_SIZE, + subtype => 'qemu', + vmid => '16110', + volid => "local:backup/$file_name", + }, + }, + ], + }; + + push($test_param_list->@*, $backup_vmid_test_params); +} + +{ + my $file_name = "vzdump-qemu-16110-2020_03_30-21_13_55.vma"; + my $storage_path = File::Temp->newdir() . '/' . $file_name; + + my $backup_vmid_test_params = { + description => "VMID: 16110, file name in storage path and subdir of 'backup' vtype", + storeid => $DEFAULT_STOREID, + scfg => { + type => 'dir', + path => $storage_path, + shared => 0, + content => { + backup => 1, + }, + 'content-dirs' => { + backup => $file_name, + }, + }, + vmid => 16110, + vtypes => ['backup'], + cases => [ + { + file => "$storage_path/$file_name/$file_name", + expected => { + content => 'backup', + ctime => 1585602835, + format => 'vma', + size => $DEFAULT_SIZE, + subtype => 'qemu', + vmid => '16110', + volid => "local:backup/$file_name", + }, + }, + ], + }; + + push($test_param_list->@*, $backup_vmid_test_params); +} + +{ + my $file_name = "vzdump-qemu-16110-2020_03_30-21_13_55.vma"; + my $storage_path = File::Temp->newdir() . '/' . $file_name; + + my $backup_vmid_test_params = { + description => + "VMID: 19253, file name with different VMID in storage path and subdir of 'backup' vtype", + storeid => $DEFAULT_STOREID, + scfg => { + type => 'dir', + path => $storage_path, + shared => 0, + content => { + backup => 1, + }, + 'content-dirs' => { + backup => $file_name, + }, + }, + vmid => 19253, + vtypes => ['backup'], + cases => [ + { + file => "$storage_path/$file_name/vzdump-lxc-19253-2020_02_03-19_57_43.tar.gz", + expected => { + content => 'backup', + ctime => 1580759863, + format => 'tar.gz', + size => $DEFAULT_SIZE, + subtype => 'lxc', + vmid => '19253', + volid => 'local:backup/vzdump-lxc-19253-2020_02_03-19_57_43.tar.gz', + }, + }, + ], + }; + + push($test_param_list->@*, $backup_vmid_test_params); +} + +{ + my $file_name = "vzdump-qemu-16110-2020_03_30-21_13_55.vma"; + my $storage_path = File::Temp->newdir() . '/' . $file_name; + + my $backup_vmid_test_params = { + description => + "VMID: none, file name in storage path and subdir of 'backup' vtype, no backups", + storeid => $DEFAULT_STOREID, + scfg => { + type => 'dir', + path => $storage_path, + shared => 0, + content => { + backup => 1, + snippets => 1, + }, + 'content-dirs' => { + backup => $file_name, + }, + }, + vmid => 19253, + vtypes => ['backup'], + cases => [ + { + file => "$storage_path/snippets/hookscript.pl", + expected => undef, + }, + ], + }; + + push($test_param_list->@*, $backup_vmid_test_params); +} + # provide static vmlist for tests my $mock_cluster = Test::MockModule->new('PVE::Cluster', no_auto => 1); $mock_cluster->redefine(get_vmlist => sub { return $mocked_vmlist; }); -- 2.47.3