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 3CFDC1FF0E5 for ; Wed, 29 Jul 2026 15:02:44 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0122C20AF5; Wed, 29 Jul 2026 15:02:44 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 29 Jul 2026 15:02:15 +0200 Message-Id: To: "Wolfgang Bumiller" From: "Max R. Carrara" Subject: Re: [PATCH pve-storage v1 25/54] tree-wide: replace usages of BACKUP_EXT_RE_2 with parsing functions X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20260422111322.257380-1-m.carrara@proxmox.com> <20260422111322.257380-26-m.carrara@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785330128752 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.025 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust 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: OJVQ2MVYWARUDP2EWF3NGFYRQ2XTOM3M X-Message-ID-Hash: OJVQ2MVYWARUDP2EWF3NGFYRQ2XTOM3M X-MailFrom: m.carrara@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 Tue Jun 30, 2026 at 4:06 PM CEST, Wolfgang Bumiller wrote: > 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 =3D $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/P= arse.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 =3D ('gz', 'xz', 'zst', 'bz2'); > > > > +my @BACKUP_COMPRESSION_EXTENSIONS =3D ('gz', 'lzo', 'zst', 'bz2'); > > + > > my sub join_to_re_alternations(@list) { > > return join('|', map { quotemeta } @list); > > } > > > > my $RE_VZTMPL_COMPRESSION_EXTENSIONS =3D join_to_re_alternations(@VZTM= PL_COMPRESSION_EXTENSIONS); > > > > +my $RE_BACKUP_COMPRESSION_EXTENSIONS =3D join_to_re_alternations(@BACK= UP_COMPRESSION_EXTENSIONS); > > + > > my $RE_PARENT_DIR =3D quotemeta('..'); > > my $RE_CONTAINS_PARENT_DIR =3D qr! > > ( ^$RE_PARENT_DIR/ ) # ../ --> Beginning of path > > @@ -50,6 +54,11 @@ my $RE_CONTAINS_PARENT_DIR =3D 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=3Dpve-common.git;a=3Dblob;f=3Dsrc/PV= E/JSONSchema.pm;h=3D17e7126a6c42f8326a1da890680af10b6106d906;hb=3Drefs/head= s/master#l62 > > +# See also: https://git.proxmox.com/?p=3Dpve-common.git;a=3Dblob;f=3Ds= rc/PVE/JSONSchema.pm;h=3D17e7126a6c42f8326a1da890680af10b6106d906;hb=3Drefs= /heads/master#l304 > > +my $RE_VMID =3D qr![1-9][0-9]{2,8}!; > > + > > my $RE_ISO_FILE_PATH =3D qr! > > (? > > (? [^/]+ \. (? (?i: iso|img) ) ) > > @@ -69,14 +78,44 @@ my $RE_VZTMPL_FILE_PATH =3D qr! > > ) > > !xn; > > > > +my $RE_GUEST_VZDUMP_BACKUP_FILE_NAME =3D qr! > > + vzdump > > + - (? openvz|lxc|qemu) > > + - (? $RE_VMID) > > + - [^/]+ > > +!xn; > > + > > +my $RE_BACKUP_FILE_PATH =3D qr! > > + (? > > + (? > > + ( > > + ($RE_GUEST_VZDUMP_BACKUP_FILE_NAME) > > + | > > + ([^/]+) > > + ) > > + \. > > + (? > > + (? tgz) > > + | > > + ( > > + (? tar|vma) > > + ( \. (? $RE_BACKUP_COMPRESSION_EX= TENSIONS) )? > > + ) > > + ) > > + ) > > + ) > > +!xn; > > + > > my $RE_FILE_PATH_FOR_VTYPE =3D { > > iso =3D> qr/^$RE_ISO_FILE_PATH$/, > > vztmpl =3D> qr/^$RE_VZTMPL_FILE_PATH$/, > > + backup =3D> qr/^$RE_BACKUP_FILE_PATH$/, > > }; > > > > my $RE_VOLNAME_FOR_VTYPE =3D { > > iso =3D> qr/^$RE_ISO_FILE_PATH$/, > > vztmpl =3D> qr/^$RE_VZTMPL_FILE_PATH$/, > > + backup =3D> 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/Stor= age/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 =3D [ > > }, > > ]; > > > > +my $volname_cases_backup_valid =3D [ > > + { > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vma', > > + expected =3D> { > > + type =3D> 'qemu', > > + vmid =3D> '16110', > > + file =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vma', > > + ext =3D> 'vma', > > + 'ext-archive' =3D> 'vma', > > + 'disk-path' =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vm= a', > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vma', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-qemu-16110-2020_03_30-21_13_55= .vma', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > > + expected =3D> { > > + type =3D> 'qemu', > > + vmid =3D> '16110', > > + file =3D> 'vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > > + ext =3D> 'vma.gz', > > + 'ext-archive' =3D> 'vma', > > + 'ext-compression' =3D> 'gz', > > + 'disk-path' =3D> 'vzdump-qemu-16110-2020_03_30-21_11_40.vm= a.gz', > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-qemu-16110-2020_03_30-21_11_40= .vma.gz', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > > + expected =3D> { > > + type =3D> 'qemu', > > + vmid =3D> '16110', > > + file =3D> 'vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > > + ext =3D> 'vma.lzo', > > + 'ext-archive' =3D> 'vma', > > + 'ext-compression' =3D> 'lzo', > > + 'disk-path' =3D> 'vzdump-qemu-16110-2020_03_30-21_12_45.vm= a.lzo', > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-qemu-16110-2020_03_30-21_12_45= .vma.lzo', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > > + expected =3D> { > > + type =3D> 'qemu', > > + vmid =3D> '16110', > > + file =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > > + ext =3D> 'vma.zst', > > + 'ext-archive' =3D> 'vma', > > + 'ext-compression' =3D> 'zst', > > + 'disk-path' =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vm= a.zst', > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-qemu-16110-2020_03_30-21_13_55= .vma.zst', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > > + expected =3D> { > > + type =3D> 'lxc', > > + vmid =3D> '16112', > > + file =3D> 'vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > > + ext =3D> 'tar.lzo', > > + 'ext-archive' =3D> 'tar', > > + 'ext-compression' =3D> 'lzo', > > + 'disk-path' =3D> 'vzdump-lxc-16112-2020_03_30-21_39_30.tar= .lzo', > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-lxc-16112-2020_03_30-21_39_30.= tar.lzo', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > > + expected =3D> { > > + type =3D> 'lxc', > > + vmid =3D> '16112', > > + file =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > > + ext =3D> 'tar.gz', > > + 'ext-archive' =3D> 'tar', > > + 'ext-compression' =3D> 'gz', > > + 'disk-path' =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar= .gz', > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-lxc-16112-2020_03_30-21_49_30.= tar.gz', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > > + expected =3D> { > > + type =3D> 'lxc', > > + vmid =3D> '16112', > > + file =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > > + ext =3D> 'tar.zst', > > + 'ext-archive' =3D> 'tar', > > + 'ext-compression' =3D> 'zst', > > + 'disk-path' =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar= .zst', > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-lxc-16112-2020_03_30-21_49_30.= tar.zst', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > > + expected =3D> { > > + type =3D> 'lxc', > > + vmid =3D> '16112', > > + file =3D> 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > > + ext =3D> 'tgz', > > + 'ext-archive' =3D> 'tgz', > > + 'disk-path' =3D> 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz= ', > > + path =3D> 'vzdump-lxc-16112-2020_03_30-21_59_30.tgz', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-lxc-16112-2020_03_30-21_59_30.= tgz', > > + }, > > + }, > > + { > > + path =3D> 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', > > + expected =3D> { > > + type =3D> 'openvz', > > + vmid =3D> '16112', > > + file =3D> 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2= ', > > + ext =3D> 'tar.bz2', > > + 'ext-archive' =3D> 'tar', > > + 'ext-compression' =3D> 'bz2', > > + 'disk-path' =3D> 'vzdump-openvz-16112-2020_03_30-21_39_30.= tar.bz2', > > + path =3D> 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2= ', > > + vtype =3D> 'backup', > > + volname =3D> 'backup/vzdump-openvz-16112-2020_03_30-21_39_= 30.tar.bz2', > > + }, > > + }, > > +]; > > + > > +my $volname_cases_backup_invalid =3D [ > > + { > > + description =3D> "Invalid file extension (1) (backup)", > > + args =3D> { > > + path =3D> 'vzdump-openvz-16112-2020_03_30-21_39_30.tar.zip= ', > > + vtype =3D> 'backup', > > + }, > > + expected =3D> undef, > > + }, > > + { > > + description =3D> "Invalid file extension (2) (backup)", > > + args =3D> { > > + path =3D> 'vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz'= , > > + vtype =3D> 'backup', > > + }, > > + expected =3D> undef, > > + }, > > + { > > + description =3D> "Invalid file extension (3) (backup)", > > + args =3D> { > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz', > > + vtype =3D> 'backup', > > + }, > > + expected =3D> undef, > > + }, > > + { > > + description =3D> "Uppercase letter in file extension (1) (back= up)", > > + args =3D> { > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_12_40.Tar.gz', > > + vtype =3D> 'backup', > > + }, > > + expected =3D> undef, > > + }, > > + { > > + description =3D> "Uppercase letter in file extension (2) (back= up)", > > + args =3D> { > > + path =3D> 'vzdump-qemu-16110-2020_03_30-21_12_40.tar.Gz', > > + vtype =3D> 'backup', > > + }, > > + expected =3D> undef, > > + }, > > +]; > > + > > my $cases_valid_all =3D [ > > $volname_cases_iso_valid, > > $volname_cases_vztmpl_valid, > > + $volname_cases_backup_valid, > > ]; > > > > my $cases_invalid_all =3D [ > > $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 =3D> ('gz', 'lzo', 'zst', 'bz2'= ); > > use constant COMPRESSOR_RE =3D> 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 =3D~ m!^backup/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$= !) { > > - my $fn =3D $1; > > - if ($fn =3D~ 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 =3D~ 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 =3D parse_path_as_volid_parts($storeid, $scfg, $= path, $vtype); > > + return if !defined($parts); > > > > - my $original =3D $path; > > - my $format =3D $2; > > - $filename =3D $1; > > + my $format =3D $parts->{ext}; > > + my $volume_path =3D $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+/; > > =E2=86=91 This strictly requires the `$filename` to contain a VMID=E2=80= =A6 > > > + # Check if parsed VMID matched provided VMID in order to a= void > > + # false positives (VMID in parent directory name) > > + my $parsed_vmid =3D $parts->{vmid}; > > =E2=80=A6 the regex used above has a generic non-guest case in its `(?)` > part matching `([^/]+)`, so `vmid` can indeed be undefined=E2=80=A6 > > > + if (defined($vmid) && defined($parsed_vmid)) { > > + return if $vmid ne $parsed_vmid; > > =E2=80=A6 but this =E2=86=91, 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. Ah, very good point... I double-checked how the method behaves when e.g. a backup named `some-backup.tar.gz` is in the subdir, but a `vmid` is passed without this patch -- we do indeed not include the backup in the resulting list. I'll adapt this and the accompanying test case that accounts for this. Thanks for pointing this out, will be fixed in v2! > > > + } > > > > > my $info =3D { > > - volid =3D> "$storeid:backup/$filename", > > + volid =3D> $parts->{volid}, > > format =3D> $format, > > }; > > > > - my $archive_info =3D eval { PVE::Storage::archive_info($fi= lename) } // {}; > > + my $archive_info =3D eval { PVE::Storage::archive_info($vo= lume_path) } // {}; > > > > $info->{ctime} =3D $archive_info->{ctime} if defined($arch= ive_info->{ctime}); > > $info->{subtype} =3D $archive_info->{type} // 'unknown'; > > > > - if (defined($vmid) || $filename =3D~ m!\-([1-9][0-9]{2,8})= \-[^/]+\.${format}$!) { > > - $info->{vmid} =3D $vmid // $1; > > + if (defined($vmid) || defined($parsed_vmid)) { > > + $info->{vmid} =3D $vmid // $parsed_vmid; > > } > > > > - my $notes_filename =3D $original . NOTES_EXT; > > + my $notes_filename =3D $path . NOTES_EXT; > > if (-f $notes_filename) { > > my $notes =3D PVE::Tools::file_read_firstline($notes_f= ilename); > > $info->{notes} =3D eval { decode('UTF-8', $notes, 1) }= // $notes > > if defined($notes); > > } > > > > - $info->{protected} =3D 1 if -e PVE::Storage::protection_fi= le_path($original); > > + $info->{protected} =3D 1 if -e PVE::Storage::protection_fi= le_path($path); > > > > return $info; > > } > > -- > > 2.47.3 > > > > > > > > > >