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 CB74E1FF13B for ; Wed, 22 Apr 2026 13:17:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B485E18AC2; Wed, 22 Apr 2026 13:14:42 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v1 28/54] tree-wide: replace remaining usages of regexes for 'import' vtype Date: Wed, 22 Apr 2026 13:12:54 +0200 Message-ID: <20260422111322.257380-29-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: 1776856345403 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: LVYH7523QUSWUZXXCDLYWX65ZZ7N2ZVH X-Message-ID-Hash: LVYH7523QUSWUZXXCDLYWX65ZZ7N2ZVH 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: Implement the remaining pieces that were missing from fully supporting the 'import' volume type in `PVE::Storage::Common::Parse`. In particular, parsing volume names for '.ova' files that refer to contents inside the '.ova' file is now fully supported. Also add corresponding test cases to 'parser_tests.pl'. Replace the remaining usages of the `PVE::Storage::IMPORT_EXT_RE_1` and `PVE::Storage::OVA_CONTENT_RE_1` with parsing functions from `::Common::Parse` across the repository. Note that the logic in `PVE::Storage::Plugin::parse_volname` can now be simplified a little further, since the `parse_volname_as_parts()` parsing function also handles extracting the inner and outer file extensions, amongst other parts. Since the replaced regexes are now completely unused inside `PVE::Storage` and family, they should be phased out with a future APIVER + APIAGE bump, like the regexes in previous commits. Finally, update the test cases in 'guest_import_test.pl' that match for specific parsing-related exception messages. Signed-off-by: Max R. Carrara --- src/PVE/GuestImport.pm | 28 +- src/PVE/Storage/Common/Parse.pm | 45 ++++ src/PVE/Storage/Common/test/parser_tests.pl | 281 ++++++++++++++++++++ src/PVE/Storage/Plugin.pm | 20 +- src/test/guest_import_test.pl | 6 +- 5 files changed, 350 insertions(+), 30 deletions(-) diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm index 3d59dcd7..3be84fc9 100644 --- a/src/PVE/GuestImport.pm +++ b/src/PVE/GuestImport.pm @@ -6,6 +6,9 @@ use warnings; use File::Path; use PVE::Storage; +use PVE::Storage::Common::Parse qw( + parse_volname_as_parts +); use PVE::Tools qw(run_command); sub extract_disk_from_import_file { @@ -13,31 +16,26 @@ sub extract_disk_from_import_file { my ($source_storeid, $volname) = PVE::Storage::parse_volume_id($volid); $target_storeid //= $source_storeid; - my $cfg = PVE::Storage::config(); - my ($vtype, $name, undef, undef, undef, undef, $fmt) = - PVE::Storage::parse_volname($cfg, $volid); + my $parts = parse_volname_as_parts($volname); + die "cannot extract $volid - invalid volname $volname\n" + if !defined($parts); + + my ($vtype, $outer_fmt) = $parts->@{qw(vtype ext)}; die "only files with content type 'import' can be extracted\n" if $vtype ne 'import'; die "only files from 'ova' format can be extracted\n" - if $fmt !~ m/^ova\+/; + if $outer_fmt ne 'ova'; - # extract the inner file from the name - my $archive_volid; - my $inner_file; - my $inner_fmt; - if ($name =~ m!^(.*\.ova)/(${PVE::Storage::SAFE_CHAR_WITH_WHITESPACE_CLASS_RE}+)$!) { - $archive_volid = "$source_storeid:import/$1"; - $inner_file = $2; - ($inner_fmt) = $fmt =~ /^ova\+(.*)$/; - } else { - die "cannot extract $volid - invalid volname $volname\n"; - } + my $archive_volid = $source_storeid . ':' . $vtype . '/' . $parts->{'disk-path'}; + my $inner_file = $parts->{'content-file'}; + my $inner_fmt = $parts->{'content-ext'}; die "cannot determine format of '$volid'\n" if !$inner_fmt; + my $cfg = PVE::Storage::config(); my $ova_path = PVE::Storage::path($cfg, $archive_volid); my $tmpdir = PVE::Storage::get_image_dir($cfg, $target_storeid, $vmid); diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm index bd7cec13..9fb45b0c 100644 --- a/src/PVE/Storage/Common/Parse.pm +++ b/src/PVE/Storage/Common/Parse.pm @@ -39,6 +39,8 @@ my @BACKUP_COMPRESSION_EXTENSIONS = ('gz', 'lzo', 'zst', 'bz2'); my @IMPORT_EXTENSIONS = ('ova', 'ovf', 'qcow2', 'raw', 'vmdk'); +my @OVA_CONTENT_EXTENSIONS = ('qcow2', 'raw', 'vmdk'); + my sub join_to_re_alternations(@list) { return join('|', map { quotemeta } @list); } @@ -49,8 +51,12 @@ my $RE_BACKUP_COMPRESSION_EXTENSIONS = join_to_re_alternations(@BACKUP_COMPRESSI my $RE_IMPORT_EXTENSIONS = join_to_re_alternations(@IMPORT_EXTENSIONS); +my $RE_OVA_CONTENT_EXTENSIONS = join_to_re_alternations(@OVA_CONTENT_EXTENSIONS); + my $RE_SAFE_CHAR_CLASS = qr/[a-zA-Z0-9\-\.\+\=\_]/; +my $RE_SAFE_CHAR_WITH_WHITESPACE_CLASS = qr/[ a-zA-Z0-9\-\.\+\=\_]/; + my $RE_PARENT_DIR = quotemeta('..'); my $RE_CONTAINS_PARENT_DIR = qr! ( ^$RE_PARENT_DIR/ ) # ../ --> Beginning of path @@ -124,6 +130,42 @@ my $RE_IMPORT_FILE_PATH = qr! ) !xn; +my $RE_OVA_CONTENT = qr! + (? + ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+ \. (? $RE_OVA_CONTENT_EXTENSIONS) + ) +!xn; + +# NOTE: Volume names with the 'import' vtype are treated differently when +# they do not stem from a file path directly - see comments inline. +my $RE_IMPORT_VOLNAME_OVA_FILE_WITH_CONTENT = qr! + (? + # NOTE: Unlike in RE_IMPORT_FILE_PATH, we allow whitespace here + ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+ + \. + (? ova) + ) / (? $RE_OVA_CONTENT) +!xn; + +my $RE_IMPORT_VOLNAME_REGULAR_FILE = qr! + (? + # NOTE: Unlike in RE_IMPORT_FILE_PATH, we allow whitespace here + ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+ + \. + (? $RE_IMPORT_EXTENSIONS) + ) +!xn; + +my $RE_IMPORT_VOLNAME = qr! + (? + # NOTE: Order here matters - the ova+content regex is stricter, + # so try this branch here first + ($RE_IMPORT_VOLNAME_OVA_FILE_WITH_CONTENT) + | + ($RE_IMPORT_VOLNAME_REGULAR_FILE) + ) +!xn; + my $RE_FILE_PATH_FOR_VTYPE = { iso => qr/^$RE_ISO_FILE_PATH$/, vztmpl => qr/^$RE_VZTMPL_FILE_PATH$/, @@ -137,6 +179,9 @@ my $RE_VOLNAME_FOR_VTYPE = { vztmpl => qr/^$RE_VZTMPL_FILE_PATH$/, backup => qr/^$RE_BACKUP_FILE_PATH$/, snippets => qr/^$RE_SNIPPETS_FILE_PATH$/, + + # special cases - not reusing file path regexes: + import => qr/^$RE_IMPORT_VOLNAME$/, }; my sub contains_parent_dir($path) { diff --git a/src/PVE/Storage/Common/test/parser_tests.pl b/src/PVE/Storage/Common/test/parser_tests.pl index e0029fc6..b6e42307 100755 --- a/src/PVE/Storage/Common/test/parser_tests.pl +++ b/src/PVE/Storage/Common/test/parser_tests.pl @@ -396,17 +396,104 @@ my $volname_cases_snippets_valid = [ }, ]; +my $volname_cases_import_valid = [ + { + path => 'import.ova', + expected => { + file => 'import.ova', + ext => 'ova', + 'disk-path' => 'import.ova', + path => 'import.ova', + vtype => 'import', + volname => 'import/import.ova', + }, + }, + { + path => 'import.ovf', + expected => { + file => 'import.ovf', + ext => 'ovf', + 'disk-path' => 'import.ovf', + path => 'import.ovf', + vtype => 'import', + volname => 'import/import.ovf', + }, + }, + { + path => 'disk-0.qcow2', + expected => { + file => 'disk-0.qcow2', + ext => 'qcow2', + 'disk-path' => 'disk-0.qcow2', + path => 'disk-0.qcow2', + vtype => 'import', + volname => 'import/disk-0.qcow2', + }, + }, + { + path => 'raw_disk_v2.5.raw', + expected => { + file => 'raw_disk_v2.5.raw', + ext => 'raw', + 'disk-path' => 'raw_disk_v2.5.raw', + path => 'raw_disk_v2.5.raw', + vtype => 'import', + volname => 'import/raw_disk_v2.5.raw', + }, + }, + { + path => 'disk-1+data.vmdk', + expected => { + file => 'disk-1+data.vmdk', + ext => 'vmdk', + 'disk-path' => 'disk-1+data.vmdk', + path => 'disk-1+data.vmdk', + vtype => 'import', + volname => 'import/disk-1+data.vmdk', + }, + }, +]; + +my $volname_cases_import_invalid = [ + { + description => "Invalid file extension (import)", + args => { + path => 'import.zip', + vtype => 'import', + }, + expected => undef, + }, + { + description => "Uppercase letter in file extension (import)", + args => { + path => 'import.Ova', + vtype => 'import', + }, + expected => undef, + }, + { + description => "Unsafe characters in file name (import)", + args => { + path => '🐪perl-playground🐪.ova', + vtype => 'import', + }, + expected => undef, + }, +]; + my $cases_valid_all = [ $volname_cases_iso_valid, $volname_cases_vztmpl_valid, $volname_cases_backup_valid, $volname_cases_snippets_valid, + $volname_cases_import_valid, ]; my $cases_invalid_all = [ $volname_cases_iso_invalid, $volname_cases_vztmpl_invalid, $volname_cases_backup_invalid, + $volname_cases_import_invalid, ]; { @@ -474,6 +561,178 @@ my sub run_volname_parsing_tests : prototype($) ($volname_tests) { return; } +# NOTE: These run for parse_path_as_volname_parts() and parse_path_as_volname() only! +my $cases_import_special_file_path = [ + { + description => "Whitespace not allowed in volume file paths (import)", + args => { + path => 'Some Disk.qcow2', + vtype => 'import', + }, + expected => undef, + }, + { + description => "Volume file path with disallowed OVA content (raw) (import)", + args => { + path => 'import.ova/disk.raw', + vtype => 'import', + }, + expected => undef, + }, + { + description => "Volume file path with disallowed OVA content (qcow2) (import)", + args => { + path => 'import.ova/disk.qcow2', + vtype => 'import', + }, + expected => undef, + }, + { + description => "Volume file path with disallowed OVA content (vmdk) (import)", + args => { + path => 'import.ova/disk.vmdk', + vtype => 'import', + }, + expected => undef, + }, +]; + +# NOTE: These run for parse_volname_as_parts() only! +my $cases_import_special_volname = [ + { + description => "Whitespace allowed in volume names (import)", + args => { + volname => 'import/Some Disk.qcow2', + }, + expected => { + file => 'Some Disk.qcow2', + ext => 'qcow2', + 'disk-path' => 'Some Disk.qcow2', + path => 'Some Disk.qcow2', + vtype => 'import', + volname => 'import/Some Disk.qcow2', + }, + }, + { + description => "Volume name with OVA content (raw) (import)", + args => { + volname => 'import/import.ova/disk.raw', + }, + expected => { + file => 'import.ova', + ext => 'ova', + 'disk-path' => 'import.ova', + path => 'import.ova/disk.raw', + content => 'disk.raw', + 'content-file' => 'disk.raw', + 'content-ext' => 'raw', + vtype => 'import', + volname => 'import/import.ova/disk.raw', + }, + }, + { + description => "Volume name with OVA content (qcow2) (import)", + args => { + volname => 'import/import.ova/disk.qcow2', + }, + expected => { + file => 'import.ova', + ext => 'ova', + 'disk-path' => 'import.ova', + path => 'import.ova/disk.qcow2', + content => 'disk.qcow2', + 'content-file' => 'disk.qcow2', + 'content-ext' => 'qcow2', + vtype => 'import', + volname => 'import/import.ova/disk.qcow2', + }, + }, + { + description => "Volume name with OVA content (vmdk) (import)", + args => { + volname => 'import/import.ova/disk.vmdk', + }, + expected => { + file => 'import.ova', + ext => 'ova', + 'disk-path' => 'import.ova', + path => 'import.ova/disk.vmdk', + content => 'disk.vmdk', + 'content-file' => 'disk.vmdk', + 'content-ext' => 'vmdk', + vtype => 'import', + volname => 'import/import.ova/disk.vmdk', + }, + }, + { + description => "Whitespace in volume name + OVA content (import)", + args => { + volname => 'import/Some Import.ova/disk.qcow2', + }, + expected => { + file => 'Some Import.ova', + ext => 'ova', + 'disk-path' => 'Some Import.ova', + path => 'Some Import.ova/disk.qcow2', + content => 'disk.qcow2', + 'content-file' => 'disk.qcow2', + 'content-ext' => 'qcow2', + vtype => 'import', + volname => 'import/Some Import.ova/disk.qcow2', + }, + }, +]; + +my sub run_special_import_volname_parsing_tests : prototype() () { + for my $case ($cases_import_special_file_path->@*) { + subtest $case->{description} => sub () { + my ($path, $vtype) = $case->{args}->@{qw(path vtype)}; + + my $got_volname_parts = parse_path_as_volname_parts($path, $vtype); + my $got_volname = parse_path_as_volname($path, $vtype); + + if (defined($case->{expected})) { + eq_or_diff( + $got_volname_parts, + $case->{expected}, + 'parse_path_as_volname_parts() returns expected hashref', + { context => 50000 }, + ); + + is( + $got_volname, + $case->{expected}->{volname}, + 'parse_path_as_volname() returns expected volname', + ); + } else { + is($got_volname_parts, undef, 'parse_path_as_volname_parts() returns undef'); + is($got_volname, undef, 'parse_path_as_volname() returns undef'); + } + }; + } + + for my $case ($cases_import_special_volname->@*) { + subtest $case->{description} => sub () { + my ($volname) = $case->{args}->@{qw(volname)}; + + my $got_volname = parse_volname_as_parts($volname); + + if (defined($case->{expected})) { + eq_or_diff( + $got_volname, + $case->{expected}, + 'parse_volname_as_parts() returns expected hashref', + { context => 50000 }, + ); + } else { + is($got_volname, undef, 'parse_volname_as_parts() returns undef'); + } + }; + } + + return; +} + my $DEFAULT_STOREID = 'local'; my $DEFAULT_STORAGE_PATH = File::Temp->newdir(); my $DEFAULT_SCFG = { @@ -546,6 +805,26 @@ my $volid_tests = [ $volname_tests->@* ], }, + { + description => + "volid parsers build on volname parsers' behaviors - 'import' vtype file paths (1)", + storedid => $DEFAULT_STOREID, + scfg => $DEFAULT_SCFG, + cases => [ + map { format_volname_case_to_volid_case($DEFAULT_STOREID, $DEFAULT_SCFG, $_) } + $cases_import_special_file_path->@* + ], + }, + { + description => + "volid parsers build on volname parsers' behaviors - 'import' vtype file paths (2)", + storedid => $ALT_STOREID, + scfg => $ALT_SCFG, + cases => [ + map { format_volname_case_to_volid_case($DEFAULT_STOREID, $DEFAULT_SCFG, $_) } + $cases_import_special_file_path->@* + ], + }, ]; my sub run_volid_parsing_tests : prototype($) ($volid_tests) { @@ -603,6 +882,8 @@ my sub main() { unified_diff(); run_volname_parsing_tests($volname_tests); + run_special_import_volname_parsing_tests(); + run_volid_parsing_tests($volid_tests); done_testing(); diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm index 39e5cb0d..a3cb1343 100644 --- a/src/PVE/Storage/Plugin.pm +++ b/src/PVE/Storage/Plugin.pm @@ -839,20 +839,16 @@ sub parse_volname { if ($vtype eq 'snippets') { return ($vtype, $volume_path, undef, undef, undef, undef, 'raw'); } - } - if ($volname =~ - m!^import/(${PVE::Storage::SAFE_CHAR_WITH_WHITESPACE_CLASS_RE}+\.ova\/${PVE::Storage::OVA_CONTENT_RE_1})$! - ) { - my $packed_image = $1; - my $format = $2; - return ('import', $packed_image, undef, undef, undef, undef, "ova+$format"); - } + if ($vtype eq 'import') { + my $format = $parts->{ext}; - if ($volname =~ - m!^import/(${PVE::Storage::SAFE_CHAR_WITH_WHITESPACE_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$! - ) { - return ('import', $1, undef, undef, undef, undef, $2); + if (defined(my $content_ext = $parts->{'content-ext'})) { + $format .= "+$content_ext"; + } + + return ($vtype, $volume_path, undef, undef, undef, undef, $format); + } } die "unable to parse directory volume name '$volname'\n"; diff --git a/src/test/guest_import_test.pl b/src/test/guest_import_test.pl index 04eec24d..234013e3 100755 --- a/src/test/guest_import_test.pl +++ b/src/test/guest_import_test.pl @@ -357,7 +357,7 @@ my $tests = [ { volname => 'import/some-import.ova/disk.txt', vmid => 1337, - 'expect-fail' => qr/unable to parse directory volume name/, + 'expect-fail' => qr/invalid volname/, }, ], }, @@ -532,7 +532,7 @@ my $tests = [ { volname => 'import/some-🐧-import.ova/disk.qcow2', vmid => 1337, - 'expect-fail' => qr/unable to parse directory volume name/, + 'expect-fail' => qr/invalid volname/, }, ], }, @@ -619,7 +619,7 @@ my $tests = [ { volname => 'import/some-import.ova', vmid => 1337, - 'expect-fail' => qr/only files from 'ova' format can be extracted/, + 'expect-fail' => qr/cannot determine format of/, }, ], }, -- 2.47.3