[PATCH pve-storage v2 29/50] tree-wide: simplify recently refactored parsing logic

["Max R. Carrara" <m.carrara@proxmox.com>]

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 <m.carrara@proxmox.com>
---
 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