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

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

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...

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);
    }

> +        };
> +    }
> +
> +    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`?

> +
> +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...

> +
> +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.

> +
> +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

> +
> +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
> 
> 
> 
> 
> 

--