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

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

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 <m.carrara@proxmox.com>
---
 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!
+    (?<path>
+        (?<file> ($RE_SAFE_CHAR_CLASS)+ \. (?<ext> $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