[pve-devel] [PATCH-SERIES v2 storage/manager] factoring out RE for backup extension

[pve-devel] [PATCH-SERIES v2 storage/manager] factoring out RE for backup extension

[Lorenz Stechauner]

changes to v1:
* also renamed iso/vztmpl REs
* new naming schema: $<X>_EXT_RE_<# of capture groups>
    for example: $BACKUP_EXT_RE_2


pve-storage:
Lorenz Stechauner (2):
  storage: rename REs for iso and vztmpl extensions
  storage/plugin: factoring out regex for backup extension re

 PVE/API2/Storage/Status.pm |  8 ++++----
 PVE/Storage.pm             | 23 ++++++++++++++---------
 PVE/Storage/Plugin.pm      | 12 ++++++------
 3 files changed, 24 insertions(+), 19 deletions(-)


pve-manager:
Lorenz Stechauner (1):
  api: aplinfo: rename REs for iso and vztmpl extensions

 PVE/APLInfo.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.30.2

[pve-devel] [PATCH v2 storage 1/2] storage: rename REs for iso and vztmpl extensions

[Lorenz Stechauner]

these changes make it more clear, how many capture groups each
RE inclues.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/API2/Storage/Status.pm |  8 ++++----
 PVE/Storage.pm             | 11 +++++++----
 PVE/Storage/Plugin.pm      |  8 ++++----
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index b838461..3acb647 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -424,12 +424,12 @@ __PACKAGE__->register_method ({
 	my $path;
 
 	if ($content eq 'iso') {
-	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
+	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
 		raise_param_exc({ filename => "wrong file extension" });
 	    }
 	    $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
 	} elsif ($content eq 'vztmpl') {
-	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
+	    if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
 		raise_param_exc({ filename => "wrong file extension" });
 	    }
 	    $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
@@ -584,12 +584,12 @@ __PACKAGE__->register_method({
 
 	my $path;
 	if ($content eq 'iso') {
-	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
+	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
 		raise_param_exc({ filename => "wrong file extension" });
 	    }
 	    $path = PVE::Storage::get_iso_dir($cfg, $storage);
 	} elsif ($content eq 'vztmpl') {
-	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
+	    if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
 		raise_param_exc({ filename => "wrong file extension" });
 	    }
 	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index c04b5a2..b5c2460 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -101,9 +101,12 @@ if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
 # initialize all plugins
 PVE::Storage::Plugin->init();
 
-our $iso_extension_re = qr/\.(?:iso|img)/i;
+# the following REs indicate the number or capture groups via the trailing digit
+# CAUTION don't forget to update the digits accordingly after messing with the capture groups
 
-our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
+our $ISO_EXT_RE_0 = qr/\.(?:iso|img)/i;
+
+our $VZTMPL_EXT_RE_1 = qr/\.tar\.([gx]z)/i;
 
 #  PVE::Storage utility functions
 
@@ -574,10 +577,10 @@ sub path_to_volume_id {
 		    return ('images', $info->{volid});
 		}
 	    }
-	} elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
+	} elsif ($path =~ m!^$isodir/([^/]+$ISO_EXT_RE_0)$!) {
 	    my $name = $1;
 	    return ('iso', "$sid:iso/$name");
-	} elsif ($path =~ m!^$tmpldir/([^/]+$vztmpl_extension_re)$!) {
+	} elsif ($path =~ m!^$tmpldir/([^/]+$VZTMPL_EXT_RE_1)$!) {
 	    my $name = $1;
 	    return ('vztmpl', "$sid:vztmpl/$name");
 	} elsif ($path =~ m!^$privatedir/(\d+)$!) {
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index b1865cb..502951f 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -516,9 +516,9 @@ sub parse_volname {
 	my ($vmid, $name) = ($1, $2);
 	my (undef, $format, $isBase) = parse_name_dir($name);
 	return ('images', $name, $vmid, undef, undef, $isBase, $format);
-    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
+    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
 	return ('iso', $1);
-    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::vztmpl_extension_re)$!) {
+    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
 	return ('vztmpl', $1);
     } elsif ($volname =~ m!^rootdir/(\d+)$!) {
 	return ('rootdir', $1, $1);
@@ -1045,12 +1045,12 @@ my $get_subdir_files = sub {
 	my $info;
 
 	if ($tt eq 'iso') {
-	    next if $fn !~ m!/([^/]+$PVE::Storage::iso_extension_re)$!i;
+	    next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
 
 	    $info = { volid => "$sid:iso/$1", format => 'iso' };
 
 	} elsif ($tt eq 'vztmpl') {
-	    next if $fn !~ m!/([^/]+$PVE::Storage::vztmpl_extension_re)$!;
+	    next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
 
 	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
 
-- 
2.30.2

[pve-devel] [PATCH v2 storage 2/2] storage/plugin: factoring out regex for backup extension re

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/Storage.pm        | 12 +++++++-----
 PVE/Storage/Plugin.pm |  4 ++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index b5c2460..9bc799d 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -108,6 +108,8 @@ our $ISO_EXT_RE_0 = qr/\.(?:iso|img)/i;
 
 our $VZTMPL_EXT_RE_1 = qr/\.tar\.([gx]z)/i;
 
+our $BACKUP_EXT_RE_2 = qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)/i;
+
 #  PVE::Storage utility functions
 
 sub config {
@@ -586,7 +588,7 @@ sub path_to_volume_id {
 	} 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_EXT_RE_2)$!) {
 	    my $name = $1;
 	    return ('backup', "$sid:backup/$name");
 	} elsif ($path =~ m!^$snippetsdir/([^/]+)$!) {
@@ -1471,15 +1473,15 @@ 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_EXT_RE_2)$/) {
 	my $filename = "$1"; # untaint
-	my ($type, $format, $comp) = ($2, $3, $4);
-	my $format_re = defined($comp) ? "$format.$comp" : "$format";
+	my ($type, $extension, $comp) = ($2, $3, $4);
+	(my $format = $extension) =~ s/\..*//;
 	$info = decompressor_info($format, $comp);
 	$info->{filename} = $filename;
 	$info->{type} = $type;
 
-	if ($volid =~ /^(vzdump-${type}-([1-9][0-9]{2,8})-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2}))\.${format_re}$/) {
+	if ($volid =~ /^(vzdump-${type}-([1-9][0-9]{2,8})-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2}))\.${extension}$/) {
 	    $info->{logfilename} = "$1.log";
 	    $info->{vmid} = int($2);
 	    $info->{ctime} = timelocal($8, $7, $6, $5, $4 - 1, $3);
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 502951f..b2ffc26 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -522,7 +522,7 @@ sub parse_volname {
 	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_EXT_RE_2)$!) {
 	my $fn = $1;
 	if ($fn =~ m/^vzdump-(openvz|lxc|qemu)-(\d+)-.+/) {
 	    return ('backup', $fn, $2);
@@ -1055,7 +1055,7 @@ my $get_subdir_files = sub {
 	    $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_EXT_RE_2)$!;
 	    my $original = $fn;
 	    my $format = $2;
 	    $fn = $1;
-- 
2.30.2

[pve-devel] [PATCH v2 manager 1/1] api: aplinfo: rename REs for iso and vztmpl extensions

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/APLInfo.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/APLInfo.pm b/PVE/APLInfo.pm
index 5cee1af8..1eff7107 100644
--- a/PVE/APLInfo.pm
+++ b/PVE/APLInfo.pm
@@ -84,7 +84,7 @@ sub read_aplinfo_from_fh {
 	    my $template;
 	    if ($res->{location}) {
 		$template = $res->{location};
-		$template =~ s|.*/([^/]+$PVE::Storage::vztmpl_extension_re)$|$1|;
+		$template =~ s|.*/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$|$1|;
 		if ($res->{location} !~ m|^([a-zA-Z]+)\://|) {
 		    # relative localtion (no http:// prefix)
 		    $res->{location} = "$source/$res->{location}";
-- 
2.30.2

Re: [pve-devel] [PATCH v2 storage 1/2] storage: rename REs for iso and vztmpl extensions

[Fabian Ebner]

Am 05.08.21 um 09:34 schrieb Lorenz Stechauner:
> these changes make it more clear, how many capture groups each
> RE inclues.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   PVE/API2/Storage/Status.pm |  8 ++++----
>   PVE/Storage.pm             | 11 +++++++----
>   PVE/Storage/Plugin.pm      |  8 ++++----
>   3 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index b838461..3acb647 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -424,12 +424,12 @@ __PACKAGE__->register_method ({
>   	my $path;
>   
>   	if ($content eq 'iso') {
> -	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> +	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
>   		raise_param_exc({ filename => "wrong file extension" });
>   	    }
>   	    $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
>   	} elsif ($content eq 'vztmpl') {
> -	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
> +	    if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
>   		raise_param_exc({ filename => "wrong file extension" });
>   	    }
>   	    $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
> @@ -584,12 +584,12 @@ __PACKAGE__->register_method({
>   
>   	my $path;
>   	if ($content eq 'iso') {
> -	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> +	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
>   		raise_param_exc({ filename => "wrong file extension" });
>   	    }
>   	    $path = PVE::Storage::get_iso_dir($cfg, $storage);
>   	} elsif ($content eq 'vztmpl') {
> -	    if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
> +	    if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
>   		raise_param_exc({ filename => "wrong file extension" });
>   	    }
>   	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index c04b5a2..b5c2460 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -101,9 +101,12 @@ if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
>   # initialize all plugins
>   PVE::Storage::Plugin->init();
>   
> -our $iso_extension_re = qr/\.(?:iso|img)/i;
> +# the following REs indicate the number or capture groups via the trailing digit
> +# CAUTION don't forget to update the digits accordingly after messing with the capture groups
>   
> -our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;

Removing this is not backwards compatible and requires a versioned 
Breaks for pve-manager.

> +our $ISO_EXT_RE_0 = qr/\.(?:iso|img)/i;
> +
> +our $VZTMPL_EXT_RE_1 = qr/\.tar\.([gx]z)/i;
>   
>   #  PVE::Storage utility functions
>   
> @@ -574,10 +577,10 @@ sub path_to_volume_id {
>   		    return ('images', $info->{volid});
>   		}
>   	    }
> -	} elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
> +	} elsif ($path =~ m!^$isodir/([^/]+$ISO_EXT_RE_0)$!) {
>   	    my $name = $1;
>   	    return ('iso', "$sid:iso/$name");
> -	} elsif ($path =~ m!^$tmpldir/([^/]+$vztmpl_extension_re)$!) {
> +	} elsif ($path =~ m!^$tmpldir/([^/]+$VZTMPL_EXT_RE_1)$!) {
>   	    my $name = $1;
>   	    return ('vztmpl', "$sid:vztmpl/$name");
>   	} elsif ($path =~ m!^$privatedir/(\d+)$!) {
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index b1865cb..502951f 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -516,9 +516,9 @@ sub parse_volname {
>   	my ($vmid, $name) = ($1, $2);
>   	my (undef, $format, $isBase) = parse_name_dir($name);
>   	return ('images', $name, $vmid, undef, undef, $isBase, $format);
> -    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
> +    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
>   	return ('iso', $1);
> -    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::vztmpl_extension_re)$!) {
> +    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
>   	return ('vztmpl', $1);
>       } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>   	return ('rootdir', $1, $1);
> @@ -1045,12 +1045,12 @@ my $get_subdir_files = sub {
>   	my $info;
>   
>   	if ($tt eq 'iso') {
> -	    next if $fn !~ m!/([^/]+$PVE::Storage::iso_extension_re)$!i;
> +	    next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
>   
>   	    $info = { volid => "$sid:iso/$1", format => 'iso' };
>   
>   	} elsif ($tt eq 'vztmpl') {
> -	    next if $fn !~ m!/([^/]+$PVE::Storage::vztmpl_extension_re)$!;
> +	    next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
>   
>   	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
>   
> 

Re: [pve-devel] [PATCH v2 storage 2/2] storage/plugin: factoring out regex for backup extension re

[Fabian Ebner]

Am 05.08.21 um 09:34 schrieb Lorenz Stechauner:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   PVE/Storage.pm        | 12 +++++++-----
>   PVE/Storage/Plugin.pm |  4 ++--
>   2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index b5c2460..9bc799d 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -108,6 +108,8 @@ our $ISO_EXT_RE_0 = qr/\.(?:iso|img)/i;
>   
>   our $VZTMPL_EXT_RE_1 = qr/\.tar\.([gx]z)/i;
>   
> +our $BACKUP_EXT_RE_2 = qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)/i;

Please mention in the commit message that this now matches 
case-insensitively, which is different from the current behavior.

But matching case-insensitively doesn't work, because at least 
decompressor_info() cannot handle uppercase extensions currently.

> +
>   #  PVE::Storage utility functions
>   
>   sub config {
> @@ -586,7 +588,7 @@ sub path_to_volume_id {
>   	} 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_EXT_RE_2)$!) {
>   	    my $name = $1;
>   	    return ('backup', "$sid:backup/$name");
>   	} elsif ($path =~ m!^$snippetsdir/([^/]+)$!) {
> @@ -1471,15 +1473,15 @@ 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_EXT_RE_2)$/) {
>   	my $filename = "$1"; # untaint
> -	my ($type, $format, $comp) = ($2, $3, $4);
> -	my $format_re = defined($comp) ? "$format.$comp" : "$format";
> +	my ($type, $extension, $comp) = ($2, $3, $4);
> +	(my $format = $extension) =~ s/\..*//;
>   	$info = decompressor_info($format, $comp);
>   	$info->{filename} = $filename;
>   	$info->{type} = $type;
>   
> -	if ($volid =~ /^(vzdump-${type}-([1-9][0-9]{2,8})-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2}))\.${format_re}$/) {
> +	if ($volid =~ /^(vzdump-${type}-([1-9][0-9]{2,8})-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2}))\.${extension}$/) {
>   	    $info->{logfilename} = "$1.log";
>   	    $info->{vmid} = int($2);
>   	    $info->{ctime} = timelocal($8, $7, $6, $5, $4 - 1, $3);
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 502951f..b2ffc26 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -522,7 +522,7 @@ sub parse_volname {
>   	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_EXT_RE_2)$!) {
>   	my $fn = $1;
>   	if ($fn =~ m/^vzdump-(openvz|lxc|qemu)-(\d+)-.+/) {
>   	    return ('backup', $fn, $2);
> @@ -1055,7 +1055,7 @@ my $get_subdir_files = sub {
>   	    $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_EXT_RE_2)$!;
>   	    my $original = $fn;
>   	    my $format = $2;
>   	    $fn = $1;
> 

Re: [pve-devel] [PATCH v2 storage 1/2] storage: rename REs for iso and vztmpl extensions

[Lorenz Stechauner]

On 13.08.21 12:09, Fabian Ebner wrote:
> Am 05.08.21 um 09:34 schrieb Lorenz Stechauner:
>> these changes make it more clear, how many capture groups each
>> RE inclues.
>>
>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>> ---
>>   PVE/API2/Storage/Status.pm |  8 ++++----
>>   PVE/Storage.pm             | 11 +++++++----
>>   PVE/Storage/Plugin.pm      |  8 ++++----
>>   3 files changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
>> index b838461..3acb647 100644
>> --- a/PVE/API2/Storage/Status.pm
>> +++ b/PVE/API2/Storage/Status.pm
>> @@ -424,12 +424,12 @@ __PACKAGE__->register_method ({
>>       my $path;
>>         if ($content eq 'iso') {
>> -        if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
>> +        if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
>>           raise_param_exc({ filename => "wrong file extension" });
>>           }
>>           $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
>>       } elsif ($content eq 'vztmpl') {
>> -        if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
>> +        if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
>>           raise_param_exc({ filename => "wrong file extension" });
>>           }
>>           $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
>> @@ -584,12 +584,12 @@ __PACKAGE__->register_method({
>>         my $path;
>>       if ($content eq 'iso') {
>> -        if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
>> +        if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
>>           raise_param_exc({ filename => "wrong file extension" });
>>           }
>>           $path = PVE::Storage::get_iso_dir($cfg, $storage);
>>       } elsif ($content eq 'vztmpl') {
>> -        if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
>> +        if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
>>           raise_param_exc({ filename => "wrong file extension" });
>>           }
>>           $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index c04b5a2..b5c2460 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -101,9 +101,12 @@ if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
>>   # initialize all plugins
>>   PVE::Storage::Plugin->init();
>>   -our $iso_extension_re = qr/\.(?:iso|img)/i;
>> +# the following REs indicate the number or capture groups via the 
>> trailing digit
>> +# CAUTION don't forget to update the digits accordingly after 
>> messing with the capture groups
>>   -our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
>
> Removing this is not backwards compatible and requires a versioned 
> Breaks for pve-manager.
makes sense. should I split this commit in two, where in the first part 
I add the new variable and in the second part remove the old one? or is 
it enough to state the need for "breaks" in the commit message?
>
>> +our $ISO_EXT_RE_0 = qr/\.(?:iso|img)/i;
>> +
>> +our $VZTMPL_EXT_RE_1 = qr/\.tar\.([gx]z)/i;
>>     #  PVE::Storage utility functions
>>   @@ -574,10 +577,10 @@ sub path_to_volume_id {
>>               return ('images', $info->{volid});
>>           }
>>           }
>> -    } elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
>> +    } elsif ($path =~ m!^$isodir/([^/]+$ISO_EXT_RE_0)$!) {
>>           my $name = $1;
>>           return ('iso', "$sid:iso/$name");
>> -    } elsif ($path =~ m!^$tmpldir/([^/]+$vztmpl_extension_re)$!) {
>> +    } elsif ($path =~ m!^$tmpldir/([^/]+$VZTMPL_EXT_RE_1)$!) {
>>           my $name = $1;
>>           return ('vztmpl', "$sid:vztmpl/$name");
>>       } elsif ($path =~ m!^$privatedir/(\d+)$!) {
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index b1865cb..502951f 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -516,9 +516,9 @@ sub parse_volname {
>>       my ($vmid, $name) = ($1, $2);
>>       my (undef, $format, $isBase) = parse_name_dir($name);
>>       return ('images', $name, $vmid, undef, undef, $isBase, $format);
>> -    } elsif ($volname =~ 
>> m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
>> +    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
>>       return ('iso', $1);
>> -    } elsif ($volname =~ 
>> m!^vztmpl/([^/]+$PVE::Storage::vztmpl_extension_re)$!) {
>> +    } elsif ($volname =~ 
>> m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
>>       return ('vztmpl', $1);
>>       } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>>       return ('rootdir', $1, $1);
>> @@ -1045,12 +1045,12 @@ my $get_subdir_files = sub {
>>       my $info;
>>         if ($tt eq 'iso') {
>> -        next if $fn !~ m!/([^/]+$PVE::Storage::iso_extension_re)$!i;
>> +        next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
>>             $info = { volid => "$sid:iso/$1", format => 'iso' };
>>         } elsif ($tt eq 'vztmpl') {
>> -        next if $fn !~ m!/([^/]+$PVE::Storage::vztmpl_extension_re)$!;
>> +        next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
>>             $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
>>

Re: [pve-devel] [PATCH v2 storage 1/2] storage: rename REs for iso and vztmpl extensions

[Fabian Ebner]

Am 16.08.21 um 10:46 schrieb Lorenz Stechauner:
> 
> On 13.08.21 12:09, Fabian Ebner wrote:
>> Am 05.08.21 um 09:34 schrieb Lorenz Stechauner:
>>> these changes make it more clear, how many capture groups each
>>> RE inclues.
>>>
>>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>>> ---
>>>   PVE/API2/Storage/Status.pm |  8 ++++----
>>>   PVE/Storage.pm             | 11 +++++++----
>>>   PVE/Storage/Plugin.pm      |  8 ++++----
>>>   3 files changed, 15 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
>>> index b838461..3acb647 100644
>>> --- a/PVE/API2/Storage/Status.pm
>>> +++ b/PVE/API2/Storage/Status.pm
>>> @@ -424,12 +424,12 @@ __PACKAGE__->register_method ({
>>>       my $path;
>>>         if ($content eq 'iso') {
>>> -        if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
>>> +        if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
>>>           raise_param_exc({ filename => "wrong file extension" });
>>>           }
>>>           $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
>>>       } elsif ($content eq 'vztmpl') {
>>> -        if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
>>> +        if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
>>>           raise_param_exc({ filename => "wrong file extension" });
>>>           }
>>>           $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
>>> @@ -584,12 +584,12 @@ __PACKAGE__->register_method({
>>>         my $path;
>>>       if ($content eq 'iso') {
>>> -        if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
>>> +        if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
>>>           raise_param_exc({ filename => "wrong file extension" });
>>>           }
>>>           $path = PVE::Storage::get_iso_dir($cfg, $storage);
>>>       } elsif ($content eq 'vztmpl') {
>>> -        if ($filename !~ m![^/]+$PVE::Storage::vztmpl_extension_re$!) {
>>> +        if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
>>>           raise_param_exc({ filename => "wrong file extension" });
>>>           }
>>>           $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>> index c04b5a2..b5c2460 100755
>>> --- a/PVE/Storage.pm
>>> +++ b/PVE/Storage.pm
>>> @@ -101,9 +101,12 @@ if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
>>>   # initialize all plugins
>>>   PVE::Storage::Plugin->init();
>>>   -our $iso_extension_re = qr/\.(?:iso|img)/i;
>>> +# the following REs indicate the number or capture groups via the 
>>> trailing digit
>>> +# CAUTION don't forget to update the digits accordingly after 
>>> messing with the capture groups
>>>   -our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
>>
>> Removing this is not backwards compatible and requires a versioned 
>> Breaks for pve-manager.
> makes sense. should I split this commit in two, where in the first part 
> I add the new variable and in the second part remove the old one? or is 
> it enough to state the need for "breaks" in the commit message?

If we want to do the Breaks now, it's enough to mention it. I don't 
think you need to split the patch.
If we don't want to do the Breaks now, adding a reminder comment like
     # FIXME: Remove with PVE 8.0 and add a versioned breaks for pve-manager
is probably the way to go.

And which of those we want, is for the release team to decide.

>>
>>> +our $ISO_EXT_RE_0 = qr/\.(?:iso|img)/i;
>>> +
>>> +our $VZTMPL_EXT_RE_1 = qr/\.tar\.([gx]z)/i;
>>>     #  PVE::Storage utility functions
>>>   @@ -574,10 +577,10 @@ sub path_to_volume_id {
>>>               return ('images', $info->{volid});
>>>           }
>>>           }
>>> -    } elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
>>> +    } elsif ($path =~ m!^$isodir/([^/]+$ISO_EXT_RE_0)$!) {
>>>           my $name = $1;
>>>           return ('iso', "$sid:iso/$name");
>>> -    } elsif ($path =~ m!^$tmpldir/([^/]+$vztmpl_extension_re)$!) {
>>> +    } elsif ($path =~ m!^$tmpldir/([^/]+$VZTMPL_EXT_RE_1)$!) {
>>>           my $name = $1;
>>>           return ('vztmpl', "$sid:vztmpl/$name");
>>>       } elsif ($path =~ m!^$privatedir/(\d+)$!) {
>>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>>> index b1865cb..502951f 100644
>>> --- a/PVE/Storage/Plugin.pm
>>> +++ b/PVE/Storage/Plugin.pm
>>> @@ -516,9 +516,9 @@ sub parse_volname {
>>>       my ($vmid, $name) = ($1, $2);
>>>       my (undef, $format, $isBase) = parse_name_dir($name);
>>>       return ('images', $name, $vmid, undef, undef, $isBase, $format);
>>> -    } elsif ($volname =~ 
>>> m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
>>> +    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
>>>       return ('iso', $1);
>>> -    } elsif ($volname =~ 
>>> m!^vztmpl/([^/]+$PVE::Storage::vztmpl_extension_re)$!) {
>>> +    } elsif ($volname =~ 
>>> m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
>>>       return ('vztmpl', $1);
>>>       } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>>>       return ('rootdir', $1, $1);
>>> @@ -1045,12 +1045,12 @@ my $get_subdir_files = sub {
>>>       my $info;
>>>         if ($tt eq 'iso') {
>>> -        next if $fn !~ m!/([^/]+$PVE::Storage::iso_extension_re)$!i;
>>> +        next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
>>>             $info = { volid => "$sid:iso/$1", format => 'iso' };
>>>         } elsif ($tt eq 'vztmpl') {
>>> -        next if $fn !~ m!/([^/]+$PVE::Storage::vztmpl_extension_re)$!;
>>> +        next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
>>>             $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
>>>