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

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

On Tue Jun 30, 2026 at 4:28 PM CEST, Wolfgang Bumiller wrote:
> 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 intention here was to already do some preparation to also get rid of
the regex in PVE::Storage and have plugin authors instead *only* use
shared parsing functions -- but on second thought, duplicating the regex
here isn't really ideal at all.

Relaxing it was never the intention.

Anyhow, I'll rework this for v2. Thanks for spotting this!

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