Re: [PATCH pve-storage v1 09/54] plugin: adapt get_subdir_files helper of list_volumes API method

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

Mostly fine.

On Wed, Apr 22, 2026 at 01:12:35PM +0200, Max R. Carrara wrote:
> Pull the body of the loop inside `get_subdir_files()` out into a
> closure and break up the if-elsif chain into more readable if-clauses.
> 
> Strip the vtype subdirectory from the beginning of the file paths
> being worked on inside that closure. This does not have an effect on
> how any of the subsequent regexes match on the path / file name,
> because they match at the end of a given path.
> 
> Additionally, take take the storage config hash `$scfg` instead of the
> volume type subdir `$path` as parameter. Passing `$scfg` along
> makes it possible to obtain the subdir for the given `$vtype` inside
> the helper directly instead of passing both the vtype and its
> associated subdirectory along.
> 
> All of these things make future changes easier to manage.
> 
> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/Storage/Plugin.pm | 95 ++++++++++++++++++++++++++-------------
>  1 file changed, 64 insertions(+), 31 deletions(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 2d76f452..c4eb6f18 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1673,37 +1673,54 @@ sub list_images {
> 
>  # $vtype = <iso|vztmpl|backup|snippets|import>
>  my sub get_subdir_files {
> -    my ($storeid, $path, $vtype, $vmid) = @_;
> +    my ($storeid, $scfg, $vtype, $vmid) = @_;
> +
> +    my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype);
> 
>      my $res = [];
> 
> -    for my $filename (<$path/*>) {
> -        my $st = File::stat::stat($filename);
> +    my $get_subdir_file_info = sub {
> +        my ($path, $st) = @_;
> 
> -        next if (!$st || S_ISDIR($st->mode));
> -
> -        my $info;
> +        # Strip the vtype subdir from beginning of the current path
> +        # so that we don't have to take it into account when parsing the file name
> +        my $filename = $path;
> +        if ($filename !~ s!^\Q$vtype_subdir\E!!) {

It might be good to also look-ahead for a slash here. That is, add a
`(?=/)` after the `\E`. (Alternatively just do `\E/!/!` (to re-insert the
slash after the removal))

This won't change the result, but if custom subdirs match by prefix we
won't do unnecessary work. (eg. if we have `foo` for one vtype and
`foobar` for the other, `foo` will match the `foobar/...` paths)

(I know this later gets changed by the `Parse` module, but something
similar probably applies there.)

> +            return;
> +        }
> 
>          if ($vtype eq 'iso') {
> -            next if $filename !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
> +            return if $filename !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
> 
> -            $info = { volid => "$storeid:iso/$1", format => 'iso' };
> +            return {
> +                volid => "$storeid:iso/$1",
> +                format => 'iso',
> +            };
> +        }
> 
> -        } elsif ($vtype eq 'vztmpl') {
> -            next if $filename !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
> +        if ($vtype eq 'vztmpl') {
> +            return if $filename !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
> 
> -            $info = { volid => "$storeid:vztmpl/$1", format => $2 eq 'tar' ? $2 : "t$2" };
> +            return {
> +                volid => "$storeid:vztmpl/$1",
> +                format => $2 eq 'tar' ? $2 : "t$2",
> +            };
> +        }
> 
> -        } elsif ($vtype eq 'backup') {
> -            next if $filename !~ m!/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$!;
> -            my $original = $filename;
> +        if ($vtype eq 'backup') {
> +            return if $filename !~ m!/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$!;
> +
> +            my $original = $path;
>              my $format = $2;
>              $filename = $1;
> 
>              # only match for VMID now, to avoid false positives (VMID in parent directory name)
> -            next if defined($vmid) && $filename !~ m/\S+-$vmid-\S+/;
> +            return if defined($vmid) && $filename !~ m/\S+-$vmid-\S+/;
> 
> -            $info = { volid => "$storeid:backup/$filename", format => $format };
> +            my $info = {
> +                volid => "$storeid:backup/$filename",
> +                format => $format,
> +            };
> 
>              my $archive_info = eval { PVE::Storage::archive_info($filename) } // {};
> 
> @@ -1722,24 +1739,42 @@ my sub get_subdir_files {
>              }
> 
>              $info->{protected} = 1 if -e PVE::Storage::protection_file_path($original);
> -        } elsif ($vtype eq 'snippets') {
> 
> -            $info = {
> +            return $info;
> +        }
> +
> +        if ($vtype eq 'snippets') {
> +            return {
>                  volid => "$storeid:snippets/" . basename($filename),
>                  format => 'snippet',
>              };
> -        } elsif ($vtype eq 'import') {
> -            next
> +        }
> +
> +        if ($vtype eq 'import') {
> +            return
>                  if $filename !~
>                  m!/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$!i;
> 
> -            $info = { volid => "$storeid:import/$1", format => "$2" };
> +            return {
> +                volid => "$storeid:import/$1",
> +                format => "$2",
> +            };
>          }
> 
> -        $info->{size} = $st->size;
> -        $info->{ctime} //= $st->ctime;
> +        return;
> +    };
> 
> -        push @$res, $info;
> +    for my $path (<$vtype_subdir/*>) {
> +        my $st = File::stat::stat($path);
> +
> +        next if (!$st || S_ISDIR($st->mode));
> +
> +        if (defined(my $info = $get_subdir_file_info->($path, $st))) {
> +            $info->{size} = $st->size;
> +            $info->{ctime} //= $st->ctime;
> +
> +            push $res->@*, $info;
> +        }
>      }
> 
>      return $res;
> @@ -1758,18 +1793,16 @@ sub list_volumes {
>          if ($type eq 'images' || $type eq 'rootdir') {
>              $data = $class->list_images($storeid, $scfg, $vmid);
>          } elsif ($scfg->{path}) {
> -            my $path = plugin_get_vtype_subdir($scfg, $type);
> -
>              if ($type eq 'iso' && !defined($vmid)) {
> -                $data = get_subdir_files($storeid, $path, 'iso');
> +                $data = get_subdir_files($storeid, $scfg, 'iso', undef);
>              } elsif ($type eq 'vztmpl' && !defined($vmid)) {
> -                $data = get_subdir_files($storeid, $path, 'vztmpl');
> +                $data = get_subdir_files($storeid, $scfg, 'vztmpl', undef);
>              } elsif ($type eq 'backup') {
> -                $data = get_subdir_files($storeid, $path, 'backup', $vmid);
> +                $data = get_subdir_files($storeid, $scfg, 'backup', $vmid);
>              } elsif ($type eq 'snippets') {
> -                $data = get_subdir_files($storeid, $path, 'snippets');
> +                $data = get_subdir_files($storeid, $scfg, 'snippets', undef);
>              } elsif ($type eq 'import') {
> -                $data = get_subdir_files($storeid, $path, 'import');
> +                $data = get_subdir_files($storeid, $scfg, 'import', undef);
>              }
>          }
> 
> --
> 2.47.3
> 
> 
> 
> 
> 

--