Re: [pve-devel] [PATCH v10 storage 1/3] factoring out regex'es for backup and vztmpl

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 22.06.21 11:19, Lorenz Stechauner wrote:
> uniformly stores these regex definitions in PVE::Storage.
> 
> One test had to be adapted because it tested obsolete code. Namely:
> it expects vztmpl to only end with .tar.gz, but the new regex also
> includes .tar.xz, there is nothing against allowing .tar.xz files as
> vztmpl files.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/Storage/Status.pm     |  6 +++---
>  PVE/Storage.pm                 | 11 ++++++++---
>  PVE/Storage/Plugin.pm          | 15 +++++++++------
>  test/path_to_volume_id_test.pm |  7 ++++---
>  4 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 8a36aef..7069244 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -423,12 +423,12 @@ __PACKAGE__->register_method ({
>  
>  	if ($content eq 'iso') {
>  	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> -		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
> +		raise_param_exc({ filename => "wrong file extension" });
>  	    }
>  	    $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
>  	} elsif ($content eq 'vztmpl') {
> -	    if ($filename !~ m![^/]+\.tar\.[gx]z$!) {
> -		raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
> +	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
> +		raise_param_exc({ filename => "wrong file extension" });
>  	    }
>  	    $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
>  	} else {
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index ea887a4..519431c 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -101,6 +101,11 @@ PVE::Storage::Plugin->init();
>  
>  our $iso_extension_re = qr/\.(?:iso|img)/i;
>  
> +our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
> +
> +# Caution: because of the '$' inside, this regex may only used to match at the end of strings
> +our $backup_extension_re = qr/\.(tgz$|tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?/i;

should the $ be at the actual end of the regex, not just for .tgz? I mean, then below usage
would need to adapt and it would not be ideal, as it makes fixed choices for any user, but
at least not confusing. Otherwise, maybe even preferring that, I'd drop *any* $ anchor.

our $backup_extension_re = qr/\.(?:(?:(tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?|tgz))/i;

But it really needs to be either all $-anchored or none, a mix is just weird and confusing,
commented or not.

> +
>  #  PVE::Storage utility functions
>  
>  sub config {
> @@ -573,13 +578,13 @@ sub path_to_volume_id {
>  	} elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
>  	    my $name = $1;
>  	    return ('iso', "$sid:iso/$name");
> -	} elsif ($path =~ m!^$tmpldir/([^/]+\.tar\.gz)$!) {
> +	} elsif ($path =~ m!^$tmpldir/([^/]+$vztmpl_extension_re)$!) {
>  	    my $name = $1;
>  	    return ('vztmpl', "$sid:vztmpl/$name");
>  	} elsif ($path =~ m!^$privatedir/(\d+)$!) {
>  	    my $vmid = $1;
>  	    return ('rootdir', "$sid:rootdir/$vmid");
> -	} elsif ($path =~ m!^$backupdir/([^/]+\.(?:tgz|(?:(?:tar|vma)(?:\.(?:${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)))$!) {
> +	} elsif ($path =~ m!^$backupdir/([^/]+$backup_extension_re)$!) {
>  	    my $name = $1;
>  	    return ('backup', "$sid:backup/$name");
>  	} elsif ($path =~ m!^$snippetsdir/([^/]+)$!) {
> @@ -1463,7 +1468,7 @@ sub archive_info {
>      my $info;
>  
>      my $volid = basename($archive);
> -    if ($volid =~ /^(vzdump-(lxc|openvz|qemu)-.+\.(tgz$|tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)$/) {
> +    if ($volid =~ /^(vzdump-(lxc|openvz|qemu)-.+$backup_extension_re)$/) {
>  	my $filename = "$1"; # untaint
>  	my ($type, $format, $comp) = ($2, $3, $4);
>  	my $format_re = defined($comp) ? "$format.$comp" : "$format";
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index d330845..afb05ed 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -518,11 +518,11 @@ sub parse_volname {
>  	return ('images', $name, $vmid, undef, undef, $isBase, $format);
>      } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
>  	return ('iso', $1);
> -    } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
> +    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::vztmpl_extension_re)$!) {
>  	return ('vztmpl', $1);
>      } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>  	return ('rootdir', $1, $1);
> -    } elsif ($volname =~ m!^backup/([^/]+(?:\.(?:tgz|(?:(?:tar|vma)(?:\.(?:${\COMPRESSOR_RE}))?))))$!) {
> +    } elsif ($volname =~ m!^backup/([^/]+$PVE::Storage::backup_extension_re)$!) {
>  	my $fn = $1;
>  	if ($fn =~ m/^vzdump-(openvz|lxc|qemu)-(\d+)-.+/) {
>  	    return ('backup', $fn, $2);
> @@ -1041,15 +1041,18 @@ my $get_subdir_files = sub {
>  	    $info = { volid => "$sid:iso/$1", format => 'iso' };
>  
>  	} elsif ($tt eq 'vztmpl') {
> -	    next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
> +	    next if $fn !~ m!/([^/]+$PVE::Storage::vztmpl_extension_re)$!;
>  
>  	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
>  
>  	} elsif ($tt eq 'backup') {
> -	    next if $fn !~ m!/([^/]+\.(tgz|(?:(?:tar|vma)(?:\.(${\COMPRESSOR_RE}))?)))$!;
> +	    next if $fn !~ m!/([^/]+$PVE::Storage::backup_extension_re)$!;
> +
>  	    my $original = $fn;
> -	    my $format = $2;
> -	    $fn = $1;
> +	    my ($format, $comp);
> +
> +	    ($fn, $format, $comp) = ($1, $2, $3);
> +	    $format = defined($comp) ? "$format.$comp" : $format;
>  
>  	    # only match for VMID now, to avoid false positives (VMID in parent directory name)
>  	    next if defined($vmid) && $fn !~ m/\S+-$vmid-\S+/;
> diff --git a/test/path_to_volume_id_test.pm b/test/path_to_volume_id_test.pm
> index 5eee2f6..e0c46d9 100644
> --- a/test/path_to_volume_id_test.pm
> +++ b/test/path_to_volume_id_test.pm
> @@ -166,12 +166,13 @@ my @tests = (
>  	    'local:snippets/hookscript.pl',
>  	],
>      },
> -
> -    # no matches
>      {
>  	description => 'CT template, tar.xz',
>  	volname     => "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.xz",
> -	expected    => [''],
> +	expected    => [
> +	    'vztmpl',
> +	    'local:vztmpl/debian-10.0-standard_10.0-1_amd64.tar.xz'
> +	],
>      },
>  
>      # no matches, path or files with failures
>