Re: [PATCH pve-storage v1 16/54] storage: clean up code that was moved into helper in path_to_volume_id

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

On Wed, Apr 22, 2026 at 01:12:42PM +0200, Max R. Carrara wrote:
> In order to make the parsing logic in `path_to_volume_id()` both
> simpler and more manageable for future changes, do the following:
> 
> 1. Iterate over a list of vtypes that store their contents in
>    subdirectories and fetch said subdirectories dynamically instead of
>    hardcoding them.
> 
> 2. Explicitly exclude the `rootdir` vtype in this list of vtypes, as
>    its subdirectory is currently not used.
> 
> 3. Strip the vtype subdirectory from the beginning of the path in
>    order to make matching on the file name easier. Return early as a
>    safeguard if this fails.
> 
> 4. Adapt the original regexes in the parsing helper to not include the
>    vtype subdir anymore, since this is now being stripped.
> 
> 5. Instead of returning a 2-element array, return `undef` if the path
>    fails to parse, and the volume ID (volid) alone if parsing
>    succeeds. Since the caller is now passing along the vtype, it does
>    not need to be returned anymore.
> 
> Note that the parsing logic of the `parse_volid_from_file_path()`
> helper inside `path_to_volume_id()` now is very similar to the parsing
> logic of the `get_subdir_file_info()` closure in the
> `get_subdir_files()` helper inside `Plugin.pm`. This opens up further
> simplification / code de-duplication in the future, so that our
> parsing logic can eventually be kept in one place only.
> 
> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/Storage.pm | 73 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 50 insertions(+), 23 deletions(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 2886ab9e..1b0569dd 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -23,6 +23,7 @@ use PVE::INotify;
>  use PVE::RPCEnvironment;
>  use PVE::SSHInfo;
>  use PVE::Storage::Common qw(
> +    plugin_get_default_vtype_subdirs
>      plugin_get_vtype_subdir
>  );
>  use PVE::RESTEnvironment qw(log_warn);
> @@ -712,17 +713,31 @@ sub path_to_volume_id {
>      # for example when nfs storage is not mounted
>      $path = abs_path($path) || $path;
> 
> +    my $get_vtypes_to_check = sub {
> +        my $default_vtype_subdirs = plugin_get_default_vtype_subdirs();
> +
> +        # TODO: vtype split: handle directory for 'rootdir' type.
> +        # CTs currently use the same dir as VMs --> the dir for the 'images' type.
> +        # Directory is therefore unused right now, so exclude it.
> +        delete $default_vtype_subdirs->{rootdir};
> +
> +        return [sort keys $default_vtype_subdirs->%*];
> +    };
> +
>      my $parse_volid_from_file_path = sub {
> -        my ($plugin, $sid, $scfg) = @_;
> +        my ($plugin, $sid, $scfg, $vtype) = @_;
> 
> -        my $imagedir = plugin_get_vtype_subdir($scfg, 'images');
> -        my $isodir = plugin_get_vtype_subdir($scfg, 'iso');
> -        my $tmpldir = plugin_get_vtype_subdir($scfg, 'vztmpl');
> -        my $backupdir = plugin_get_vtype_subdir($scfg, 'backup');
> -        my $snippetsdir = plugin_get_vtype_subdir($scfg, 'snippets');
> -        my $importdir = plugin_get_vtype_subdir($scfg, 'import');
> +        my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype);
> 
> -        if ($path =~ m!^\Q$imagedir\E/(\d+)/([^/\s]+)$!) {
> +        # 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!!) {

Same as in 9/54 - lookahead for a slash may be useful.

> +            return;
> +        }
> +
> +        if ($vtype eq 'images') {
> +            return if $filename !~ m!/(\d+)/([^/\s]+)$!;
>              my $vmid = $1;
>              my $name = $2;
> 
> @@ -731,48 +746,60 @@ sub path_to_volume_id {
>                  my ($storeid, $volname) = parse_volume_id($info->{volid});
>                  my $volpath = $plugin->path($scfg, $volname, $storeid);
>                  if ($volpath eq $path) {
> -                    return ('images', $info->{volid});
> +                    return $info->{volid};
>                  }
>              }
> 
> -            return ('');
> +            return;
>          }
> 
> -        if ($path =~ m!^\Q$isodir\E/([^/]+$ISO_EXT_RE_0)$!) {
> +        if ($vtype eq 'iso') {
> +            return if $filename !~ m!/([^/]+$ISO_EXT_RE_0)$!;
>              my $name = $1;
> -            return ('iso', "$sid:iso/$name");
> +            return "$sid:iso/$name";
>          }
> 
> -        if ($path =~ m!^\Q$tmpldir\E/([^/]+$VZTMPL_EXT_RE_1)$!) {
> +        if ($vtype eq 'vztmpl') {
> +            return if $filename !~ m!/([^/]+$VZTMPL_EXT_RE_1)$!;
>              my $name = $1;
> -            return ('vztmpl', "$sid:vztmpl/$name");
> +            return "$sid:vztmpl/$name";
>          }
> 
> -        if ($path =~ m!^\Q$backupdir\E/([^/]+$BACKUP_EXT_RE_2)$!) {
> +        if ($vtype eq 'backup') {
> +            return if $filename !~ m!/([^/]+$BACKUP_EXT_RE_2)$!;
>              my $name = $1;
> -            return ('backup', "$sid:backup/$name");
> +            return "$sid:backup/$name";
>          }
> 
> -        if ($path =~ m!^\Q$snippetsdir\E/([^/]+)$!) {
> +        if ($vtype eq 'snippets') {
> +            return if $filename !~ m!/([^/]+)$!;
>              my $name = $1;
> -            return ('snippets', "$sid:snippets/$name");
> +            return "$sid:snippets/$name";
>          }
> 
> -        if ($path =~ m!^\Q$importdir\E/(${SAFE_CHAR_CLASS_RE}+${IMPORT_EXT_RE_1})$!) {
> +        if ($vtype eq 'import') {
> +            return if $filename !~ m!/(${SAFE_CHAR_CLASS_RE}+${IMPORT_EXT_RE_1})$!;
>              my $name = $1;
> -            return ('import', "$sid:import/$name");
> +            return "$sid:import/$name";
>          }
> 
> -        return ('');
> +        return;
>      };
> 
> +    my $vtypes_to_check = $get_vtypes_to_check->();
> +
>      for my $sid (keys $ids->%*) {
>          my $scfg = $ids->{$sid};
>          next if !$scfg->{path};
>          my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> 
> -        my ($vtype, $volid) = $parse_volid_from_file_path->($plugin, $sid, $scfg);
> -        return ($vtype, $volid) if $vtype;
> +        for my $vtype ($vtypes_to_check->@*) {
> +            my $volid = $parse_volid_from_file_path->($plugin, $sid, $scfg, $vtype);
> +
> +            if (defined($volid)) {
> +                return ($vtype, $volid);
> +            }
> +        }
>      }
> 
>      # can't map path to volume id
> --
> 2.47.3
> 
> 
> 
> 
> 

--