From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 88EE81FF146 for ; Tue, 28 Apr 2026 17:07:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3DFF317F79; Tue, 28 Apr 2026 17:07:25 +0200 (CEST) Date: Tue, 28 Apr 2026 17:07:19 +0200 From: Wolfgang Bumiller To: "Max R. Carrara" Subject: Re: [PATCH pve-storage v1 09/54] plugin: adapt get_subdir_files helper of list_volumes API method Message-ID: References: <20260422111322.257380-1-m.carrara@proxmox.com> <20260422111322.257380-10-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260422111322.257380-10-m.carrara@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777388743580 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.088 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [plugin.pm] Message-ID-Hash: MKFMBXQH7L5I27ZRUFLWYCTL3SKF56DT X-Message-ID-Hash: MKFMBXQH7L5I27ZRUFLWYCTL3SKF56DT 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: Mostly fine. On Wed, Apr 22, 2026 at 01:12:35PM +0200, Max R. Carrara wrote: > Pull the body of the loop inside `get_subdir_files()` out into a > closure and break up the if-elsif chain into more readable if-clauses. > > Strip the vtype subdirectory from the beginning of the file paths > being worked on inside that closure. This does not have an effect on > how any of the subsequent regexes match on the path / file name, > because they match at the end of a given path. > > Additionally, take take the storage config hash `$scfg` instead of the > volume type subdir `$path` as parameter. Passing `$scfg` along > makes it possible to obtain the subdir for the given `$vtype` inside > the helper directly instead of passing both the vtype and its > associated subdirectory along. > > All of these things make future changes easier to manage. > > Signed-off-by: Max R. Carrara > --- > src/PVE/Storage/Plugin.pm | 95 ++++++++++++++++++++++++++------------- > 1 file changed, 64 insertions(+), 31 deletions(-) > > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index 2d76f452..c4eb6f18 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -1673,37 +1673,54 @@ sub list_images { > > # $vtype = > my sub get_subdir_files { > - my ($storeid, $path, $vtype, $vmid) = @_; > + my ($storeid, $scfg, $vtype, $vmid) = @_; > + > + my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype); > > my $res = []; > > - for my $filename (<$path/*>) { > - my $st = File::stat::stat($filename); > + my $get_subdir_file_info = sub { > + my ($path, $st) = @_; > > - next if (!$st || S_ISDIR($st->mode)); > - > - my $info; > + # Strip the vtype subdir from beginning of the current path > + # so that we don't have to take it into account when parsing the file name > + my $filename = $path; > + if ($filename !~ s!^\Q$vtype_subdir\E!!) { It might be good to also look-ahead for a slash here. That is, add a `(?=/)` after the `\E`. (Alternatively just do `\E/!/!` (to re-insert the slash after the removal)) This won't change the result, but if custom subdirs match by prefix we won't do unnecessary work. (eg. if we have `foo` for one vtype and `foobar` for the other, `foo` will match the `foobar/...` paths) (I know this later gets changed by the `Parse` module, but something similar probably applies there.) > + return; > + } > > if ($vtype eq 'iso') { > - next if $filename !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i; > + return if $filename !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i; > > - $info = { volid => "$storeid:iso/$1", format => 'iso' }; > + return { > + volid => "$storeid:iso/$1", > + format => 'iso', > + }; > + } > > - } elsif ($vtype eq 'vztmpl') { > - next if $filename !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!; > + if ($vtype eq 'vztmpl') { > + return if $filename !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!; > > - $info = { volid => "$storeid:vztmpl/$1", format => $2 eq 'tar' ? $2 : "t$2" }; > + return { > + volid => "$storeid:vztmpl/$1", > + format => $2 eq 'tar' ? $2 : "t$2", > + }; > + } > > - } elsif ($vtype eq 'backup') { > - next if $filename !~ m!/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$!; > - my $original = $filename; > + if ($vtype eq 'backup') { > + return if $filename !~ m!/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$!; > + > + my $original = $path; > my $format = $2; > $filename = $1; > > # only match for VMID now, to avoid false positives (VMID in parent directory name) > - next if defined($vmid) && $filename !~ m/\S+-$vmid-\S+/; > + return if defined($vmid) && $filename !~ m/\S+-$vmid-\S+/; > > - $info = { volid => "$storeid:backup/$filename", format => $format }; > + my $info = { > + volid => "$storeid:backup/$filename", > + format => $format, > + }; > > my $archive_info = eval { PVE::Storage::archive_info($filename) } // {}; > > @@ -1722,24 +1739,42 @@ my sub get_subdir_files { > } > > $info->{protected} = 1 if -e PVE::Storage::protection_file_path($original); > - } elsif ($vtype eq 'snippets') { > > - $info = { > + return $info; > + } > + > + if ($vtype eq 'snippets') { > + return { > volid => "$storeid:snippets/" . basename($filename), > format => 'snippet', > }; > - } elsif ($vtype eq 'import') { > - next > + } > + > + if ($vtype eq 'import') { > + return > if $filename !~ > m!/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$!i; > > - $info = { volid => "$storeid:import/$1", format => "$2" }; > + return { > + volid => "$storeid:import/$1", > + format => "$2", > + }; > } > > - $info->{size} = $st->size; > - $info->{ctime} //= $st->ctime; > + return; > + }; > > - push @$res, $info; > + for my $path (<$vtype_subdir/*>) { > + my $st = File::stat::stat($path); > + > + next if (!$st || S_ISDIR($st->mode)); > + > + if (defined(my $info = $get_subdir_file_info->($path, $st))) { > + $info->{size} = $st->size; > + $info->{ctime} //= $st->ctime; > + > + push $res->@*, $info; > + } > } > > return $res; > @@ -1758,18 +1793,16 @@ sub list_volumes { > if ($type eq 'images' || $type eq 'rootdir') { > $data = $class->list_images($storeid, $scfg, $vmid); > } elsif ($scfg->{path}) { > - my $path = plugin_get_vtype_subdir($scfg, $type); > - > if ($type eq 'iso' && !defined($vmid)) { > - $data = get_subdir_files($storeid, $path, 'iso'); > + $data = get_subdir_files($storeid, $scfg, 'iso', undef); > } elsif ($type eq 'vztmpl' && !defined($vmid)) { > - $data = get_subdir_files($storeid, $path, 'vztmpl'); > + $data = get_subdir_files($storeid, $scfg, 'vztmpl', undef); > } elsif ($type eq 'backup') { > - $data = get_subdir_files($storeid, $path, 'backup', $vmid); > + $data = get_subdir_files($storeid, $scfg, 'backup', $vmid); > } elsif ($type eq 'snippets') { > - $data = get_subdir_files($storeid, $path, 'snippets'); > + $data = get_subdir_files($storeid, $scfg, 'snippets', undef); > } elsif ($type eq 'import') { > - $data = get_subdir_files($storeid, $path, 'import'); > + $data = get_subdir_files($storeid, $scfg, 'import', undef); > } > } > > -- > 2.47.3 > > > > > --