From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B42BC1FF146 for ; Tue, 28 Apr 2026 17:08:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 906D7180B6; Tue, 28 Apr 2026 17:08:11 +0200 (CEST) Date: Tue, 28 Apr 2026 17:08:05 +0200 From: Wolfgang Bumiller To: "Max R. Carrara" Subject: Re: [PATCH pve-storage v1 16/54] storage: clean up code that was moved into helper in path_to_volume_id Message-ID: References: <20260422111322.257380-1-m.carrara@proxmox.com> <20260422111322.257380-17-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260422111322.257380-17-m.carrara@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777388789603 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 Message-ID-Hash: TB37JRQYZCY4PS32NE6FMBJF4NVT3A53 X-Message-ID-Hash: TB37JRQYZCY4PS32NE6FMBJF4NVT3A53 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:42PM +0200, Max R. Carrara wrote: > In order to make the parsing logic in `path_to_volume_id()` both > simpler and more manageable for future changes, do the following: > > 1. Iterate over a list of vtypes that store their contents in > subdirectories and fetch said subdirectories dynamically instead of > hardcoding them. > > 2. Explicitly exclude the `rootdir` vtype in this list of vtypes, as > its subdirectory is currently not used. > > 3. Strip the vtype subdirectory from the beginning of the path in > order to make matching on the file name easier. Return early as a > safeguard if this fails. > > 4. Adapt the original regexes in the parsing helper to not include the > vtype subdir anymore, since this is now being stripped. > > 5. Instead of returning a 2-element array, return `undef` if the path > fails to parse, and the volume ID (volid) alone if parsing > succeeds. Since the caller is now passing along the vtype, it does > not need to be returned anymore. > > Note that the parsing logic of the `parse_volid_from_file_path()` > helper inside `path_to_volume_id()` now is very similar to the parsing > logic of the `get_subdir_file_info()` closure in the > `get_subdir_files()` helper inside `Plugin.pm`. This opens up further > simplification / code de-duplication in the future, so that our > parsing logic can eventually be kept in one place only. > > Signed-off-by: Max R. Carrara > --- > src/PVE/Storage.pm | 73 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 50 insertions(+), 23 deletions(-) > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index 2886ab9e..1b0569dd 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -23,6 +23,7 @@ use PVE::INotify; > use PVE::RPCEnvironment; > use PVE::SSHInfo; > use PVE::Storage::Common qw( > + plugin_get_default_vtype_subdirs > plugin_get_vtype_subdir > ); > use PVE::RESTEnvironment qw(log_warn); > @@ -712,17 +713,31 @@ sub path_to_volume_id { > # for example when nfs storage is not mounted > $path = abs_path($path) || $path; > > + my $get_vtypes_to_check = sub { > + my $default_vtype_subdirs = plugin_get_default_vtype_subdirs(); > + > + # TODO: vtype split: handle directory for 'rootdir' type. > + # CTs currently use the same dir as VMs --> the dir for the 'images' type. > + # Directory is therefore unused right now, so exclude it. > + delete $default_vtype_subdirs->{rootdir}; > + > + return [sort keys $default_vtype_subdirs->%*]; > + }; > + > my $parse_volid_from_file_path = sub { > - my ($plugin, $sid, $scfg) = @_; > + my ($plugin, $sid, $scfg, $vtype) = @_; > > - my $imagedir = plugin_get_vtype_subdir($scfg, 'images'); > - my $isodir = plugin_get_vtype_subdir($scfg, 'iso'); > - my $tmpldir = plugin_get_vtype_subdir($scfg, 'vztmpl'); > - my $backupdir = plugin_get_vtype_subdir($scfg, 'backup'); > - my $snippetsdir = plugin_get_vtype_subdir($scfg, 'snippets'); > - my $importdir = plugin_get_vtype_subdir($scfg, 'import'); > + my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype); > > - if ($path =~ m!^\Q$imagedir\E/(\d+)/([^/\s]+)$!) { > + # 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!!) { Same as in 9/54 - lookahead for a slash may be useful. > + return; > + } > + > + if ($vtype eq 'images') { > + return if $filename !~ m!/(\d+)/([^/\s]+)$!; > my $vmid = $1; > my $name = $2; > > @@ -731,48 +746,60 @@ sub path_to_volume_id { > my ($storeid, $volname) = parse_volume_id($info->{volid}); > my $volpath = $plugin->path($scfg, $volname, $storeid); > if ($volpath eq $path) { > - return ('images', $info->{volid}); > + return $info->{volid}; > } > } > > - return (''); > + return; > } > > - if ($path =~ m!^\Q$isodir\E/([^/]+$ISO_EXT_RE_0)$!) { > + if ($vtype eq 'iso') { > + return if $filename !~ m!/([^/]+$ISO_EXT_RE_0)$!; > my $name = $1; > - return ('iso', "$sid:iso/$name"); > + return "$sid:iso/$name"; > } > > - if ($path =~ m!^\Q$tmpldir\E/([^/]+$VZTMPL_EXT_RE_1)$!) { > + if ($vtype eq 'vztmpl') { > + return if $filename !~ m!/([^/]+$VZTMPL_EXT_RE_1)$!; > my $name = $1; > - return ('vztmpl', "$sid:vztmpl/$name"); > + return "$sid:vztmpl/$name"; > } > > - if ($path =~ m!^\Q$backupdir\E/([^/]+$BACKUP_EXT_RE_2)$!) { > + if ($vtype eq 'backup') { > + return if $filename !~ m!/([^/]+$BACKUP_EXT_RE_2)$!; > my $name = $1; > - return ('backup', "$sid:backup/$name"); > + return "$sid:backup/$name"; > } > > - if ($path =~ m!^\Q$snippetsdir\E/([^/]+)$!) { > + if ($vtype eq 'snippets') { > + return if $filename !~ m!/([^/]+)$!; > my $name = $1; > - return ('snippets', "$sid:snippets/$name"); > + return "$sid:snippets/$name"; > } > > - if ($path =~ m!^\Q$importdir\E/(${SAFE_CHAR_CLASS_RE}+${IMPORT_EXT_RE_1})$!) { > + if ($vtype eq 'import') { > + return if $filename !~ m!/(${SAFE_CHAR_CLASS_RE}+${IMPORT_EXT_RE_1})$!; > my $name = $1; > - return ('import', "$sid:import/$name"); > + return "$sid:import/$name"; > } > > - return (''); > + return; > }; > > + my $vtypes_to_check = $get_vtypes_to_check->(); > + > for my $sid (keys $ids->%*) { > my $scfg = $ids->{$sid}; > next if !$scfg->{path}; > my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); > > - my ($vtype, $volid) = $parse_volid_from_file_path->($plugin, $sid, $scfg); > - return ($vtype, $volid) if $vtype; > + for my $vtype ($vtypes_to_check->@*) { > + my $volid = $parse_volid_from_file_path->($plugin, $sid, $scfg, $vtype); > + > + if (defined($volid)) { > + return ($vtype, $volid); > + } > + } > } > > # can't map path to volume id > -- > 2.47.3 > > > > > --