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 1CA401FF13B for ; Wed, 22 Apr 2026 13:20:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1926A1AC01; Wed, 22 Apr 2026 13:16:04 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v1 45/54] test: volume id: modernize code Date: Wed, 22 Apr 2026 13:13:11 +0200 Message-ID: <20260422111322.257380-46-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: 1776856364084 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.082 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: HJVOE6IEIBTHEGTW4WD7P7QLZMIT7EBJ X-Message-ID-Hash: HJVOE6IEIBTHEGTW4WD7P7QLZMIT7EBJ 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: Modernize the existing code in `path_to_volume_id_test.pm` while leaving the tests unchanged otherwise. In particular, - move the test execution code into its own subroutine - remove needless `diag(explain(...))` call - suppress warnings when calling `path_to_volume_id()` to make test output less noisy - add and call a `main()` subroutine - declare `use v5.36;` instead of `use strict;` and `use warnings;` - capitalize constants - move the comment regarding `File::Temp->newdir()` unlinking on exit to where the method is actually called - adapt the code style to fit our more modern style guide Signed-off-by: Max R. Carrara --- src/test/path_to_volume_id_test.pm | 149 ++++++++++++++++------------- 1 file changed, 82 insertions(+), 67 deletions(-) diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm index e7e36037..72644482 100644 --- a/src/test/path_to_volume_id_test.pm +++ b/src/test/path_to_volume_id_test.pm @@ -1,7 +1,6 @@ package PVE::Storage::TestPathToVolumeId; -use strict; -use warnings; +use v5.36; use lib qw(..); @@ -13,17 +12,17 @@ use PVE::Storage::Common qw( use Test::More; use Cwd; -use File::Basename; +use File::Basename qw(fileparse); use File::Path qw(make_path remove_tree); use File::Temp; -my $storage_dir = File::Temp->newdir(); -my $scfg = { +my $DEFAULT_STORAGE_DIR = File::Temp->newdir(); # unlinks on exit +my $DEFAULT_CFG = { 'digest' => 'd29306346b8b25b90a4a96165f1e8f52d1af1eda', 'ids' => { 'local' => { 'shared' => 0, - 'path' => "$storage_dir", + 'path' => "$DEFAULT_STORAGE_DIR", 'type' => 'dir', 'content' => { 'snippets' => 1, @@ -44,24 +43,24 @@ my $scfg = { # description => to identify the test case # volname => to create the test file # expected => the result that path_to_volume_id should return -my @tests = ( +my $tests = [ { description => 'Image, qcow2', - volname => "$storage_dir/images/16110/vm-16110-disk-0.qcow2", + volname => "$DEFAULT_STORAGE_DIR/images/16110/vm-16110-disk-0.qcow2", expected => [ 'images', 'local:16110/vm-16110-disk-0.qcow2', ], }, { description => 'Image, raw', - volname => "$storage_dir/images/16112/vm-16112-disk-0.raw", + volname => "$DEFAULT_STORAGE_DIR/images/16112/vm-16112-disk-0.raw", expected => [ 'images', 'local:16112/vm-16112-disk-0.raw', ], }, { description => 'Image template, qcow2', - volname => "$storage_dir/images/9004/base-9004-disk-0.qcow2", + volname => "$DEFAULT_STORAGE_DIR/images/9004/base-9004-disk-0.qcow2", expected => [ 'images', 'local:9004/base-9004-disk-0.qcow2', ], @@ -69,49 +68,49 @@ my @tests = ( { description => 'Backup, vma.gz', - volname => "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz", expected => [ 'backup', 'local:backup/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', ], }, { description => 'Backup, vma.lzo', - volname => "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo", expected => [ 'backup', 'local:backup/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', ], }, { description => 'Backup, vma', - volname => "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma", expected => [ 'backup', 'local:backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma', ], }, { description => 'Backup, tar.lzo', - volname => "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo", expected => [ 'backup', 'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', ], }, { description => 'Backup, vma.zst', - volname => "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst", expected => [ 'backup', 'local:backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', ], }, { description => 'Backup, tar.zst', - volname => "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_39_30.tar.zst", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-lxc-16112-2020_03_30-21_39_30.tar.zst", expected => [ 'backup', 'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.zst', ], }, { description => 'Backup, tar.bz2', - volname => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", expected => [ 'backup', 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', ], @@ -119,21 +118,23 @@ my @tests = ( { description => 'ISO file', - volname => "$storage_dir/template/iso/yet-again-a-installation-disk.iso", + volname => "$DEFAULT_STORAGE_DIR/template/iso/yet-again-a-installation-disk.iso", expected => [ 'iso', 'local:iso/yet-again-a-installation-disk.iso', ], }, { description => 'CT template, tar.gz', - volname => "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.gz", + volname => + "$DEFAULT_STORAGE_DIR/template/cache/debian-10.0-standard_10.0-1_amd64.tar.gz", expected => [ 'vztmpl', 'local:vztmpl/debian-10.0-standard_10.0-1_amd64.tar.gz', ], }, { description => 'CT template, wrong ending, tar bz2', - volname => "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2", + volname => + "$DEFAULT_STORAGE_DIR/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2", expected => [ 'vztmpl', 'local:vztmpl/debian-10.0-standard_10.0-1_amd64.tar.bz2', ], @@ -141,49 +142,50 @@ my @tests = ( { description => 'Rootdir, folder subvol, legacy naming', - volname => "$storage_dir/images/1234/subvol-1234-disk-0.subvol/", # fileparse needs / at the end + volname => "$DEFAULT_STORAGE_DIR/images/1234/subvol-1234-disk-0.subvol/", # fileparse needs / at the end expected => [ 'images', 'local:1234/subvol-1234-disk-0.subvol', ], }, { description => 'Rootdir, folder subvol', - volname => "$storage_dir/images/1234/subvol-1234-disk-0.subvol/", # fileparse needs / at the end + volname => "$DEFAULT_STORAGE_DIR/images/1234/subvol-1234-disk-0.subvol/", # fileparse needs / at the end expected => [ 'images', 'local:1234/subvol-1234-disk-0.subvol', ], }, { description => 'Snippets, yaml', - volname => "$storage_dir/snippets/userconfig.yaml", + volname => "$DEFAULT_STORAGE_DIR/snippets/userconfig.yaml", expected => [ 'snippets', 'local:snippets/userconfig.yaml', ], }, { description => 'Snippets, hookscript', - volname => "$storage_dir/snippets/hookscript.pl", + volname => "$DEFAULT_STORAGE_DIR/snippets/hookscript.pl", expected => [ 'snippets', 'local:snippets/hookscript.pl', ], }, { description => 'CT template, tar.xz', - volname => "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.xz", + volname => + "$DEFAULT_STORAGE_DIR/template/cache/debian-10.0-standard_10.0-1_amd64.tar.xz", expected => [ 'vztmpl', 'local:vztmpl/debian-10.0-standard_10.0-1_amd64.tar.xz', ], }, { description => 'Import, ova', - volname => "$storage_dir/import/import.ova", + volname => "$DEFAULT_STORAGE_DIR/import/import.ova", expected => [ 'import', 'local:import/import.ova', ], }, { description => 'Import, ovf', - volname => "$storage_dir/import/import.ovf", + volname => "$DEFAULT_STORAGE_DIR/import/import.ovf", expected => [ 'import', 'local:import/import.ovf', ], @@ -192,91 +194,104 @@ my @tests = ( # no matches, path or files with failures { description => 'Base template, string as vmid in folder name', - volname => "$storage_dir/images/ssss/base-4321-disk-0.raw", + volname => "$DEFAULT_STORAGE_DIR/images/ssss/base-4321-disk-0.raw", expected => [''], }, { description => 'ISO file, wrong ending', - volname => "$storage_dir/template/iso/yet-again-a-installation-disk.dvd", + volname => "$DEFAULT_STORAGE_DIR/template/iso/yet-again-a-installation-disk.dvd", expected => [''], }, { description => 'CT template, wrong ending, zip.gz', - volname => "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz", + volname => + "$DEFAULT_STORAGE_DIR/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz", expected => [''], }, { description => 'Backup, wrong format, openvz, zip.gz', - volname => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz", expected => [''], }, { description => 'Backup, wrong format, openvz, tgz.lzo', - volname => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tgz.lzo", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tgz.lzo", expected => [''], }, { description => 'Backup, wrong ending, qemu, vma.xz', - volname => "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz", expected => [''], }, { description => 'Backup, wrong format, qemu, vms.gz', - volname => "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vms.gz", + volname => "$DEFAULT_STORAGE_DIR/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vms.gz", expected => [''], }, { description => 'Image, string as vmid in folder name', - volname => "$storage_dir/images/ssss/vm-1234-disk-0.qcow2", + volname => "$DEFAULT_STORAGE_DIR/images/ssss/vm-1234-disk-0.qcow2", expected => [''], }, { description => 'Import, non ova/ovf/disk image in import dir', - volname => "$storage_dir/import/test.foo", + volname => "$DEFAULT_STORAGE_DIR/import/test.foo", expected => [''], }, -); +]; -plan tests => scalar @tests + 1; +my sub run_tests($tests) { + my $seen_vtype; + my $vtype_subdirs = { map { $_ => 1 } keys plugin_get_default_vtype_subdirs()->%* }; -my $seen_vtype; -my $vtype_subdirs = - { map { $_ => 1 } keys %{ plugin_get_default_vtype_subdirs() } }; + for my $t ($tests->@*) { + my $file = $t->{volname}; + my $expected = $t->{expected}; + my $description = $t->{description}; -foreach my $tt (@tests) { - my $file = $tt->{volname}; - my $expected = $tt->{expected}; - my $description = $tt->{description}; + # prepare environment + my ($name, $dir, $suffix) = fileparse($file); + make_path($dir, { verbose => 1, mode => 0755 }); - # prepare environment - my ($name, $dir, $suffix) = fileparse($file); - make_path($dir, { verbose => 1, mode => 0755 }); + if ($name) { + open(my $fh, ">>", "$file") || die "Error open file: $!"; + close($fh); + } - if ($name) { - open(my $fh, ">>", "$file") || die "Error open file: $!"; - close($fh); + my $got; + eval { + # Suppress warnings here to make output less noisy for certain tests + local $SIG{__WARN__} = sub { }; + + $got = [PVE::Storage::path_to_volume_id($DEFAULT_CFG, $file)]; + }; + $got = $@ if $@; + + is_deeply($got, $expected, $description); + + $seen_vtype->{ $expected->[0] } = 1 + if ($expected->[0] ne '' && scalar($expected->@*) > 1); } - # run tests - my $got; - eval { $got = [PVE::Storage::path_to_volume_id($scfg, $file)] }; - $got = $@ if $@; + # to check if all $vtype_subdirs are defined in path_to_volume_id + # or have a test + # FIXME re-enable after vtype split changes + #is_deeply($seen_vtype, $vtype_subdirs, "vtype_subdir check"); + is_deeply({}, {}, "vtype_subdir check"); - is_deeply($got, $expected, $description) || diag(explain($got)); - - $seen_vtype->{ @$expected[0] } = 1 - if (@$expected[0] ne '' && scalar @$expected > 1); + return; } -# to check if all $vtype_subdirs are defined in path_to_volume_id -# or have a test -# FIXME re-enable after vtype split changes -#is_deeply($seen_vtype, $vtype_subdirs, "vtype_subdir check"); -is_deeply({}, {}, "vtype_subdir check"); +my sub main() { + plan tests => scalar($tests->@*) + 1; -#cleanup -# File::Temp unlinks tempdir on exit + run_tests($tests); -done_testing(); + done_testing(); + + return; +} + +main(); 1; -- 2.47.3