[PATCH pve-storage v1 28/54] tree-wide: replace remaining usages of regexes for 'import' vtype

["Max R. Carrara" <m.carrara@proxmox.com>]

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 <m.carrara@proxmox.com>
---
 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!
+    (?<content_file>
+        ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+ \. (?<content_ext> $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!
+    (?<file>
+        # NOTE: Unlike in RE_IMPORT_FILE_PATH, we allow whitespace here
+        ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+
+        \.
+        (?<ext> ova)
+    ) / (?<content> $RE_OVA_CONTENT)
+!xn;
+
+my $RE_IMPORT_VOLNAME_REGULAR_FILE = qr!
+    (?<file>
+        # NOTE: Unlike in RE_IMPORT_FILE_PATH, we allow whitespace here
+        ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+
+        \.
+        (?<ext> $RE_IMPORT_EXTENSIONS)
+    )
+!xn;
+
+my $RE_IMPORT_VOLNAME = qr!
+    (?<path>
+        # 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