From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id D9F451FF0AB for ; Wed, 23 Sep 2026 17:12:29 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 903B82188B; Wed, 23 Sep 2026 17:10:19 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v2 35/50] plugin: correct comment in get_subdir_files helper Date: Wed, 23 Sep 2026 17:05:49 +0200 Message-ID: <20260923150606.531239-36-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260923150606.531239-1-m.carrara@proxmox.com> References: <20260923150606.531239-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: 1790176040096 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.396 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) 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: BCREOR5DLH5XTIVACFKU3ONY7B6CNE4S X-Message-ID-Hash: BCREOR5DLH5XTIVACFKU3ONY7B6CNE4S 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 when *no* `$vmid` is given. If it is given however, it has to match the parsed `$vmid`. Therefore, also include test cases for a plain "some-backup.tar.gz" file that account for these things. Signed-off-by: Max R. Carrara --- src/PVE/Storage/Plugin.pm | 4 +- src/test/list_volumes_test.pm | 457 ++++++++++++++++++++++++++++++++++ 2 files changed, 459 insertions(+), 2 deletions(-) diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm index 0047bc5b..e2f24480 100644 --- a/src/PVE/Storage/Plugin.pm +++ b/src/PVE/Storage/Plugin.pm @@ -1729,8 +1729,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)) { if (defined($parsed_vmid)) { diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm index 1b9be6c5..53ce5116 100644 --- a/src/test/list_volumes_test.pm +++ b/src/test/list_volumes_test.pm @@ -566,6 +566,275 @@ 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 only included if no $vmid is provided. + # 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 only included if no $vmid is provided. + { + file => "$DEFAULT_STORAGE_PATH/dump/some-backup.tar.gz", + expected => undef, + }, + ], + }, { description => 'VMID: none, parent, non-matching', storeid => $DEFAULT_STOREID, @@ -961,6 +1230,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