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 A27021FF13B for ; Wed, 22 Apr 2026 13:16:29 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 99189181C1; Wed, 22 Apr 2026 13:14:34 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v1 27/54] tree-wide: partially replace usages of regexes for 'import' vtype Date: Wed, 22 Apr 2026 13:12:53 +0200 Message-ID: <20260422111322.257380-28-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-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776856344308 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.084 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: LYWR3WYHMN5WFEDMQ62CBGV4NEZK77P6 X-Message-ID-Hash: LYWR3WYHMN5WFEDMQ62CBGV4NEZK77P6 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: Add partial support for the 'import' volume type in `PVE::Storage::Common::Parse`. What remains unsupported right now is parsing volume names for that type, as those require a little more care. Replace most usages of the `PVE::Storage::IMPORT_EXT_RE_1` regex with parsing functions from `::Common::Parse`. Replace the one remaining spot where we use the `PVE::Storage::UPLOAD_IMPORT_EXT_RE_1` regex with a parsing function as well. This regex should now be phased out in a future APIVER + APIAGE bump, like the others in previous commits. Note that the `UPLOAD_IMPORT_EXT_RE_1` regex only exists to handle the special case of excluding .ovf files from the upload / download_url API methods. Instead of adding a separate parser (or a flag etc.) to handle this case, just parse the path of the up-/downloaded file regardless. Then, raise a parameter exception when the `ovf` file extension is encountered. Also add a bunch of test cases that target the `import` volume type for the `list_volumes()` plugin API method. For all of these tests, sort the expected and resulting list items by their volume ID in order to make the output comparison deterministic. Iterate over the declared content / volume types of the mocked storage config instead of using a fixed list of vtypes as well. Signed-off-by: Max R. Carrara --- src/PVE/API2/Storage/Status.pm | 13 ++- src/PVE/Storage.pm | 4 +- src/PVE/Storage/Common/Parse.pm | 13 +++ src/PVE/Storage/Plugin.pm | 9 +- src/test/list_volumes_test.pm | 180 +++++++++++++++++++++++++++++++- 5 files changed, 203 insertions(+), 16 deletions(-) diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm index 03929d26..b578a1ed 100644 --- a/src/PVE/API2/Storage/Status.pm +++ b/src/PVE/API2/Storage/Status.pm @@ -96,13 +96,18 @@ my sub parse_transferred_file_path_extension : prototype($$) { } if ($vtype eq 'import') { - if ( - $path !~ m!${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::UPLOAD_IMPORT_EXT_RE_1$! - ) { + my $parts = parse_path_as_volname_parts($path, $vtype); + + if (!defined($parts)) { raise_param_exc({ filename => "invalid filename or wrong extension" }); } - my $ext = $1; + my $ext = $parts->{ext}; + + if ($ext eq 'ovf') { + raise_param_exc({ filename => "wrong file extension" }); + } + return $ext; } diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm index 20b7bafa..3a716894 100755 --- a/src/PVE/Storage.pm +++ b/src/PVE/Storage.pm @@ -773,9 +773,7 @@ sub path_to_volume_id { } if ($vtype eq 'import') { - return if $filename !~ m!/(${SAFE_CHAR_CLASS_RE}+${IMPORT_EXT_RE_1})$!; - my $name = $1; - return "$sid:import/$name"; + return parse_path_as_volid($sid, $scfg, $path, $vtype); } return; diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm index 33220758..bd7cec13 100644 --- a/src/PVE/Storage/Common/Parse.pm +++ b/src/PVE/Storage/Common/Parse.pm @@ -37,6 +37,8 @@ my @VZTMPL_COMPRESSION_EXTENSIONS = ('gz', 'xz', 'zst', 'bz2'); my @BACKUP_COMPRESSION_EXTENSIONS = ('gz', 'lzo', 'zst', 'bz2'); +my @IMPORT_EXTENSIONS = ('ova', 'ovf', 'qcow2', 'raw', 'vmdk'); + my sub join_to_re_alternations(@list) { return join('|', map { quotemeta } @list); } @@ -45,6 +47,10 @@ my $RE_VZTMPL_COMPRESSION_EXTENSIONS = join_to_re_alternations(@VZTMPL_COMPRESSI my $RE_BACKUP_COMPRESSION_EXTENSIONS = join_to_re_alternations(@BACKUP_COMPRESSION_EXTENSIONS); +my $RE_IMPORT_EXTENSIONS = join_to_re_alternations(@IMPORT_EXTENSIONS); + +my $RE_SAFE_CHAR_CLASS = qr/[a-zA-Z0-9\-\.\+\=\_]/; + my $RE_PARENT_DIR = quotemeta('..'); my $RE_CONTAINS_PARENT_DIR = qr! ( ^$RE_PARENT_DIR/ ) # ../ --> Beginning of path @@ -112,11 +118,18 @@ my $RE_SNIPPETS_FILE_PATH = qr! ) !xn; +my $RE_IMPORT_FILE_PATH = qr! + (? + (? ($RE_SAFE_CHAR_CLASS)+ \. (? $RE_IMPORT_EXTENSIONS) ) + ) +!xn; + my $RE_FILE_PATH_FOR_VTYPE = { iso => qr/^$RE_ISO_FILE_PATH$/, vztmpl => qr/^$RE_VZTMPL_FILE_PATH$/, backup => qr/^$RE_BACKUP_FILE_PATH$/, snippets => qr/^$RE_SNIPPETS_FILE_PATH$/, + import => qr/^$RE_IMPORT_FILE_PATH$/, }; my $RE_VOLNAME_FOR_VTYPE = { diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm index b08e038b..39e5cb0d 100644 --- a/src/PVE/Storage/Plugin.pm +++ b/src/PVE/Storage/Plugin.pm @@ -1768,13 +1768,12 @@ my sub get_subdir_files { } if ($vtype eq 'import') { - return - if $filename !~ - m!/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$!i; + my $parts = parse_path_as_volid_parts($storeid, $scfg, $path, $vtype); + return if !defined($parts); return { - volid => "$storeid:import/$1", - format => "$2", + volid => $parts->{volid}, + format => $parts->{ext}, }; } diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm index 08769027..5cb08880 100644 --- a/src/test/list_volumes_test.pm +++ b/src/test/list_volumes_test.pm @@ -72,6 +72,7 @@ my $scfg = { 'images' => 1, 'snippets' => 1, 'backup' => 1, + 'import' => 1, }, }; @@ -462,6 +463,171 @@ my @tests = ( ], expected => [], # returns empty list }, + { + description => 'VMID: none, valid file names for import', + vmid => undef, + files => [ + "$storage_dir/import/import.ova", + "$storage_dir/import/import.ovf", + "$storage_dir/import/some-disk.qcow2", + "$storage_dir/import/some-disk.vmdk", + "$storage_dir/import/some-raw-disk.raw", + ], + expected => [ + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ova', + size => DEFAULT_SIZE, + volid => "local:import/import.ova", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ovf', + size => DEFAULT_SIZE, + volid => "local:import/import.ovf", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'qcow2', + size => DEFAULT_SIZE, + volid => "local:import/some-disk.qcow2", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'vmdk', + size => DEFAULT_SIZE, + volid => "local:import/some-disk.vmdk", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'raw', + size => DEFAULT_SIZE, + volid => "local:import/some-raw-disk.raw", + }, + ], + }, + { + description => 'VMID: none, non-matching file paths for import', + vmid => undef, + files => [ + # Malformed file names + "$storage_dir/import/import.ovff", + "$storage_dir/import/importova", + "$storage_dir/import/import.ov", + "$storage_dir/import/diskraw", + "$storage_dir/import/diskvmdk", + "$storage_dir/import/disk.invalid", + "$storage_dir/import/.ova", + "$storage_dir/import/.raw", + # Trailing whitespace must not be trimmed + "$storage_dir/import/import.ova\t", + "$storage_dir/import/disk.raw ", + # Whitespace in file name + "$storage_dir/import/something I want to import.ova", + "$storage_dir/import/ .raw", + "$storage_dir/import/ disk .vmdk", + "$storage_dir/import/disk .qcow2", + "$storage_dir/import/ import.ova", + # Unsafe characters in file name + "$storage_dir/import/linux🐧-vm.ova", + "$storage_dir/import/🐪perl-playground🐪.ova", + "$storage_dir/import/fish_<><_<><_<><.ova", + $storage_dir . '/import/C:\\\\Windows\\Path.ova', + # Content inside .ova files may only be specified as part + # of volume names, and may never appear when looked up as + # a file path + "$storage_dir/import/import.ova/disk.qcow2", + "$storage_dir/import/import.ova/disk.raw", + "$storage_dir/import/import.ova/disk.vmdk", + "$storage_dir/import/import.ova/disk.invalid", + ], + expected => [], # returns empty list + }, + { + description => 'VMID: none, weird but valid file names for import', + vmid => undef, + files => [ + "$storage_dir/import/import.ova.ova", + "$storage_dir/import/import.ova.ova.ova", + "$storage_dir/import/import.ova.ova.ova.ova", + "$storage_dir/import/ova.ova", + "$storage_dir/import/ova.ovf", + "$storage_dir/import/ova.vmdk", + "$storage_dir/import/raw.raw.qcow2", + "$storage_dir/import/raw.raw.qcow2.import.qcow2", + "$storage_dir/import/raw.raw.raw.your-boat.ova", + ], + expected => [ + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ova', + size => DEFAULT_SIZE, + volid => "local:import/import.ova.ova", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ova', + size => DEFAULT_SIZE, + volid => "local:import/import.ova.ova.ova", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ova', + size => DEFAULT_SIZE, + volid => "local:import/import.ova.ova.ova.ova", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ova', + size => DEFAULT_SIZE, + volid => "local:import/ova.ova", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ovf', + size => DEFAULT_SIZE, + volid => "local:import/ova.ovf", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'vmdk', + size => DEFAULT_SIZE, + volid => "local:import/ova.vmdk", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'qcow2', + size => DEFAULT_SIZE, + volid => "local:import/raw.raw.qcow2", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'qcow2', + size => DEFAULT_SIZE, + volid => "local:import/raw.raw.qcow2.import.qcow2", + }, + { + content => 'import', + ctime => DEFAULT_CTIME, + format => 'ova', + size => DEFAULT_SIZE, + volid => "local:import/raw.raw.raw.your-boat.ova", + }, + ], + }, ); # provide static vmlist for tests @@ -497,6 +663,10 @@ $mock_fsi->redefine( }, ); +my sub cmp_volinfo_by_volid { + return $a->{volid} cmp $b->{volid}; +} + my $plan = scalar @tests; plan tests => $plan + 1; @@ -520,14 +690,14 @@ plan tests => $plan + 1; { my $sid = 'local'; - my $types = ['rootdir', 'images', 'vztmpl', 'iso', 'backup', 'snippets']; + my $types = [grep { $scfg->{content}->{$_} } keys $scfg->{content}->%*]; my @suffixes = ('qcow2', 'raw', 'vmdk', 'vhdx'); # run through test cases foreach my $tt (@tests) { my $vmid = $tt->{vmid}; my $files = $tt->{files}; - my $expected = $tt->{expected}; + my $expected = [sort cmp_volinfo_by_volid $tt->{expected}->@*]; my $description = $tt->{description}; my $parent = $tt->{parent}; @@ -550,8 +720,10 @@ plan tests => $plan + 1; } } - my $got; - eval { $got = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types) }; + my $got = eval { + my $volume_list = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types); + return [sort cmp_volinfo_by_volid $volume_list->@*]; + }; $got = $@ if $@; is_deeply($got, $expected, $description) || diag(explain($got)); -- 2.47.3