From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 299CC1FF135 for ; Thu, 02 Jul 2026 16:56:08 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0B6732144B; Thu, 02 Jul 2026 16:56:07 +0200 (CEST) Date: Thu, 2 Jul 2026 16:55:29 +0200 From: Wolfgang Bumiller To: "Max R. Carrara" Subject: Re: [PATCH pve-storage v1 48/54] fix #2884: implement nested subdir scanning and support 'iso' vtype Message-ID: References: <20260422111322.257380-1-m.carrara@proxmox.com> <20260422111322.257380-49-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260422111322.257380-49-m.carrara@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783004123431 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.000 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: SEK7BHY2H4523BBTUXEWJBZ2PQZ65GIU X-Message-ID-Hash: SEK7BHY2H4523BBTUXEWJBZ2PQZ65GIU X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > Signed-off-by: Max R. Carrara > --- > 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! > (? > + (? $RE_DIRECTORY_COMPONENTS )? > (? [^/]+ \. (? (?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 = > 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 > > > > > --