* [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension re
@ 2021-08-02 10:52 Lorenz Stechauner
2021-08-03 7:15 ` [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension rey Wolfgang Bumiller
0 siblings, 1 reply; 4+ messages in thread
From: Lorenz Stechauner @ 2021-08-02 10:52 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
changes to v1:
* factored $compressor_extension_re out of $backup_extension_re
should now be less confusing
PVE/Storage.pm | 14 +++++++++-----
PVE/Storage/Plugin.pm | 4 ++--
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index c04b5a2..942246f 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -105,6 +105,10 @@ our $iso_extension_re = qr/\.(?:iso|img)/i;
our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
+our $compressor_extension_re = qr/\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})/i;
+
+our $backup_extension_re = qr/\.(tgz|(?:tar|vma)$compressor_extension_re?)/i;
+
# PVE::Storage utility functions
sub config {
@@ -583,7 +587,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_extension_re)$!) {
my $name = $1;
return ('backup', "$sid:backup/$name");
} elsif ($path =~ m!^$snippetsdir/([^/]+)$!) {
@@ -1468,15 +1472,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_extension_re)$/) {
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 b1865cb..9e7eb7d 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_extension_re)$!) {
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_extension_re)$!;
my $original = $fn;
my $format = $2;
$fn = $1;
--
2.30.2
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension rey
2021-08-02 10:52 [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension re Lorenz Stechauner
@ 2021-08-03 7:15 ` Wolfgang Bumiller
2021-08-04 7:56 ` Lorenz Stechauner
0 siblings, 1 reply; 4+ messages in thread
From: Wolfgang Bumiller @ 2021-08-03 7:15 UTC (permalink / raw)
To: Lorenz Stechauner; +Cc: pve-devel
On Mon, Aug 02, 2021 at 12:52:36PM +0200, Lorenz Stechauner wrote:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
> changes to v1:
> * factored $compressor_extension_re out of $backup_extension_re
> should now be less confusing
not sure about less confusing... but I suppose it'll have to do
>
> PVE/Storage.pm | 14 +++++++++-----
> PVE/Storage/Plugin.pm | 4 ++--
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index c04b5a2..942246f 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -105,6 +105,10 @@ our $iso_extension_re = qr/\.(?:iso|img)/i;
>
> our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
>
> +our $compressor_extension_re = qr/\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})/i;
> +
> +our $backup_extension_re = qr/\.(tgz|(?:tar|vma)$compressor_extension_re?)/i;
The reason I don't find it less confusing is that both of these are
globals, and one introduces 1 capture group, the other introduces 2
capture groups.
I'd say "let's just use named capture groups everywhere", but then if we
ever add branch reset patterns (`(?|a|b|c)`) it all falls apart...
I think we should probably add doc comments at least... also maybe
introduce a naming scheme?
$COMPRESSOR_EXTENSION_RE_1
$BACKUP_EXTENSION_AND_COMPRESSION_RE_2
or something? Not sure the numbering is a good idea, but at least naming
one "X_AND_Y" shows there are 2 things involved, and might hint future
editors that adding more groups should also update the name.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension rey
2021-08-03 7:15 ` [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension rey Wolfgang Bumiller
@ 2021-08-04 7:56 ` Lorenz Stechauner
2021-08-04 8:21 ` Wolfgang Bumiller
0 siblings, 1 reply; 4+ messages in thread
From: Lorenz Stechauner @ 2021-08-04 7:56 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pve-devel
On 03.08.21 09:15, Wolfgang Bumiller wrote:
> On Mon, Aug 02, 2021 at 12:52:36PM +0200, Lorenz Stechauner wrote:
>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>> ---
>> changes to v1:
>> * factored $compressor_extension_re out of $backup_extension_re
>> should now be less confusing
> not sure about less confusing... but I suppose it'll have to do
>
>> PVE/Storage.pm | 14 +++++++++-----
>> PVE/Storage/Plugin.pm | 4 ++--
>> 2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index c04b5a2..942246f 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -105,6 +105,10 @@ our $iso_extension_re = qr/\.(?:iso|img)/i;
>>
>> our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
>>
>> +our $compressor_extension_re = qr/\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})/i;
>> +
>> +our $backup_extension_re = qr/\.(tgz|(?:tar|vma)$compressor_extension_re?)/i;
> The reason I don't find it less confusing is that both of these are
> globals, and one introduces 1 capture group, the other introduces 2
> capture groups.
how about using on all three regex'es (iso, vztmpl, backup) two capture
groups?
it would be a bit less confusing, because it's more consistent (?)
".tar.gz" -> $1 = "tar.gz"; $2 = "gz"
".iso" -> $1 = "iso"; $2 = ""
would something like this be a good (or at least better) idea?
our $ISO_EXT_RE = qr/\.(iso|img)()/i;
our $VZTMPL_EXT_RE = qr/\.(tar\.([gx]z))/i;
our $BACKUP_EXT_RE =
qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)/i;
>
> I'd say "let's just use named capture groups everywhere", but then if we
> ever add branch reset patterns (`(?|a|b|c)`) it all falls apart...
>
> I think we should probably add doc comments at least... also maybe
> introduce a naming scheme?
>
> $COMPRESSOR_EXTENSION_RE_1
> $BACKUP_EXTENSION_AND_COMPRESSION_RE_2
>
> or something? Not sure the numbering is a good idea, but at least naming
> one "X_AND_Y" shows there are 2 things involved, and might hint future
> editors that adding more groups should also update the name.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension rey
2021-08-04 7:56 ` Lorenz Stechauner
@ 2021-08-04 8:21 ` Wolfgang Bumiller
0 siblings, 0 replies; 4+ messages in thread
From: Wolfgang Bumiller @ 2021-08-04 8:21 UTC (permalink / raw)
To: Lorenz Stechauner; +Cc: pve-devel
On Wed, Aug 04, 2021 at 09:56:14AM +0200, Lorenz Stechauner wrote:
>
> On 03.08.21 09:15, Wolfgang Bumiller wrote:
> > On Mon, Aug 02, 2021 at 12:52:36PM +0200, Lorenz Stechauner wrote:
> > > Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> > > ---
> > > changes to v1:
> > > * factored $compressor_extension_re out of $backup_extension_re
> > > should now be less confusing
> > not sure about less confusing... but I suppose it'll have to do
> >
> > > PVE/Storage.pm | 14 +++++++++-----
> > > PVE/Storage/Plugin.pm | 4 ++--
> > > 2 files changed, 11 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> > > index c04b5a2..942246f 100755
> > > --- a/PVE/Storage.pm
> > > +++ b/PVE/Storage.pm
> > > @@ -105,6 +105,10 @@ our $iso_extension_re = qr/\.(?:iso|img)/i;
> > > our $vztmpl_extension_re = qr/\.tar\.([gx]z)/i;
> > > +our $compressor_extension_re = qr/\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})/i;
> > > +
> > > +our $backup_extension_re = qr/\.(tgz|(?:tar|vma)$compressor_extension_re?)/i;
> > The reason I don't find it less confusing is that both of these are
> > globals, and one introduces 1 capture group, the other introduces 2
> > capture groups.
> how about using on all three regex'es (iso, vztmpl, backup) two capture
> groups?
> it would be a bit less confusing, because it's more consistent (?)
>
> ".tar.gz" -> $1 = "tar.gz"; $2 = "gz"
> ".iso" -> $1 = "iso"; $2 = ""
>
> would something like this be a good (or at least better) idea?
>
> our $ISO_EXT_RE = qr/\.(iso|img)()/i;
Part of me is making a 🤪 face here...
> our $VZTMPL_EXT_RE = qr/\.(tar\.([gx]z))/i;
> our $BACKUP_EXT_RE =
> qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))?)/i;
BUt also, part of me *does* like this more (though maybe with
`((iso|img))` to have `$2 eq $1` on the ISO one... but I'm not sure
there'll ever be a "perfect" version anyway... I guess doc comments to
point out the differences are more important than the actual REs...
Your choice.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-04 8:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-02 10:52 [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension re Lorenz Stechauner
2021-08-03 7:15 ` [pve-devel] [PATCH v2 storage] storage/plugin: factoring out regex for backup extension rey Wolfgang Bumiller
2021-08-04 7:56 ` Lorenz Stechauner
2021-08-04 8:21 ` Wolfgang Bumiller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal