Re: [PATCH pve-storage v1 22/54] tree-wide: introduce parsing module and replace usages of ISO_EXT_RE_0

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

On Tue Apr 28, 2026 at 5:20 PM CEST, Wolfgang Bumiller wrote:
> On Wed, Apr 22, 2026 at 01:12:48PM +0200, Max R. Carrara wrote:
> > Introduce the `PVE::Storage::Common::Parse` module with the following
> > subroutines:
> >
> > * `parse_path_as_volname_parts($path, $vtype)`
> >
> >   Parses a given file path into several smaller parts that constitute
> >   a volume name for the given volume type.
> >
> > * `parse_path_as_volname($path, $vtype)`
> >
> >   Parses a given file path directly into a volume name.
> >
> > * `parse_volname_as_parts($volname)`
> >
> >   Parses an existing volume name into its constituent parts.
> >
> > * `parse_path_as_volid_parts($storeid, $scfg, $path, $vtype)`
> >
> >   Parses a given file path into several smaller parts that constitute
> >   a volume ID for the given storage ID, its configuration and volume
> >   type.
> >
> > * `parse_path_as_volid($storeid, $scfg, $path, $vtype)`
> >
> >   Like `parse_path_as_volid_parts()`, but parses the given path
> >   directly into a volume ID.
> >
> > * `parse_volid_as_parts($volid)`
> >
> >   Parses an existing volume ID into its storage ID and volume name
> >   parts.
> >
> > As of this commit, these parsing functions only support the 'iso'
> > volume type, with the exception of `parse_volid_as_parts()`, which
> > does not depend on knowing the volume type.
> >
> > Using the newly introduced parsing helpers, replace all occurrences of
> > the `PVE::Storage::ISO_EXT_RE_0` regex across the repository.
> >
> > Keep the `ISO_EXT_RE_0` regex around for now, since its visibility is
> > public, which also means that it is part of the storage API (whether
> > that was intended or not). On an APIVER + APIAGE bump, this regex
> > should be marked for removal.
> >
> > Additional Notes Regarding the new Parsers
> > ==========================================
> >
> > Support for other volume types will be added individually in future
> > commits.
> >
> > The new *private* regex used for 'iso' vtype parsing is still matching
> > file extensions case-insensitively, but also matches the entire path
> > and file name portions of 'iso' file paths and volume names.
> >
> > These parts are extracted using named regex groups, as that is much
> > easier to handle and keep track of mentally, even with smaller
> > regexes. These named groups are the "constituent parts" that are
> > returned by some of the new parser subroutines.
> >
> > However, one important difference here is that named regex groups do
> > not support dashes `-` in their names, only underscores `_`. To keep
> > things consistent with our style (using dashes instead of underscores
> > in hash keys and the API), these named groups are formatted before
> > being returned—underscores are simply substituted with dashes.
> >
> > Finally, all parser subroutines check whether a parent directory
> > reference (`..` or "double dots") is contained in the passed or
> > extracted file path, and return early if there is. This is an
> > additional safety measure that is intentionally introduced in this
> > commit to guard against any mishaps in the future as the parsers gain
> > more functionality, such as supporting nested directories inside
> > different volume types' subdirectories.
> >
> > Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
> > ---
> >  src/PVE/API2/Storage/Status.pm  |   9 +-
> >  src/PVE/Storage.pm              |   7 +-
> >  src/PVE/Storage/Common.pm       |   2 +
> >  src/PVE/Storage/Common/Makefile |   1 +
> >  src/PVE/Storage/Common/Parse.pm | 330 ++++++++++++++++++++++++++++++++
> >  src/PVE/Storage/Plugin.pm       |  19 +-
> >  6 files changed, 358 insertions(+), 10 deletions(-)
> >  create mode 100644 src/PVE/Storage/Common/Parse.pm
> >
> > diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
> > index 2b0b121a..cf60c148 100644
> > --- a/src/PVE/API2/Storage/Status.pm
> > +++ b/src/PVE/API2/Storage/Status.pm
> > @@ -23,6 +23,9 @@ use PVE::Storage;
> >  use PVE::Storage::Common qw(
> >      plugin_get_vtype_subdir
> >  );
> > +use PVE::Storage::Common::Parse qw(
> > +    parse_path_as_volname_parts
> > +);
> >
> >  use base qw(PVE::RESTHandler);
> >
> > @@ -71,11 +74,13 @@ my sub parse_transferred_file_path_extension : prototype($$) {
> >      my ($path, $vtype) = @_;
> >
> >      if ($vtype eq 'iso') {
> > -        if ($path !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
> > +        my $parts = parse_path_as_volname_parts($path, $vtype);
> > +
> > +        if (!defined($parts)) {
> >              raise_param_exc({ filename => "wrong file extension" });
> >          }
> >
> > -        my $ext = $1;
> > +        my $ext = $parts->{ext};
> >          return $ext;
> >      }
> >
> > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> > index 1b0569dd..d7d683ad 100755
> > --- a/src/PVE/Storage.pm
> > +++ b/src/PVE/Storage.pm
> > @@ -26,6 +26,9 @@ use PVE::Storage::Common qw(
> >      plugin_get_default_vtype_subdirs
> >      plugin_get_vtype_subdir
> >  );
> > +use PVE::Storage::Common::Parse qw(
> > +    parse_path_as_volid
> > +);
> >  use PVE::RESTEnvironment qw(log_warn);
> >
> >  use PVE::Storage::Plugin;
> > @@ -754,9 +757,7 @@ sub path_to_volume_id {
> >          }
> >
> >          if ($vtype eq 'iso') {
> > -            return if $filename !~ m!/([^/]+$ISO_EXT_RE_0)$!;
> > -            my $name = $1;
> > -            return "$sid:iso/$name";
> > +            return parse_path_as_volid($sid, $scfg, $path, $vtype);
> >          }
> >
> >          if ($vtype eq 'vztmpl') {
> > diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
> > index abccf52b..49defb98 100644
> > --- a/src/PVE/Storage/Common.pm
> > +++ b/src/PVE/Storage/Common.pm
> > @@ -55,6 +55,8 @@ be grouped in a submodule can also be found here.
> >
> >  =over
> >
> > +=item * C<L<PVE::Storage::Common::Parse>>
> > +
> >  =back
> >
> >  =head1 STANDARD OPTIONS FOR JSON SCHEMA
> > diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
> > index 0c4bba5b..0d9b1be1 100644
> > --- a/src/PVE/Storage/Common/Makefile
> > +++ b/src/PVE/Storage/Common/Makefile
> > @@ -1,4 +1,5 @@
> >  SOURCES = \
> > +	  Parse.pm			\
> >
> >
> >  .PHONY: install
> > diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm
> > new file mode 100644
> > index 00000000..bcc6f9fc
> > --- /dev/null
> > +++ b/src/PVE/Storage/Common/Parse.pm
> > @@ -0,0 +1,330 @@
> > +package PVE::Storage::Common::Parse;
> > +
> > +use v5.36;
> > +
> > +use PVE::Storage::Common qw(
> > +    plugin_get_vtype_subdir
> > +);
> > +
> > +use Exporter qw(import);
> > +
> > +our @EXPORT_OK = qw(
> > +    parse_path_as_volname_parts
> > +    parse_path_as_volname
> > +    parse_volname_as_parts
> > +
> > +    parse_path_as_volid_parts
> > +    parse_path_as_volid
> > +    parse_volid_as_parts
> > +);
> > +
> > +=head1 NAME
> > +
> > +C<PVE::Storage::Common::Parse> - Storage-related Parsing Functions
> > +
> > +=head1 DESCRIPTION
> > +
> > +This module contains various parsing functions for use within C<L<PVE::Storage>>,
> > +its submodules (including storage plugins) and other related modules.
> > +
> > +Parsing functions are categorized by their main purpose / area of application
> > +and may be further subdivided depending on what kind of data type they are
> > +primarily related to.
> > +
> > +=cut
> > +
> > +my $RE_PARENT_DIR = quotemeta('..');
> > +my $RE_CONTAINS_PARENT_DIR = qr!
> > +    ( ^$RE_PARENT_DIR/ )  #  ../ --> Beginning of path
> > +    |
> > +    ( /$RE_PARENT_DIR/ )  # /../ --> Between two path components
> > +    |
> > +    ( /$RE_PARENT_DIR$ )  # /..  --> End of path
> > +!xn;
> > +
> > +my $RE_ISO_FILE_PATH = qr!
> > +    (?<path>
> > +        (?<file> [^/]+ \. (?<ext> (?i: iso|img) ) )
> > +    )
> > +!xn;
> > +
> > +my $RE_FILE_PATH_FOR_VTYPE = {
> > +    iso => qr/^$RE_ISO_FILE_PATH$/,
> > +};
> > +
> > +my $RE_VOLNAME_FOR_VTYPE = {
> > +    iso => qr/^$RE_ISO_FILE_PATH$/,
> > +};
> > +
> > +my sub contains_parent_dir($path) {
> > +    return $path =~ $RE_CONTAINS_PARENT_DIR;
> > +}
> > +
> > +my sub strip_leading_path_separators($path) {
> > +    return $path =~ s!^/+!!r;
> > +}
> > +
> > +my sub strip_trailing_path_separators($path) {
> > +    return $path =~ s!/+$!!r;
> > +}
> > +
> > +my sub format_named_groups(%groups) {
> > +    my $result = {};
> > +
> > +    for my $old_key (keys %groups) {
> > +        my $new_key = $old_key =~ s/_/-/gr;
> > +        $result->{$new_key} = $groups{$old_key};
> > +    }
> > +
> > +    my @disk_path_components = ();
> > +
> > +    if (defined($result->{file})) {
> > +        $result->{file} = strip_leading_path_separators($result->{file});
> > +        push(@disk_path_components, $result->{file});
> > +    }
> > +
> > +    if (scalar(@disk_path_components)) {
> > +        $result->{'disk-path'} = join('/', @disk_path_components);
> > +    }
> > +
> > +    return $result;
> > +}
> > +
> > +my sub split_leading_dir_from_path($path, $directory) {
> > +    $directory = strip_trailing_path_separators($directory);
> > +
> > +    if ($path =~ m!^(?<leading_dir> \Q$directory\E ) / (?<remainder> .* ) $ !xn) {
> > +        return {
> > +            'leading-dir' => strip_trailing_path_separators($+{leading_dir}),
> > +            remainder => strip_leading_path_separators($+{remainder}),
>
> Do we really want a hash here?
> The 'leading-dir' one is literally `$directory` 99% of the time - with
> $directory getting additional sanitization here, so maybe just `return
> ($directory, $rest);`
>
> That said... for consistency within the file I won't block it based on
> that, but I'm not happy about it...

Nah, I'm partial about it -- I simply used a hash for consistency's
sake, as I didn't deem the code to be performance-critical or anything.

>
> Code wise, we could use `substr` with a `''` replacement.
> Since we match an exact string we already have in a var, it's fine to
> throw it away. Then again I also wouldn't be surprised if perl's RE
> engine performed better than the following:
>
>     if ("$directory/" eq substr($path, 0, length($directory) + 1, '')) {
>         return ($directory, $path);
>     }

ACK, that seems to work; thanks for the suggestion! Will include this in
v2.

>
> > +        };
> > +    }
> > +
> > +    return;
> > +}
> > +
> > +=head1 PARSERS RELATED TO VOLUMES
> > +
> > +The parsing functions in this section primarily deal with parsing data related
> > +to storage volumes, primarily C<L<volname>>s and C<L<volid>>s.
> > +
> > +=head2 VOLUME NAMES
> > +
> > +=cut
> > +
> > +=head3 parse_path_as_volname_parts
> > +
> > +Parses the given relative file path C<$path> according to the given C<$vtype>
> > +and returns a hashref containing the individually extracted parts that make up
> > +a C<volname> on success.
> > +
> > +On failure or when the provided C<$vtype> is not supported or does not exist,
> > +returns C<undef> instead.
> > +
> > +B<NOTE:> This function assumes that C<$path> is already relative to the
> > +directory that corresponds to the given C<$vtype>.
>
> I wonder if we should then distinguish the sub as
> `parse_relative_path_as_volname_parts`?

Yeah, agree with you here. Will do the same for `parse_path_as_volname`,
since that takes a relative path as well.

Actually, I might just make it explicit for each sub whether it takes an
absolute or relative path, even if it's a bit verbose, just to make it
clear at call sites what kind of path is passed to a given parsing
function. Might make it a little easier to use, and also a little easier
to understand for anyone reading the code later.

Thanks for the suggestion, will be included in v2!

>
> > +
> > +For example, the path C</var/lib/vz/template/iso/custom-debian.iso> points to an
> > +ISO file on the default C<local> directory storage. In this case, the storage
> > +path is C</var/lib/vz>, the subdirectory for the C<$vtype = "iso"> is
> > +C<template/iso>, and C<$path> is C<custom-debian.iso>. To illustrate:
> > +
> > +    /var/lib/vz/template/iso/custom-debian.iso
> > +    └┬────────┘ └┬─────────┘ └┬──────────────┘
> > +     │           │            │
> > +     │           │           $path
> > +     │          subdir for
> > +     │          $vtype = "iso"
> > +     │
> > +     storage path
> > +
> > +If you want to parse an absolute path for an already known storage instead, see
> > +C<L<< parse_path_as_volid_parts()|/"parse_path_as_volid_parts" >>>.
> > +
> > +For a counterpart to this function, see
> > +C<L<< parse_volname_as_parts()|/"parse_volname_as_parts" >>>.
> > +
> > +=cut
> > +
> > +sub parse_path_as_volname_parts : prototype($$) ($path, $vtype) {
> > +    return if contains_parent_dir($path);
> > +
> > +    # TODO: vtype split: Handle parsing for 'images' and 'rootdir' vtypes.
> > +
> > +    my $re_filepath = $RE_FILE_PATH_FOR_VTYPE->{$vtype};
> > +    return if !defined($re_filepath);
> > +
> > +    return if $path !~ $re_filepath;
> > +
> > +    my $parts = format_named_groups(%+);
> > +    $parts->{vtype} = $vtype;
> > +    $parts->{volname} = $vtype . '/' . $parts->{path};
> > +
> > +    return $parts;
> > +}
> > +
> > +=head3 parse_path_as_volname
> > +
> > +Like C<L<< parse_path_as_volname_parts()|/"parse_path_as_volname_parts" >>>, but
> > +instead of extracting the individual parts of the relative C<$path>, returns
> > +the correctly formatted C<volname> directly.
> > +
> > +For a counterpart to this function, see
> > +C<L<< parse_volname_as_parts()|/"parse_volname_as_parts" >>>.
> > +
> > +=cut
> > +
> > +sub parse_path_as_volname : prototype($$) ($path, $vtype) {
> > +    return if contains_parent_dir($path);
> > +
> > +    # TODO: vtype split: Handle parsing for 'images' and 'rootdir' vtypes.
> > +
> > +    my $re_filepath = $RE_FILE_PATH_FOR_VTYPE->{$vtype};
> > +    return if !defined($re_filepath);
> > +
> > +    return if $path !~ $re_filepath;
> > +
> > +    return $vtype . '/' . $+{path};
> > +}
> > +
> > +=head3 parse_volname_as_parts
> > +
> > +Parses the provided C<$volname> and returns its constituent parts in a hashref
> > +upon success.
> > +
> > +Returns C<undef> if C<$volname> is prefixed with an unknown or unsupported
> > +C<vtype>, or if the path after the C<vtype> prefix cannot be parsed.
>
> Technically returns an empty list - this *can* make a difference...

Good point; will correct this. Thanks!

>
> > +
> > +This function can be seen as a counterpart to
> > +C<L<< parse_path_as_volname_parts()|/"parse_path_as_volname_parts" >>> and
> > +C<L<< parse_path_as_volname()|/"parse_path_as_volname" >>> and can be used to
> > +extract the information embedded within an already existing volume name, such
> > +as file extensions or the name of the file that a volume refers to.
> > +
> > +=cut
> > +
> > +sub parse_volname_as_parts : prototype($) ($volname) {
> > +    # TODO: vtype split: Handle volname for 'images' and 'rootdir' vtypes.
> > +    my ($vtype, $path) = split('/', $volname, 2);
> > +
> > +    # Either variable could be undef or an empty string here
> > +    return if !$vtype || !$path;
> > +
> > +    return if contains_parent_dir($path);
> > +
> > +    my $re_volname = $RE_VOLNAME_FOR_VTYPE->{$vtype};
> > +    return if !defined($re_volname);
> > +
> > +    return if $path !~ $re_volname;
> > +
> > +    my $parts = format_named_groups(%+);
> > +    $parts->{vtype} = $vtype;
> > +    $parts->{volname} = $volname;
> > +
> > +    return $parts;
> > +}
> > +
> > +=head2 VOLUME IDS
> > +
> > +=cut
> > +
> > +my $RE_VOLID = qr!
> > +    ^
> > +    (?<volid>
> > +        (?<storeid> (?i: [a-z][a-z0-9\-\_\.]*[a-z0-9] ) )
> > +        :    # separated by colon
> > +        (?<volname> .+)
> > +    )
> > +    $
> > +!xn;
> > +
> > +=head3 parse_path_as_volid_parts
> > +
> > +Parses the given absolute file path C<$path> according to the given C<$vtype>
> > +and returns a hashref containing the individually extracted parts that make up
> > +a C<$volid>, but only if C<$path> is valid for C<$storeid> and its config
> > +C<$scfg>.
> > +
> > +On failure or when the provided C<$vtype> is not supported or does not exist,
> > +returns C<undef> instead.
>
> Also: technically returns an empty list.

Same as above; will be corrected in v2.

>
> > +
> > +B<NOTE:> Unlike C<L<< parse_path_as_volname_parts()|/"parse_path_as_volname_parts" >>>,
> > +this function requires that C<$path> is prefixed with the directory for the
> > +given C<$vtype>. To illustrate:
> > +
> > +    directory for $vtype = "iso"
> > +     │
> > +     │
> > +    ┌┴─────────────────────┐
> > +    │                      │
> > +    /var/lib/vz/template/iso/custom-debian.iso
> > +    │                                        │
> > +    └┬───────────────────────────────────────┘
> > +     │
> > +     │
> > +    $path
> > +
> > +If you are only interested in parsing the portion of the path belonging inside
> > +the C<vtype> directory independent of a C<$storeid>, see
> > +C<L<< parse_path_as_volname_parts()|/"parse_path_as_volname_parts" >>>.
> > +
> > +For a counterpart to this function, see
> > +C<L<< parse_volid_as_parts()|/"parse_volid_as_parts" >>>.
> > +
> > +=cut
> > +
> > +sub parse_path_as_volid_parts : prototype($$$$) ($storeid, $scfg, $path, $vtype) {
> > +    my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype);
> > +
> > +    my $split_path_parts = split_leading_dir_from_path($path, $vtype_subdir);
> > +    return if !defined($split_path_parts);
> > +
> > +    my $volid_parts = parse_path_as_volname_parts($split_path_parts->{remainder}, $vtype);
> > +    return if !defined($volid_parts);
> > +
> > +    $volid_parts->{volid} = $storeid . ':' . $volid_parts->{volname};
> > +
> > +    return $volid_parts;
> > +}
> > +
> > +=head3 parse_path_as_volid
> > +
> > +Like C<L<< parse_path_as_volid_parts()|/"parse_path_as_volid_parts" >>>, but
> > +instead of extracting the individual parts of the C<$path>, returns the
> > +correctly formatted C<volid> directly.
> > +
> > +For a counterpart to this function, see
> > +C<L<< parse_volid_as_parts()|/"parse_volid_as_parts" >>>.
> > +
> > +=cut
> > +
> > +sub parse_path_as_volid : prototype($$$$) ($storeid, $scfg, $path, $vtype) {
> > +    my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype);
> > +
> > +    my $split_path_parts = split_leading_dir_from_path($path, $vtype_subdir);
> > +    return if !defined($split_path_parts);
> > +
> > +    my $volname = parse_path_as_volname($split_path_parts->{remainder}, $vtype);
> > +    return if !defined($volname);
> > +
> > +    return $storeid . ':' . $volname;
> > +}
> > +
> > +=head3 parse_volid_as_parts
> > +
> > +Parses the provided C<$volid> and returns its C<storeid> and C<volname> parts
> > +in a hashref upon success and C<undef> otherwise.
>
> undef => empty list

Same as above; will be corrected in v2.

>
> > +
> > +The C<volname> part of volumes that represent files can be further parsed into
> > +its constituent parts using the
> > +C<L<< parse_volname_as_parts()|/"parse_volname_as_parts" >>> function.
> > +
> > +=cut
> > +
> > +sub parse_volid_as_parts : prototype($) ($volid) {
> > +    return if $volid !~ $RE_VOLID;
> > +
> > +    return format_named_groups(%+);
> > +}
> > +
> > +1;
> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> > index c96c1ed7..590ba3e0 100644
> > --- a/src/PVE/Storage/Plugin.pm
> > +++ b/src/PVE/Storage/Plugin.pm
> > @@ -19,6 +19,10 @@ use PVE::Storage::Common qw(
> >      plugin_get_default_vtype_subdirs
> >      plugin_get_vtype_subdir
> >  );
> > +use PVE::Storage::Common::Parse qw(
> > +    parse_volname_as_parts
> > +    parse_path_as_volid_parts
> > +);
> >
> >  use JSON;
> >
> > @@ -816,8 +820,12 @@ sub parse_volname {
> >          return ('images', $name, $vmid, undef, undef, $isBase, $format);
> >      }
> >
> > -    if ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
> > -        return ('iso', $1, undef, undef, undef, undef, 'raw');
> > +    if (defined(my $parts = parse_volname_as_parts($volname))) {
> > +        my ($vtype, $volume_path) = $parts->@{qw(vtype path)};
> > +
> > +        if ($vtype eq 'iso') {
> > +            return ($vtype, $volume_path, undef, undef, undef, undef, 'raw');
> > +        }
> >      }
> >
> >      if ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
> > @@ -1690,11 +1698,12 @@ my sub get_subdir_files {
> >          }
> >
> >          if ($vtype eq 'iso') {
> > -            return if $filename !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
> > +            my $parts = parse_path_as_volid_parts($storeid, $scfg, $path, $vtype);
> > +            return if !defined($parts);
> >
> >              return {
> > -                volid => "$storeid:iso/$1",
> > -                format => 'iso',
> > +                volid => $parts->{volid},
> > +                format => 'iso', # always 'iso' even if we have a file ending in .img
> >              };
> >          }
> >
> > --
> > 2.47.3
> >
> >
> >
> >
> >