Re: [PATCH pve-storage v1 48/54] fix #2884: implement nested subdir scanning and support 'iso' vtype

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

On Wed, Apr 22, 2026 at 01:13:14PM +0200, Max R. Carrara wrote:
> Introduce a new 'max-scan-depth' property for directory-based
> storages. This property makes it so that files for specific volume
> types may also be put into (nested) subdirectories inside the volume
> type's directory.
> 
> The maximum allowed depth is between 0 and 50, with 0 being the
> default, which corresponds to the current behavior.
> 
> Support nested subdirectories for ISOs (the 'iso' vtype) only for now.
> 
> Achieve all of this by calling the `get_subdir_files()` helper in
> `PVE::Storage::Plugin` recursively. Note that the default recursion
> limit in Perl appears to be exactly 100 [0], way above the maximum
> value of 50 for the 'max-scan-depth' property. Should we need deeper
> nesting, we can always make the subroutine iterative instead of
> recursive later.
> 
> Add additional test cases wherever applicable to account for nested
> subdirectories, including cases that check whether the limit set by
> the property is upheld, and also cases that check for the existence of
> parent directory references ('..') in volume names.
> 
> [0]: https://perldoc.perl.org/perl5101delta#Deep-recursion-on-subroutine-%22%25s%22
> 
> Originally-by: Noel Ullreich <n.ullreich@proxmox.com>
> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/Storage/BTRFSPlugin.pm              |   1 +
>  src/PVE/Storage/CephFSPlugin.pm             |   1 +
>  src/PVE/Storage/Common/Parse.pm             |  14 +++
>  src/PVE/Storage/Common/test/parser_tests.pl |  50 ++++++++
>  src/PVE/Storage/DirPlugin.pm                |   1 +
>  src/PVE/Storage/Plugin.pm                   |  41 ++++++-
>  src/test/filesystem_path_test.pm            |   8 ++
>  src/test/list_volumes_test.pm               | 127 ++++++++++++++++++++
>  src/test/parse_volname_test.pm              |  56 +++++++++
>  src/test/path_to_volume_id_test.pm          |   7 ++
>  10 files changed, 302 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index 67b442f6..0a29fd31 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -68,6 +68,7 @@ sub properties {
>  sub options {
>      return {
>          path => { fixed => 1 },
> +        'max-scan-depth' => { optional => 1 },
>          nodes => { optional => 1 },
>          shared => { optional => 1 },
>          disable => { optional => 1 },
> diff --git a/src/PVE/Storage/CephFSPlugin.pm b/src/PVE/Storage/CephFSPlugin.pm
> index fbc97113..b31f53d9 100644
> --- a/src/PVE/Storage/CephFSPlugin.pm
> +++ b/src/PVE/Storage/CephFSPlugin.pm
> @@ -140,6 +140,7 @@ sub options {
>      return {
>          path => { fixed => 1 },
>          'content-dirs' => { optional => 1 },
> +        'max-scan-depth' => { optional => 1 },
>          monhost => { optional => 1 },
>          nodes => { optional => 1 },
>          subdir => { optional => 1 },
> diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm
> index 9fb45b0c..f98b702d 100644
> --- a/src/PVE/Storage/Common/Parse.pm
> +++ b/src/PVE/Storage/Common/Parse.pm
> @@ -57,6 +57,10 @@ my $RE_SAFE_CHAR_CLASS = qr/[a-zA-Z0-9\-\.\+\=\_]/;
> 
>  my $RE_SAFE_CHAR_WITH_WHITESPACE_CLASS = qr/[ a-zA-Z0-9\-\.\+\=\_]/;
> 
> +my $RE_DIRECTORY_COMPONENTS = qr!
> +    ( ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+ / )*?  # NOTE: non-greedy matching

↑ Greedy should work fine (and is usually faster).
This regex always ends in a slash and all uses of this regex are
immediately followed by a `[^/]` (not-a-slash), so the boundary should
always be the same.

> +!xn;
> +
>  my $RE_PARENT_DIR = quotemeta('..');
>  my $RE_CONTAINS_PARENT_DIR = qr!
>      ( ^$RE_PARENT_DIR/ )  #  ../ --> Beginning of path
> @@ -73,6 +77,7 @@ my $RE_VMID = qr![1-9][0-9]{2,8}!;
> 
>  my $RE_ISO_FILE_PATH = qr!
>      (?<path>
> +        (?<dir> $RE_DIRECTORY_COMPONENTS )?
>          (?<file> [^/]+ \. (?<ext> (?i: iso|img) ) )
>      )
>  !xn;
> @@ -206,6 +211,15 @@ my sub format_named_groups(%groups) {
> 
>      my @disk_path_components = ();
> 
> +    if (defined($result->{dir})) {
> +        if ($result->{dir} eq '') {
> +            delete $result->{dir};
> +        } else {
> +            $result->{dir} = strip_trailing_path_separators($result->{dir});
> +            push(@disk_path_components, $result->{dir});
> +        }
> +    }
> +
>      if (defined($result->{file})) {
>          $result->{file} = strip_leading_path_separators($result->{file});
>          push(@disk_path_components, $result->{file});
> diff --git a/src/PVE/Storage/Common/test/parser_tests.pl b/src/PVE/Storage/Common/test/parser_tests.pl
> index b6e42307..ad122cd2 100755
> --- a/src/PVE/Storage/Common/test/parser_tests.pl
> +++ b/src/PVE/Storage/Common/test/parser_tests.pl
> @@ -73,6 +73,32 @@ my $volname_cases_iso_valid = [
>              volname => 'iso/Fedora.Img',
>          },
>      },
> +
> +    # subdirectories
> +    {
> +        path => 'subdir/custom-debian.iso',
> +        expected => {
> +            file => 'custom-debian.iso',
> +            ext => 'iso',
> +            dir => 'subdir',
> +            'disk-path' => 'subdir/custom-debian.iso',
> +            path => 'subdir/custom-debian.iso',
> +            vtype => 'iso',
> +            volname => 'iso/subdir/custom-debian.iso',
> +        },
> +    },
> +    {
> +        path => 'deeply/nested/dir/hannah-montana-linux.IMG',
> +        expected => {
> +            file => 'hannah-montana-linux.IMG',
> +            ext => 'IMG',
> +            dir => 'deeply/nested/dir',
> +            'disk-path' => 'deeply/nested/dir/hannah-montana-linux.IMG',
> +            path => 'deeply/nested/dir/hannah-montana-linux.IMG',
> +            vtype => 'iso',
> +            volname => 'iso/deeply/nested/dir/hannah-montana-linux.IMG',
> +        },
> +    },
>  ];
> 
>  my $volname_cases_iso_invalid = [
> @@ -84,6 +110,30 @@ my $volname_cases_iso_invalid = [
>          },
>          expected => undef,
>      },
> +    {
> +        description => "Parent dir reference in path (beginning) (iso)",
> +        args => {
> +            path => '../custom-debian.iso',
> +            vtype => 'iso',
> +        },
> +        expected => undef,
> +    },
> +    {
> +        description => "Parent dir reference in path (middle) (iso)",
> +        args => {
> +            path => 'subdir/../custom-debian.iso',
> +            vtype => 'iso',
> +        },
> +        expected => undef,
> +    },
> +    {
> +        description => "Parent dir reference in path (end) (iso)",
> +        args => {
> +            path => 'subdir/custom-debian.iso/..',
> +            vtype => 'iso',
> +        },
> +        expected => undef,
> +    },
>  ];
> 
>  my $volname_cases_vztmpl_valid = [
> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
> index 80c4a031..58b942e7 100644
> --- a/src/PVE/Storage/DirPlugin.pm
> +++ b/src/PVE/Storage/DirPlugin.pm
> @@ -81,6 +81,7 @@ sub options {
>      return {
>          path => { fixed => 1 },
>          'content-dirs' => { optional => 1 },
> +        'max-scan-depth' => { optional => 1 },
>          nodes => { optional => 1 },
>          shared => { optional => 1 },
>          disable => { optional => 1 },
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 101e0b6d..0fbcbd4b 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -233,6 +233,15 @@ my $defaultData = {
>              format => "pve-dir-override-list",
>              optional => 1,
>          },
> +        'max-scan-depth' => {
> +            description => "Maximum depth of subdirectories to traverse when searching for"
> +                . " ISOs in directories.",
> +            type => 'integer',
> +            default => 0,
> +            minimum => 0,
> +            maximum => 50,
> +            optional => 1,
> +        },
>          options => {
>              description => "NFS/CIFS mount options (see 'man nfs' or 'man mount.cifs')",
>              type => 'string',
> @@ -1669,10 +1678,16 @@ sub list_images {
> 
>  # $vtype = <iso|vztmpl|backup|snippets|import>
>  my sub get_subdir_files {
> -    my ($storeid, $scfg, $vtype, $vmid) = @_;
> +    use feature 'current_sub'; # Needed for the __SUB__ token further below
> +
> +    my ($storeid, $scfg, $vtype, $vmid, $remaining_depth, $current_path) = @_;
> 
>      my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype);
> 
> +    if (!defined($current_path)) {
> +        $current_path = $vtype_subdir;
> +    }
> +
>      my $res = [];
> 
>      my $get_subdir_file_info = sub {
> @@ -1752,10 +1767,24 @@ my sub get_subdir_files {
>          return;
>      };
> 
> -    for my $path (<$vtype_subdir/*>) {
> +    for my $path (<$current_path/*>) {
>          my $st = File::stat::stat($path);
> 
> -        next if (!$st || S_ISDIR($st->mode));
> +        next if !$st;
> +
> +        if (S_ISDIR($st->mode)) {
> +            if (defined($remaining_depth) && $remaining_depth > 0) {
> +                # Note: Have to call the subroutine via __SUB__->(...) because
> +                # lexical subroutines otherwise cannot reference themselves

↑ You can drop this comment.

> +                my $inner_res = __SUB__->(
> +                    $storeid, $scfg, $vtype, $vmid, $remaining_depth - 1, $path,
> +                );
> +
> +                push($res->@*, $inner_res->@*);
> +            }
> +
> +            next;
> +        }
> 
>          if (defined(my $info = $get_subdir_file_info->($path, $st))) {
>              $info->{size} = $st->size;
> @@ -1773,6 +1802,10 @@ my sub get_subdir_files {
>  sub list_volumes {
>      my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> 
> +    my $depth = $scfg->{'max-scan-depth'} // 0;
> +    $depth = 0 if $depth < 0;
> +    $depth = 50 if $depth > 50;
> +
>      my $res = [];
>      my $vmlist = PVE::Cluster::get_vmlist();
> 
> @@ -1786,7 +1819,7 @@ sub list_volumes {
>          return if !$scfg->{path};
> 
>          if ($type eq 'iso' && !defined($vmid)) {
> -            return get_subdir_files($storeid, $scfg, 'iso', undef);
> +            return get_subdir_files($storeid, $scfg, 'iso', undef, $depth);
>          }
> 
>          if ($type eq 'vztmpl' && !defined($vmid)) {
> diff --git a/src/test/filesystem_path_test.pm b/src/test/filesystem_path_test.pm
> index 5a715ce9..26a74a0d 100644
> --- a/src/test/filesystem_path_test.pm
> +++ b/src/test/filesystem_path_test.pm
> @@ -48,6 +48,14 @@ my $tests = [
>              "$DEFAULT_STORAGE_DIR/template/iso/my-awesome-proxmox.iso", undef, 'iso',
>          ],
>      },
> +    {
> +        volname => 'iso/foo/bar/baz/my-awesome-proxmox.iso',
> +        snapname => undef,
> +        expected => [
> +            "$DEFAULT_STORAGE_DIR/template/iso/foo/bar/baz/my-awesome-proxmox.iso", undef,
> +            'iso',
> +        ],
> +    },
>      {
>          volname => "backup/vzdump-qemu-1234-2020_03_30-21_12_40.vma",
>          snapname => undef,
> diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm
> index ce35c782..0daaba94 100644
> --- a/src/test/list_volumes_test.pm
> +++ b/src/test/list_volumes_test.pm
> @@ -1296,6 +1296,133 @@ my $test_param_list = [
>              },
>          ],
>      },
> +    {
> +        description => "VMID: none, no nested subdirectories when using defaults",
> +        storeid => $DEFAULT_STOREID,
> +        scfg => $DEFAULT_SCFG,
> +        vmid => undef,
> +        vtypes => ['iso', 'vztmpl', 'snippets', 'import'],
> +        cases => [
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/some-installer.iso",
> +                expected => {
> +                    content => 'iso',
> +                    ctime => $DEFAULT_CTIME,
> +                    format => 'iso',
> +                    size => $DEFAULT_SIZE,
> +                    volid => 'local:iso/some-installer.iso',
> +                },
> +            },
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/1/some-installer.iso",
> +                expected => undef,
> +            },
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/3/4/5/some-installer.iso",
> +                expected => undef,
> +            },
> +        ],
> +    },
> +    {
> +        description => "VMID: none, nested subdirectories allowed, max-scan-depth = 1",
> +        storeid => $DEFAULT_STOREID,
> +        scfg => {
> +            type => 'dir',
> +            path => $DEFAULT_STORAGE_PATH,
> +            shared => 0,
> +            'max-scan-depth' => 1,
> +            content => {
> +                iso => 1,
> +                vztmpl => 1,
> +                snippets => 1,
> +                import => 1,
> +            },
> +        },
> +        vmid => undef,
> +        vtypes => ['iso', 'vztmpl', 'snippets', 'import'],
> +        cases => [
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/some-installer.iso",
> +                expected => {
> +                    content => 'iso',
> +                    ctime => $DEFAULT_CTIME,
> +                    format => 'iso',
> +                    size => $DEFAULT_SIZE,
> +                    volid => 'local:iso/some-installer.iso',
> +                },
> +            },
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/1/some-installer.iso",
> +                expected => {
> +                    content => 'iso',
> +                    ctime => $DEFAULT_CTIME,
> +                    format => 'iso',
> +                    size => $DEFAULT_SIZE,
> +                    volid => 'local:iso/1/some-installer.iso',
> +                },
> +            },
> +            {
> +                # Exceeds max-scan-depth
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/some-installer.iso",
> +                expected => undef,
> +            },
> +        ],
> +    },
> +    {
> +        description => "VMID: none, nested subdirectories allowed, max-scan-depth = 5",
> +        storeid => $DEFAULT_STOREID,
> +        scfg => {
> +            type => 'dir',
> +            path => $DEFAULT_STORAGE_PATH,
> +            shared => 0,
> +            'max-scan-depth' => 5,
> +            content => {
> +                iso => 1,
> +                vztmpl => 1,
> +                snippets => 1,
> +                import => 1,
> +            },
> +        },
> +        vmid => undef,
> +        vtypes => ['iso', 'vztmpl', 'snippets', 'import'],
> +        cases => [
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/some-installer.iso",
> +                expected => {
> +                    content => 'iso',
> +                    ctime => $DEFAULT_CTIME,
> +                    format => 'iso',
> +                    size => $DEFAULT_SIZE,
> +                    volid => 'local:iso/some-installer.iso',
> +                },
> +            },
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/1/some-installer.iso",
> +                expected => {
> +                    content => 'iso',
> +                    ctime => $DEFAULT_CTIME,
> +                    format => 'iso',
> +                    size => $DEFAULT_SIZE,
> +                    volid => 'local:iso/1/some-installer.iso',
> +                },
> +            },
> +            {
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/3/4/5/some-installer.iso",
> +                expected => {
> +                    content => 'iso',
> +                    ctime => $DEFAULT_CTIME,
> +                    format => 'iso',
> +                    size => $DEFAULT_SIZE,
> +                    volid => 'local:iso/1/2/3/4/5/some-installer.iso',
> +                },
> +            },
> +            {
> +                # Exceeds max-scan-depth
> +                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/3/4/5/6/some-installer.iso",
> +                expected => undef,
> +            },
> +        ],
> +    },
>  ];
> 
>  # Additional test cases that cannot be constructed within the list above
> diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm
> index 1dcec9d6..24d1ed0e 100644
> --- a/src/test/parse_volname_test.pm
> +++ b/src/test/parse_volname_test.pm
> @@ -167,10 +167,66 @@ my $tests = [
>                  volname => "iso/$file_name",
>                  expected => ['iso', "$file_name", undef, undef, undef, undef, 'raw'],
>              },
> +            {
> +                description => "ISO image, $suffix, subdirectory",
> +                volname => "iso/foo/$file_name",
> +                expected => [
> +                    'iso', "foo/$file_name", undef, undef, undef, undef, 'raw',
> +                ],
> +            },
> +            {
> +                description => "ISO image, $suffix, nested subdirectories",
> +                volname => "iso/foo/bar/baz/$file_name",
> +                expected => [
> +                    'iso', "foo/bar/baz/$file_name", undef, undef, undef, undef, 'raw',
> +                ],
> +            },
> +            {
> +                description => "ISO image, $suffix, subdirectory with same name as file",
> +                volname => "iso/$file_name/$file_name",
> +                expected => [
> +                    'iso', "$file_name/$file_name", undef, undef, undef, undef, 'raw',
> +                ],
> +            },
>          );
> 
>          push($tests->@*, @extra_tests);
>      }
> +
> +    # Failed tests
> +    {
> +        my $file_name = "$prefix.iso";
> +
> +        my @extra_failed_tests = (
> +            {
> +                description =>
> +                    "ISO image, iso, parent directory reference before volume type prefix",
> +                volname => "../iso/$file_name",
> +                expected => "unable to parse directory volume name '../iso/$file_name'\n",
> +            },
> +            {
> +                description =>
> +                    "ISO image, iso, parent directory reference at beginning of volume path",
> +                volname => "iso/../$file_name",
> +                expected => "unable to parse directory volume name 'iso/../$file_name'\n",
> +            },
> +            {
> +                description =>
> +                    "ISO image, iso, parent directory reference at end of volume path",
> +                volname => "iso/$file_name/..",
> +                expected => "unable to parse directory volume name 'iso/$file_name/..'\n",
> +            },
> +            {
> +                description =>
> +                    "ISO image, iso, parent directory reference between dir components of volume path",
> +                volname => "iso/foo/../bar/$file_name",
> +                expected =>
> +                    "unable to parse directory volume name 'iso/foo/../bar/$file_name'\n",
> +            },
> +        );
> +
> +        push($tests->@*, @extra_failed_tests);
> +    }
>  }
> 
>  # Test cases for container templates
> diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm
> index 6c7d0d67..bc87d289 100644
> --- a/src/test/path_to_volume_id_test.pm
> +++ b/src/test/path_to_volume_id_test.pm
> @@ -123,6 +123,13 @@ my $tests = [
>              'iso', 'local:iso/yet-again-a-installation-disk.iso',
>          ],
>      },
> +    {
> +        description => 'ISO file, nested subdirectories',
> +        file => "$DEFAULT_STORAGE_DIR/template/iso/foo/bar/hannah-montana-linux-installer.iso",
> +        expected => [
> +            'iso', 'local:iso/foo/bar/hannah-montana-linux-installer.iso',
> +        ],
> +    },
>      {
>          description => 'CT template, tar.gz',
>          file => "$DEFAULT_STORAGE_DIR/template/cache/debian-10.0-standard_10.0-1_amd64.tar.gz",
> --
> 2.47.3
> 
> 
> 
> 
> 

--