From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 6C7E31FF0AB for ; Wed, 23 Sep 2026 17:11:00 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 65F4F2172F; Wed, 23 Sep 2026 17:10:00 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v2 29/50] tree-wide: simplify recently refactored parsing logic Date: Wed, 23 Sep 2026 17:05:43 +0200 Message-ID: <20260923150606.531239-30-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260923150606.531239-1-m.carrara@proxmox.com> References: <20260923150606.531239-1-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790176027520 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.411 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_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: JUMRBOGLI4VBUFCE3MSAFH5N5FXU3XU2 X-Message-ID-Hash: JUMRBOGLI4VBUFCE3MSAFH5N5FXU3XU2 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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: PVE::API2::Storage::Status: In the parsing helper for file transfers, it is not necessary to handle the parsing of each volume type individually anymore, since we use `assert_can_transfer_files()` beforehand and also raise an exception if parsing fails in general. Therefore, pull out the common parts of the branches for individual volume types. Retain only the special case for the `import `type where we do not accept `.ovf` files. PVE::Storage: Merge the branches in `path_to_volume_id()` for all volume types, except `images`, since they all do the same thing now. Strip the volume type subdirectory from the given file path only for the `images` volume type. PVE::Storage::Plugin: Condense all of the if-clauses as much as possible in the `parse_volname()` and `get_subdir_files()` subs. In `parse_volname()`, extend the error message of the final `die` at the end of the method, stating that the volume type is unhandled. The guard clause after the `parse_volname_as_parts()` call already `die`s on parsing failure, so that final `die` should never be hit unless we introduce a new volume type and happen to forget to handle it in this method. In `get_subdir_files(), do not strip the volume type subdirectory from the beginning of the current path anymore, because that is handled by the `parse_path_as_volid_parts()` parsing function, which left `$filename` unused. Signed-off-by: Max R. Carrara --- src/PVE/API2/Storage/Status.pm | 40 ++++-------------- src/PVE/Storage.pm | 40 +++++------------- src/PVE/Storage/Plugin.pm | 76 ++++++++++++---------------------- 3 files changed, 43 insertions(+), 113 deletions(-) diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm index d2451898..2fd23555 100644 --- a/src/PVE/API2/Storage/Status.pm +++ b/src/PVE/API2/Storage/Status.pm @@ -75,45 +75,19 @@ my sub assert_can_transfer_files : prototype($$$) { my sub parse_transferred_file_path_extension : prototype($$) { my ($path, $vtype) = @_; - if ($vtype eq 'iso') { - my $parts = parse_rel_path_as_volname_parts($path, $vtype); + my $parts = parse_rel_path_as_volname_parts($path, $vtype); - if (!defined($parts)) { - raise_param_exc({ filename => "wrong file extension" }); - } - - my $ext = $parts->{ext}; - return $ext; + if (!defined($parts)) { + raise_param_exc({ filename => "invalid filename or wrong file extension" }); } - if ($vtype eq 'vztmpl') { - my $parts = parse_rel_path_as_volname_parts($path, $vtype); + my $ext = $parts->{ext}; - if (!defined($parts)) { - raise_param_exc({ filename => "wrong file extension" }); - } - - my $ext = $parts->{ext}; - return $ext; + if ($vtype eq 'import' && $ext eq 'ovf') { + raise_param_exc({ filename => "wrong file extension" }); } - if ($vtype eq 'import') { - my $parts = parse_rel_path_as_volname_parts($path, $vtype); - - if (!defined($parts)) { - raise_param_exc({ filename => "invalid filename or wrong extension" }); - } - - my $ext = $parts->{ext}; - - if ($ext eq 'ovf') { - raise_param_exc({ filename => "wrong file extension" }); - } - - return $ext; - } - - die "upload / download: failed to parse '$path' - unhandled content type '$vtype'\n"; + return $ext; } my sub assert_file_transfer_contents_valid : prototype($$$) { diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm index 98d5f64e..f48b47eb 100755 --- a/src/PVE/Storage.pm +++ b/src/PVE/Storage.pm @@ -741,16 +741,16 @@ sub path_to_volume_id { my $parse_volid_from_file_path = sub { my ($plugin, $sid, $scfg, $vtype) = @_; - my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype); - - # 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(?=/)!!) { - return; - } - if ($vtype eq 'images') { + my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype); + + # 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(?=/)!!) { + return; + } + return if $filename !~ m!/(\d+)/([^/\s]+)$!; my $vmid = $1; my $name = $2; @@ -767,27 +767,7 @@ sub path_to_volume_id { return; } - if ($vtype eq 'iso') { - return parse_abs_path_as_volid($sid, $scfg, $path, $vtype); - } - - if ($vtype eq 'vztmpl') { - return parse_abs_path_as_volid($sid, $scfg, $path, $vtype); - } - - if ($vtype eq 'backup') { - return parse_abs_path_as_volid($sid, $scfg, $path, $vtype); - } - - if ($vtype eq 'snippets') { - return parse_abs_path_as_volid($sid, $scfg, $path, $vtype); - } - - if ($vtype eq 'import') { - return parse_abs_path_as_volid($sid, $scfg, $path, $vtype); - } - - return; + return parse_abs_path_as_volid($sid, $scfg, $path, $vtype); }; my $vtypes_to_check = $get_vtypes_to_check->(); diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm index 45c0b838..def9e495 100644 --- a/src/PVE/Storage/Plugin.pm +++ b/src/PVE/Storage/Plugin.pm @@ -22,6 +22,7 @@ use PVE::Storage::Common qw( use PVE::Storage::Common::Parse qw( parse_volname_as_parts parse_abs_path_as_volid_parts + parse_volid_as_parts ); use JSON; @@ -434,8 +435,8 @@ PVE::JSONSchema::register_format('pve-volume-id', \&parse_volume_id); sub parse_volume_id { my ($volid, $noerr) = @_; - if ($volid =~ m/^([a-z][a-z0-9\-\_\.]*[a-z0-9]):(.+)$/i) { - return wantarray ? ($1, $2) : $1; + if (defined(my $parts = parse_volid_as_parts($volid))) { + return wantarray ? ($parts->{storeid}, $parts->{volname}) : $parts->{storeid}; } return undef if $noerr; die "unable to parse volume ID '$volid'\n"; @@ -827,37 +828,31 @@ sub parse_volname { return ('images', $name, $vmid, undef, undef, $isBase, $format); } - if (defined(my $parts = parse_volname_as_parts($volname))) { - my ($vtype, $volume_path) = $parts->@{qw(vtype path)}; + my $parts = parse_volname_as_parts($volname); + die "unable to parse directory volume name '$volname'\n" + if !defined($parts); - if ($vtype eq 'iso') { - return ($vtype, $volume_path, undef, undef, undef, undef, 'raw'); - } + my ($vtype, $volume_path) = $parts->@{qw(vtype path)}; - if ($vtype eq 'vztmpl') { - return ($vtype, $volume_path, undef, undef, undef, undef, 'raw'); - } - - if ($vtype eq 'backup') { - return ($vtype, $volume_path, $parts->{vmid}, undef, undef, undef, 'raw'); - } - - if ($vtype eq 'snippets') { - return ($vtype, $volume_path, undef, undef, undef, undef, 'raw'); - } - - if ($vtype eq 'import') { - my $format = $parts->{ext}; - - if (defined(my $content_ext = $parts->{'content-ext'})) { - $format .= "+$content_ext"; - } - - return ($vtype, $volume_path, undef, undef, undef, undef, $format); - } + if ($vtype eq 'iso' || $vtype eq 'vztmpl' || $vtype eq 'snippets') { + return ($vtype, $volume_path, undef, undef, undef, undef, 'raw'); } - die "unable to parse directory volume name '$volname'\n"; + if ($vtype eq 'backup') { + return ($vtype, $volume_path, $parts->{vmid}, undef, undef, undef, 'raw'); + } + + if ($vtype eq 'import') { + my $format = $parts->{ext}; + + if (defined(my $content_ext = $parts->{'content-ext'})) { + $format .= "+$content_ext"; + } + + return ($vtype, $volume_path, undef, undef, undef, undef, $format); + } + + die "unable to parse directory volume name '$volname' - unhandled volume type '$vtype'\n"; } # FIXME: remove on the next APIAGE reset. @@ -1710,17 +1705,10 @@ my sub get_subdir_files { my $get_subdir_file_info = sub { my ($path, $st) = @_; - # 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(?=/)!!) { - return; - } + my $parts = parse_abs_path_as_volid_parts($storeid, $scfg, $path, $vtype); + return if !defined($parts); if ($vtype eq 'iso') { - my $parts = parse_abs_path_as_volid_parts($storeid, $scfg, $path, $vtype); - return if !defined($parts); - return { volid => $parts->{volid}, format => 'iso', # always 'iso' even if we have a file ending in .img @@ -1728,9 +1716,6 @@ my sub get_subdir_files { } if ($vtype eq 'vztmpl') { - my $parts = parse_abs_path_as_volid_parts($storeid, $scfg, $path, $vtype); - return if !defined($parts); - my ($ext, $ext_compression) = $parts->@{qw(ext ext-compression)}; my $format = $ext eq 'tar' ? $ext : ('t' . $ext_compression); @@ -1741,9 +1726,6 @@ my sub get_subdir_files { } if ($vtype eq 'backup') { - my $parts = parse_abs_path_as_volid_parts($storeid, $scfg, $path, $vtype); - return if !defined($parts); - my $format = $parts->{ext}; my $volume_path = $parts->{path}; @@ -1785,9 +1767,6 @@ my sub get_subdir_files { } if ($vtype eq 'snippets') { - my $parts = parse_abs_path_as_volid_parts($storeid, $scfg, $path, $vtype); - return if !defined($parts); - return { volid => $parts->{volid}, format => 'snippet', @@ -1795,9 +1774,6 @@ my sub get_subdir_files { } if ($vtype eq 'import') { - my $parts = parse_abs_path_as_volid_parts($storeid, $scfg, $path, $vtype); - return if !defined($parts); - return { volid => $parts->{volid}, format => $parts->{ext}, -- 2.47.3