[pve-devel] [PATCH storage v2 1/3] compact regex for backup file filter

[Thomas Lamprecht]

On 1/31/20 5:00 PM, Alwin Antreich wrote:
> this, more compact form of the regex should allow easier addition of new
> file extensions.
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/Storage.pm        | 2 +-
>  PVE/Storage/Plugin.pm | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 0bd103e..1688077 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -514,7 +514,7 @@ sub path_to_volume_id {
>  	} elsif ($path =~ m!^$privatedir/(\d+)$!) {
>  	    my $vmid = $1;
>  	    return ('rootdir', "$sid:rootdir/$vmid");
> -	} elsif ($path =~ m!^$backupdir/([^/]+\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo))$!) {
> +	} elsif ($path =~ m!^$backupdir/([^/]+\.(tgz|((tar|vma)(\.(gz|lzo))?)))$!) {

the "feature" of the old regex was that $2 included the extension,
you want to throw some non-capturing groups (?:) in there for that.

Maybe we could strike a balance by pulling out the compressors into a regex variable?

(tar(?:\.$COMPRESSOR_RE)?|tgz|vma(?:\.$COMPRESSOR_RE))

Would allow for reuse below.

>  	    my $name = $1;
>  	    return ('iso', "$sid:backup/$name");
>  	}
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 0c39cbd..58a801a 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -423,7 +423,7 @@ sub parse_volname {
>  	return ('vztmpl', $1);
>      } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>  	return ('rootdir', $1, $1);
> -    } elsif ($volname =~ m!^backup/([^/]+(\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo)))$!) {
> +    } elsif ($volname =~ m!^backup/([^/]+(\.(tgz|((tar|vma)(\.(gz|lzo))?))))$!) {
>  	my $fn = $1;
>  	if ($fn =~ m/^vzdump-(openvz|lxc|qemu)-(\d+)-.+/) {
>  	    return ('backup', $fn, $2);
> @@ -910,7 +910,7 @@ my $get_subdir_files = sub {
>  
>  	} elsif ($tt eq 'backup') {
>  	    next if defined($vmid) && $fn !~  m/\S+-$vmid-\S+/;
> -	    next if $fn !~ m!/([^/]+\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo))$!;
> +	    next if $fn !~ m!/([^/]+\.(tgz|((tar|vma)(\.(gz|lzo))?)))$!;
>  
>  	    $info = { volid => "$sid:backup/$1", format => $2 };
>  
> 

writing test for this up front as a first patch would be great.
It should be pretty simple, i.e., have a array of maps with params and expected
result as two key => value entries. Additionally "expected error" checks would be
great too.
While there's already a sub test in zfspoolplugin test I'd just do a new one.

This way we would have a safety net for such and future changes.