From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 68C3375109 for ; Wed, 23 Jun 2021 13:59:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 56467A735 for ; Wed, 23 Jun 2021 13:59:34 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 7A353A725 for ; Wed, 23 Jun 2021 13:59:32 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 46AA046452 for ; Wed, 23 Jun 2021 13:59:32 +0200 (CEST) Message-ID: <2e33bdb5-0b77-9238-2070-562e626068c9@proxmox.com> Date: Wed, 23 Jun 2021 13:59:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:90.0) Gecko/20100101 Thunderbird/90.0 Content-Language: en-US To: Proxmox VE development discussion , Lorenz Stechauner References: <20210622091922.3143130-1-l.stechauner@proxmox.com> <20210622091922.3143130-6-l.stechauner@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210622091922.3143130-6-l.stechauner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.654 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v10 storage 1/3] factoring out regex'es for backup and vztmpl X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Jun 2021 11:59:34 -0000 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 > --- > 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 >