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

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

[Lorenz Stechauner]

changes to v2:
* rebased to current master
* kept old $vztmpl_extension_re to not break pve-manager and added
    # FIXME [...]

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             | 26 +++++++++++++++++---------
 PVE/Storage/Plugin.pm      | 12 ++++++------
 3 files changed, 27 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 v3 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             | 14 ++++++++++----
 PVE/Storage/Plugin.pm      |  8 ++++----
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 02c970f..e028423 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -437,12 +437,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});
@@ -618,12 +618,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 71d6ad7..4e1b1f7 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -101,9 +101,15 @@ 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\.(gz|xz|zst)/i;
+our $ISO_EXT_RE_0 = qr/\.(?:iso|img)/i;
+
+our $VZTMPL_EXT_RE_1 = qr/\.tar\.(gz|xz|zst)/i;
+
+# FIXME remove with PVE 8.0, add versioned breaks for pve-manager
+our $vztmpl_extension_re = $VZTMPL_EXT_RE_1;
 
 #  PVE::Storage utility functions
 
@@ -574,10 +580,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 aeb4fff..770f439 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -563,9 +563,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);
@@ -1093,12 +1093,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 v3 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 4e1b1f7..5688dd7 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\.(gz|xz|zst)/i;
 
+our $BACKUP_EXT_RE_2 = qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)/;
+
 # FIXME remove with PVE 8.0, add versioned breaks for pve-manager
 our $vztmpl_extension_re = $VZTMPL_EXT_RE_1;
 
@@ -589,7 +591,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/([^/]+)$!) {
@@ -1474,15 +1476,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 770f439..f691d9b 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -569,7 +569,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);
@@ -1103,7 +1103,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 v3 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-SERIES v3 storage/manager] factoring out RE for backup extension

[Dominik Csapak]

Series LGTM, looked closely at the regexes,
and they match (semantically), tested around a bit
and all seemed to work.

one super small nit (not a blocker IMHO)

the format/extension/compression handling in 2/2
could maybe use a comment what we do there
(though maybe it's just me who's confused ^^)

On 10/22/21 14:23, Lorenz Stechauner wrote:
> changes to v2:
> * rebased to current master
> * kept old $vztmpl_extension_re to not break pve-manager and added
>      # FIXME [...]
> 
> 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             | 26 +++++++++++++++++---------
>   PVE/Storage/Plugin.pm      | 12 ++++++------
>   3 files changed, 27 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(-)
> 

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

[Thomas Lamprecht]

On 04.11.21 15:40, Dominik Csapak wrote:
> Series LGTM, looked closely at the regexes,
> and they match (semantically), tested around a bit
> and all seemed to work.
> 
> one super small nit (not a blocker IMHO)
> 
> the format/extension/compression handling in 2/2
> could maybe use a comment what we do there
> (though maybe it's just me who's confused ^^)
> 

can you send a follow up with what you have in mind?

> On 10/22/21 14:23, Lorenz Stechauner wrote:
>> changes to v2:
>> * rebased to current master
>> * kept old $vztmpl_extension_re to not break pve-manager and added
>>      # FIXME [...]
>>
>> 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             | 26 +++++++++++++++++---------
>>   PVE/Storage/Plugin.pm      | 12 ++++++------
>>   3 files changed, 27 insertions(+), 19 deletions(-)

applied the storage part, thanks!

Re: [pve-devel] applied: [PATCH-SERIES v3 storage/manager] factoring out RE for backup extension

[Dominik Csapak]

On 3/16/22 17:16, Thomas Lamprecht wrote:
> On 04.11.21 15:40, Dominik Csapak wrote:
>> Series LGTM, looked closely at the regexes,
>> and they match (semantically), tested around a bit
>> and all seemed to work.
>>
>> one super small nit (not a blocker IMHO)
>>
>> the format/extension/compression handling in 2/2
>> could maybe use a comment what we do there
>> (though maybe it's just me who's confused ^^)
>>
> 
> can you send a follow up with what you have in mind?
> 

honestly i cannot totally remember what i meant, but i guess the line

(my $format = $extension) =~ s/\..*//;

confused me at first, but in now it's obvious to me that we strip the
compression from the extension...

should i send a patch to comment that ?

Re: [pve-devel] applied: [PATCH-SERIES v3 storage/manager] factoring out RE for backup extension

[Thomas Lamprecht]

On 17.03.22 08:51, Dominik Csapak wrote:
> On 3/16/22 17:16, Thomas Lamprecht wrote:
>> On 04.11.21 15:40, Dominik Csapak wrote:
>>> Series LGTM, looked closely at the regexes,
>>> and they match (semantically), tested around a bit
>>> and all seemed to work.
>>>
>>> one super small nit (not a blocker IMHO)
>>>
>>> the format/extension/compression handling in 2/2
>>> could maybe use a comment what we do there
>>> (though maybe it's just me who's confused ^^)
>>>
>>
>> can you send a follow up with what you have in mind?
>>
> 
> honestly i cannot totally remember what i meant, but i guess the line
> 
> (my $format = $extension) =~ s/\..*//;
> 
> confused me at first, but in now it's obvious to me that we strip the
> compression from the extension...

yeah with the variable name its relatively clear what's happening, so no
hard feelings.

> 
> should i send a patch to comment that ?

your call

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

[Fiona Ebner]

Am 22.10.21 um 14:23 schrieb 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}";

applied, thanks!

Stumbled across this, because the comment in pve-storage was wrong as
this never got applied:
https://lists.proxmox.com/pipermail/pve-devel/2024-June/064326.html


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel