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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Apr 22, 2026 at 01:12:53PM +0200, Max R. Carrara wrote:
> 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\-\.\+\=\_]/;

Btw. why are we copying this here, rather than using the one from
`PVE::Storage`?
Do we intend to relax this here? (Because it's IMO pretty clear that
relaxing the exported one in `PVE::Storage` is generally not an option.)

> +
>  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
> 
> 
> 
> 
> 

--