From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id D138B1FF141 for ; Tue, 30 Jun 2026 16:06:52 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 283D821425; Tue, 30 Jun 2026 16:06:52 +0200 (CEST) Date: Tue, 30 Jun 2026 16:06:15 +0200 From: Wolfgang Bumiller To: "Max R. Carrara" Subject: Re: [PATCH pve-storage v1 25/54] tree-wide: replace usages of BACKUP_EXT_RE_2 with parsing functions Message-ID: References: <20260422111322.257380-1-m.carrara@proxmox.com> <20260422111322.257380-26-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260422111322.257380-26-m.carrara@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782828363765 X-SPAM-LEVEL: Spam detection results: 0 DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 4U6TVA42WDLR4MOOY5GJEKVNQNI2KTRQ X-Message-ID-Hash: 4U6TVA42WDLR4MOOY5GJEKVNQNI2KTRQ X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, Apr 22, 2026 at 01:12:51PM +0200, Max R. Carrara wrote: > Add support for the 'backup' volume type in > `PVE::Storage::Common::Parse`. > > Also add corresponding test cases. > > Use the module's parsing functions to replace usages of the > `PVE::Storage::BACKUP_EXT_RE_2` regex across the repository. > > Note that the new regex for parsing 'backup' vtype file paths is also > able to optionally extract the VMID and the type of guest from the > file name. This makes it possible to further simplify some parts of > the original logic. > > As done with the `ISO_EXT_RE_0` and `VZTMPL_EXT_RE_1` regexes, keep > the `BACKUP_EXT_RE_2` regex around for now, since it's public and > therefore also part of the storage API. On an APIVER + APIAGE bump, > this regex should be marked for removal as well, like the others. > > Additionally, the `COMPRESSOR_RE` constant in `PVE::Storage::Plugin` > is now unused. Together with its related `KNOWN_COMPRESSION_FORMATS` > constant, it should be phased out and replaced eventually. Add a TODO > comment for that. > > In the meantime, keep track of the compression formats for specific > vtypes in the `::Common::Parse` module until we move them to a more > adequate place. > > Signed-off-by: Max R. Carrara > --- > src/PVE/Storage.pm | 4 +- > src/PVE/Storage/Common/Parse.pm | 39 +++++ > src/PVE/Storage/Common/test/parser_tests.pl | 181 ++++++++++++++++++++ > src/PVE/Storage/Plugin.pm | 37 ++-- > 4 files changed, 240 insertions(+), 21 deletions(-) > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index d5b0b637..ce6d2154 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -765,9 +765,7 @@ sub path_to_volume_id { > } > > if ($vtype eq 'backup') { > - return if $filename !~ m!/([^/]+$BACKUP_EXT_RE_2)$!; > - my $name = $1; > - return "$sid:backup/$name"; > + return parse_path_as_volid($sid, $scfg, $path, $vtype); > } > > if ($vtype eq 'snippets') { > diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm > index b18ef576..950f6b18 100644 > --- a/src/PVE/Storage/Common/Parse.pm > +++ b/src/PVE/Storage/Common/Parse.pm > @@ -35,12 +35,16 @@ primarily related to. > > my @VZTMPL_COMPRESSION_EXTENSIONS = ('gz', 'xz', 'zst', 'bz2'); > > +my @BACKUP_COMPRESSION_EXTENSIONS = ('gz', 'lzo', 'zst', 'bz2'); > + > my sub join_to_re_alternations(@list) { > return join('|', map { quotemeta } @list); > } > > my $RE_VZTMPL_COMPRESSION_EXTENSIONS = join_to_re_alternations(@VZTMPL_COMPRESSION_EXTENSIONS); > > +my $RE_BACKUP_COMPRESSION_EXTENSIONS = join_to_re_alternations(@BACKUP_COMPRESSION_EXTENSIONS); > + > my $RE_PARENT_DIR = quotemeta('..'); > my $RE_CONTAINS_PARENT_DIR = qr! > ( ^$RE_PARENT_DIR/ ) # ../ --> Beginning of path > @@ -50,6 +54,11 @@ my $RE_CONTAINS_PARENT_DIR = qr! > ( /$RE_PARENT_DIR$ ) # /.. --> End of path > !xn; > > +# A vmid is an integer between 100 and 999_999_999 > +# See: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/JSONSchema.pm;h=17e7126a6c42f8326a1da890680af10b6106d906;hb=refs/heads/master#l62 > +# See also: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/JSONSchema.pm;h=17e7126a6c42f8326a1da890680af10b6106d906;hb=refs/heads/master#l304 > +my $RE_VMID = qr![1-9][0-9]{2,8}!; > + > my $RE_ISO_FILE_PATH = qr! > (? > (? [^/]+ \. (? (?i: iso|img) ) ) > @@ -69,14 +78,44 @@ my $RE_VZTMPL_FILE_PATH = qr! > ) > !xn; > > +my $RE_GUEST_VZDUMP_BACKUP_FILE_NAME = qr! > + vzdump > + - (? openvz|lxc|qemu) > + - (? $RE_VMID) > + - [^/]+ > +!xn; > + > +my $RE_BACKUP_FILE_PATH = qr! > + (? > + (? > + ( > + ($RE_GUEST_VZDUMP_BACKUP_FILE_NAME) > + | > + ([^/]+) > + ) > + \. > + (? > + (? tgz) > + | > + ( > + (? tar|vma) > + ( \. (? $RE_BACKUP_COMPRESSION_EXTENSIONS) )? > + ) > + ) > + ) > + ) > +!xn; > + > my $RE_FILE_PATH_FOR_VTYPE = { > iso => qr/^$RE_ISO_FILE_PATH$/, > vztmpl => qr/^$RE_VZTMPL_FILE_PATH$/, > + backup => qr/^$RE_BACKUP_FILE_PATH$/, > }; > > my $RE_VOLNAME_FOR_VTYPE = { > iso => qr/^$RE_ISO_FILE_PATH$/, > vztmpl => qr/^$RE_VZTMPL_FILE_PATH$/, > + backup => qr/^$RE_BACKUP_FILE_PATH$/, > }; > > my sub contains_parent_dir($path) { > diff --git a/src/PVE/Storage/Common/test/parser_tests.pl b/src/PVE/Storage/Common/test/parser_tests.pl > index 31575b66..96159608 100755 > --- a/src/PVE/Storage/Common/test/parser_tests.pl > +++ b/src/PVE/Storage/Common/test/parser_tests.pl > @@ -193,14 +193,195 @@ my $volname_cases_vztmpl_invalid = [ > }, > ]; > > +my $volname_cases_backup_valid = [ > + { > + path => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma', > + expected => { > + type => 'qemu', > + vmid => '16110', > + file => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma', > + ext => 'vma', > + 'ext-archive' => 'vma', > + 'disk-path' => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma', > + path => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma', > + vtype => 'backup', > + volname => 'backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma', > + }, > + }, > + { > + path => 'vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > + expected => { > + type => 'qemu', > + vmid => '16110', > + file => 'vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > + ext => 'vma.gz', > + 'ext-archive' => 'vma', > + 'ext-compression' => 'gz', > + 'disk-path' => 'vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > + path => 'vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > + vtype => 'backup', > + volname => 'backup/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > + }, > + }, > + { > + path => 'vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > + expected => { > + type => 'qemu', > + vmid => '16110', > + file => 'vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > + ext => 'vma.lzo', > + 'ext-archive' => 'vma', > + 'ext-compression' => 'lzo', > + 'disk-path' => 'vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > + path => 'vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > + vtype => 'backup', > + volname => 'backup/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > + }, > + }, > + { > + path => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > + expected => { > + type => 'qemu', > + vmid => '16110', > + file => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > + ext => 'vma.zst', > + 'ext-archive' => 'vma', > + 'ext-compression' => 'zst', > + 'disk-path' => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > + path => 'vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > + vtype => 'backup', > + volname => 'backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > + }, > + }, > + { > + path => 'vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > + expected => { > + type => 'lxc', > + vmid => '16112', > + file => 'vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > + ext => 'tar.lzo', > + 'ext-archive' => 'tar', > + 'ext-compression' => 'lzo', > + 'disk-path' => 'vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > + path => 'vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > + vtype => 'backup', > + volname => 'backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > + }, > + }, > + { > + path => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > + expected => { > + type => 'lxc', > + vmid => '16112', > + file => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > + ext => 'tar.gz', > + 'ext-archive' => 'tar', > + 'ext-compression' => 'gz', > + 'disk-path' => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > + path => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > + vtype => 'backup', > + volname => 'backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > + }, > + }, > + { > + path => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > + expected => { > + type => 'lxc', > + vmid => '16112', > + file => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > + ext => 'tar.zst', > + 'ext-archive' => 'tar', > + 'ext-compression' => 'zst', > + 'disk-path' => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > + path => 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > + vtype => 'backup', > + volname => 'backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > + }, > + }, > + { > + path => 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > + expected => { > + type => 'lxc', > + vmid => '16112', > + file => 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > + ext => 'tgz', > + 'ext-archive' => 'tgz', > + 'disk-path' => 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > + path => 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > + vtype => 'backup', > + volname => 'backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > + }, > + }, > + { > + path => 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', > + expected => { > + type => 'openvz', > + vmid => '16112', > + file => 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', > + ext => 'tar.bz2', > + 'ext-archive' => 'tar', > + 'ext-compression' => 'bz2', > + 'disk-path' => 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', > + path => 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', > + vtype => 'backup', > + volname => 'backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', > + }, > + }, > +]; > + > +my $volname_cases_backup_invalid = [ > + { > + description => "Invalid file extension (1) (backup)", > + args => { > + path => 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.zip', > + vtype => 'backup', > + }, > + expected => undef, > + }, > + { > + description => "Invalid file extension (2) (backup)", > + args => { > + path => 'vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz', > + vtype => 'backup', > + }, > + expected => undef, > + }, > + { > + description => "Invalid file extension (3) (backup)", > + args => { > + path => 'vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz', > + vtype => 'backup', > + }, > + expected => undef, > + }, > + { > + description => "Uppercase letter in file extension (1) (backup)", > + args => { > + path => 'vzdump-qemu-16110-2020_03_30-21_12_40.Tar.gz', > + vtype => 'backup', > + }, > + expected => undef, > + }, > + { > + description => "Uppercase letter in file extension (2) (backup)", > + args => { > + path => 'vzdump-qemu-16110-2020_03_30-21_12_40.tar.Gz', > + vtype => 'backup', > + }, > + expected => undef, > + }, > +]; > + > my $cases_valid_all = [ > $volname_cases_iso_valid, > $volname_cases_vztmpl_valid, > + $volname_cases_backup_valid, > ]; > > my $cases_invalid_all = [ > $volname_cases_iso_invalid, > $volname_cases_vztmpl_invalid, > + $volname_cases_backup_invalid, > ]; > > { > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index 6bfa5c11..4d63e0a5 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -28,6 +28,7 @@ use JSON; > > use base qw(PVE::SectionConfig); > > +# TODO: Phase out these two constants > use constant KNOWN_COMPRESSION_FORMATS => ('gz', 'lzo', 'zst', 'bz2'); > use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS); > > @@ -830,14 +831,10 @@ sub parse_volname { > if ($vtype eq 'vztmpl') { > return ($vtype, $volume_path, undef, undef, undef, undef, 'raw'); > } > - } > > - if ($volname =~ m!^backup/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$!) { > - my $fn = $1; > - if ($fn =~ m/^vzdump-(openvz|lxc|qemu)-(\d+)-.+/) { > - return ('backup', $fn, $2, undef, undef, undef, 'raw'); > + if ($vtype eq 'backup') { > + return ($vtype, $volume_path, $parts->{vmid}, undef, undef, undef, 'raw'); > } > - return ('backup', $fn, undef, undef, undef, undef, 'raw'); > } > > if ($volname =~ m!^snippets/([^/]+)$!) { > @@ -1721,37 +1718,41 @@ my sub get_subdir_files { > } > > if ($vtype eq 'backup') { > - return if $filename !~ m!/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$!; > + my $parts = parse_path_as_volid_parts($storeid, $scfg, $path, $vtype); > + return if !defined($parts); > > - my $original = $path; > - my $format = $2; > - $filename = $1; > + my $format = $parts->{ext}; > + my $volume_path = $parts->{path}; > > - # only match for VMID now, to avoid false positives (VMID in parent directory name) > - return if defined($vmid) && $filename !~ m/\S+-$vmid-\S+/; ↑ This strictly requires the `$filename` to contain a VMID… > + # Check if parsed VMID matched provided VMID in order to avoid > + # false positives (VMID in parent directory name) > + my $parsed_vmid = $parts->{vmid}; … the regex used above has a generic non-guest case in its `(?)` part matching `([^/]+)`, so `vmid` can indeed be undefined… > + if (defined($vmid) && defined($parsed_vmid)) { > + return if $vmid ne $parsed_vmid; … but this ↑, now, only fires if there *was* a vmid. So to match the previous behavior we we should probably add an } elif (defined($vmid)) { return; } (or use a better if/else structure) here, otherwise those non-guest files will now be listed for guests as well, whereas before they were filtered out. > + } > > my $info = { > - volid => "$storeid:backup/$filename", > + volid => $parts->{volid}, > format => $format, > }; > > - my $archive_info = eval { PVE::Storage::archive_info($filename) } // {}; > + my $archive_info = eval { PVE::Storage::archive_info($volume_path) } // {}; > > $info->{ctime} = $archive_info->{ctime} if defined($archive_info->{ctime}); > $info->{subtype} = $archive_info->{type} // 'unknown'; > > - if (defined($vmid) || $filename =~ m!\-([1-9][0-9]{2,8})\-[^/]+\.${format}$!) { > - $info->{vmid} = $vmid // $1; > + if (defined($vmid) || defined($parsed_vmid)) { > + $info->{vmid} = $vmid // $parsed_vmid; > } > > - my $notes_filename = $original . NOTES_EXT; > + my $notes_filename = $path . NOTES_EXT; > if (-f $notes_filename) { > my $notes = PVE::Tools::file_read_firstline($notes_filename); > $info->{notes} = eval { decode('UTF-8', $notes, 1) } // $notes > if defined($notes); > } > > - $info->{protected} = 1 if -e PVE::Storage::protection_file_path($original); > + $info->{protected} = 1 if -e PVE::Storage::protection_file_path($path); > > return $info; > } > -- > 2.47.3 > > > > > --