[pve-devel] [PATCH storage 2/4] Recursive search for iso and vztmpl

Dominic Jäger d.jaeger at proxmox.com
Thu Apr 2 13:34:12 CEST 2020


Each content type has a predefined location in a storage where they can be
found. iso and vztmpl files can now be found and and also uploaded in
subdirectories of those locations, too.

Add a parameter "recursive" (default off) to the API at
storage/{storage}/content, to perform this search.

Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
---
Changes since RFC:
* Separate tests for existing and new stuff into 2 patches
* Die when .. appears in a path
* Make feature opt-in (relevant for symlinks)

 PVE/API2/Storage/Content.pm |  9 +++-
 PVE/API2/Storage/Status.pm  | 12 +++---
 PVE/Storage.pm              |  4 +-
 PVE/Storage/Plugin.pm       | 85 +++++++++++++++++++++++++++++++++----
 test/run_plugin_tests.pl    | 57 ++++++++++++++++++++++++-
 5 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index 63fa4fc..cd302e9 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -44,6 +44,12 @@ __PACKAGE__->register_method ({
 		optional => 1,
 		completion => \&PVE::Cluster::complete_vmid,
 	    }),
+	    recursive => {
+	        description => "Search recursively.",
+	        type => 'boolean',
+	        optional => 1,
+	        default => 0,
+	    },
 	},
     },
     returns => {
@@ -102,7 +108,8 @@ __PACKAGE__->register_method ({
 
 	my $cfg = PVE::Storage::config();
 
-	my $vollist = PVE::Storage::volume_list($cfg, $storeid, $param->{vmid}, $param->{content});
+	my $vollist = PVE::Storage::volume_list($cfg, $storeid, $param->{vmid},
+	    $param->{content}, {recursive => $param->{recursive}});
 
 	my $res = [];
 	foreach my $item (@$vollist) {
diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index 14f5930..b8caf9c 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -403,25 +403,25 @@ __PACKAGE__->register_method ({
 	my $filename = $param->{filename};
 
 	chomp $filename;
-	$filename =~ s/^.*[\/\\]//;
-	$filename =~ s/[^-a-zA-Z0-9_.]/_/g;
+	# Prevent entering other locations without permission
+	die "Filename must not contain two dots '..'" if $filename =~ m/\.\./;
+	$filename =~ s/^[\/|\\]*//; # files are uploaded relative to storage path
+	$filename =~ s/[^-a-zA-Z0-9_.\/\\]/_/g;
 
 	my $path;
-
 	if ($content eq 'iso') {
-	    if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
+	    if ($filename !~ m!$PVE::Storage::iso_extension_re$!) {
 		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
 	    }
 	    $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
 	} elsif ($content eq 'vztmpl') {
-	    if ($filename !~ m![^/]+\.tar\.[gx]z$!) {
+	    if ($filename !~ m!\.tar\.[gx]z$!) {
 		raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
 	    }
 	    $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
 	} else {
 	    raise_param_exc({ content => "upload content type '$content' not allowed" });
 	}
-
 	die "storage '$param->{storage}' does not support '$content' content\n"
 	    if !$scfg->{content}->{$content};
 
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 60b8310..9ec40a5 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -866,7 +866,7 @@ sub template_list {
 }
 
 sub volume_list {
-    my ($cfg, $storeid, $vmid, $content) = @_;
+    my ($cfg, $storeid, $vmid, $content, $param) = @_;
 
     my @ctypes = qw(rootdir images vztmpl iso backup snippets);
 
@@ -880,7 +880,7 @@ sub volume_list {
 
     activate_storage($cfg, $storeid);
 
-    my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts);
+    my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts, $param);
 
     @$res = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @$res;
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 8c0dae1..e81c89f 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -415,6 +415,9 @@ sub parse_name_dir {
     die "unable to parse volume filename '$name'\n";
 }
 
+# for iso and vztmpl returns
+#   [0] format
+#   [1] notdir (=basename+suffix; no directories)
 sub parse_volname {
     my ($class, $volname) = @_;
 
@@ -428,9 +431,11 @@ sub parse_volname {
 	my ($vmid, $name) = ($1, $2);
 	my (undef, $format, $isBase) = parse_name_dir($name);
 	return ('images', $name, $vmid, undef, undef, $isBase, $format);
-    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
+    } elsif ($volname =~ m!^iso/(?:.+/)*([^/]+$PVE::Storage::iso_extension_re)$!) {
+	die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;
 	return ('iso', $1);
-    } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
+    } elsif ($volname =~ m!^vztmpl/(?:.+/)*([^/]+\.tar\.[gx]z)$!) {
+	die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;
 	return ('vztmpl', $1);
     } elsif ($volname =~ m!^rootdir/(\d+)$!) {
 	return ('rootdir', $1, $1);
@@ -485,7 +490,14 @@ sub filesystem_path {
 
     $dir .= "/$vmid" if $vtype eq 'images';
 
-    my $path = "$dir/$name";
+    my $path;
+    if ($vtype eq 'iso' || $vtype eq 'vztmpl') {
+	# remove vtype => file name relative to storage path
+	$volname =~ m!^$vtype/(.+)!;
+	$path = "$dir/$1";
+    } else {
+	$path = "$dir/$name";
+    }
 
     return wantarray ? ($path, $vmid, $vtype) : $path;
 }
@@ -910,6 +922,61 @@ sub list_images {
     return $res;
 }
 
+
+# returns if file name fits $type
+#   [0] volname
+#   [1] format
+# undef otherwise
+sub get_file_properties {
+    my ($fn, $storage_path, $type) = @_;
+    die "fn must not contain two dots '..'" if $fn =~ m/\.\./;
+    if ($type eq 'iso') {
+	if ($fn =~ m!$storage_path/(.+$PVE::Storage::iso_extension_re)$!i) {
+	    return ["$type/$1", $type];
+	} else {
+	    return undef;
+	}
+    } elsif ($type eq 'vztmpl') {
+	if ($fn =~ m!$storage_path/(.+\.tar\.([gx]z))$!) {
+	    return ["$type/$1", "t$2"];
+	} else {
+	    return undef;
+	}
+    } else {
+	die "Invalid parameter type=$type";
+    }
+}
+
+# $type = <iso|vztmpl>
+sub get_subdir_files_rec {
+    my ($sid, $path, $type, $storage_path) = @_;
+    die "Invalid parameter" if not ($type eq 'iso' || $type eq 'vztmpl');
+    die "Path must not contain ../" if $path =~ m!.*\.\./.*!;
+
+    # Each call needs to know the original storage path
+    if (!defined $storage_path) {
+	$storage_path = $path;
+    }
+
+    my @result = ();
+
+    # Symlinks have to be set up via shell. At least as long recursive scanning
+    # is opt-in handling loops is out of scope. Instead, just follow the links.
+    if (-d $path) {
+	foreach my $fn (<$path/*>) {
+	    push @result, get_subdir_files_rec($sid, $fn, $type, $storage_path);
+	}
+    } else {
+	my $properties = get_file_properties($path, $storage_path, $type);
+	if ($properties) {
+	    my $file = { volid => "$sid:$$properties[0]", format => $$properties[1] };
+	    $file->{size} = -s $path // 0;
+	    push @result, $file;
+	}
+    }
+    return @result;
+}
+
 # list templates ($tt = <iso|vztmpl|backup|snippets>)
 my $get_subdir_files = sub {
     my ($sid, $path, $tt, $vmid) = @_;
@@ -982,7 +1049,7 @@ my $get_subdir_files = sub {
 };
 
 sub list_volumes {
-    my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
+    my ($class, $storeid, $scfg, $vmid, $content_types, $param) = @_;
 
     my $res = [];
     my $vmlist = PVE::Cluster::get_vmlist();
@@ -994,10 +1061,12 @@ sub list_volumes {
 	} elsif ($scfg->{path}) {
 	    my $path = $class->get_subdir($scfg, $type);
 
-	    if ($type eq 'iso' && !defined($vmid)) {
-		$data = $get_subdir_files->($storeid, $path, 'iso');
-	    } elsif ($type eq 'vztmpl'&& !defined($vmid)) {
-		$data = $get_subdir_files->($storeid, $path, 'vztmpl');
+	    if (($type eq 'iso' || $type eq 'vztmpl') && !defined($vmid)) {
+		if ($param->{recursive}) {
+		    $data = [get_subdir_files_rec($storeid, $path, $type)];
+		} else {
+		    $data = $get_subdir_files->($storeid, $path, $type);
+		}
 	    } elsif ($type eq 'backup') {
 		$data = $get_subdir_files->($storeid, $path, 'backup', $vmid);
 	    } elsif ($type eq 'snippets') {
diff --git a/test/run_plugin_tests.pl b/test/run_plugin_tests.pl
index cd93430..7f627b1 100755
--- a/test/run_plugin_tests.pl
+++ b/test/run_plugin_tests.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 
 use lib ('.', '..');
-use Test::More tests => 32;
+use Test::More tests => 42;
 use Test::MockModule qw(new);
 use File::Temp qw(tempdir);
 use File::Path qw(make_path);
@@ -13,16 +13,23 @@ use PVE::Storage;
 
 my $plugin = 'PVE::Storage::Plugin';
 my $basename = 'test';
+my $subdir_regular = 'some/more/subdirectories/';
 
 my $iso_type = 'iso';
 my $iso_suffix = '.iso';
 my $iso_notdir = "$basename$iso_suffix";
 my $iso_volname = "$iso_type/$iso_notdir";
+my $iso_volname_with_subdirs = "$iso_type/$subdir_regular$iso_notdir";
+# Subdirectories should be treated like normal folders, even if their name
+# is equal to our predefined storage schema
+my $subdirs_named_iso = 'a/template/iso/b/template/iso/c/template/iso/';
 
 my $vztmpl_type = 'vztmpl';
 my $vztmpl_suffix = '.tar.gz';
 my $vztmpl_notdir = "$basename$vztmpl_suffix";
 my $vztmpl_volname = "$vztmpl_type/$vztmpl_notdir";
+my $vztmpl_volname_with_subdirs = "$vztmpl_type/$subdir_regular$vztmpl_notdir";
+my $subdir_named_cache = 'a/template/cache/b/template/cache/c/template/cache/';
 
 my $iso_with_dots = "$iso_type/../$iso_notdir";
 my $vztmpl_with_dots = "$vztmpl_type/../$vztmpl_notdir";
@@ -48,11 +55,25 @@ is (($plugin->parse_volname($iso_volname))[$type_index],
     $iso_type, 'parse_volname: type for iso');
 is (($plugin->parse_volname($iso_volname))[$notdir_index],
     $iso_notdir, 'parse_volname: notdir for iso');
+is (($plugin->parse_volname($iso_volname_with_subdirs))[$type_index],
+    $iso_type, 'parse_volname: type for iso in subdir');
+is (($plugin->parse_volname($iso_volname_with_subdirs))[$notdir_index],
+    $iso_notdir, 'parse_volname: notdir for iso in subdir');
+eval { $plugin->parse_volname($iso_with_dots) };
+like ($@, qr/volname must not contain two dots/,
+    'parse_volname: forbid two dots .. for iso');
 
 is (($plugin->parse_volname($vztmpl_volname))[$type_index],
     $vztmpl_type, 'parse_volname: type for vztmpl');
 is (($plugin->parse_volname($vztmpl_volname))[$notdir_index],
     $vztmpl_notdir, 'parse_volname: notdir for vztmpl');
+is (($plugin->parse_volname($vztmpl_volname_with_subdirs))[$type_index],
+    $vztmpl_type, 'parse_volname: type for vztmpl in subdir');
+is (($plugin->parse_volname($vztmpl_volname_with_subdirs))[$notdir_index],
+    $vztmpl_notdir, 'parse_volname: notdir for vztmpl in subdir');
+eval { $plugin->parse_volname($vztmpl_with_dots) };
+like ($@, qr/volname must not contain two dots/,
+    'parse_volname: forbid two dots .. for vztmpl');
 
 is (($plugin->parse_volname($raw_image_volname))[$type_index],
     $image_type, 'parse_volname: type for raw image');
@@ -91,9 +112,15 @@ is ($plugin->get_subdir($scfg_with_path, 'rootdir'),
 is ($plugin->filesystem_path($scfg_with_path, $iso_volname),
     "$scfg_with_path->{path}/template/$iso_volname",
     'filesystem_path for iso');
+is ($plugin->filesystem_path($scfg_with_path, $iso_volname_with_subdirs),
+    "$scfg_with_path->{path}/template/$iso_volname_with_subdirs",
+    'filesystem_path for iso in subdirs');
 is ($plugin->filesystem_path($scfg_with_path, $vztmpl_volname),
     "$scfg_with_path->{path}/template/cache/$vztmpl_notdir",
     'filesystem_path for vztmpl');
+is ($plugin->filesystem_path($scfg_with_path, $vztmpl_volname_with_subdirs),
+    "$scfg_with_path->{path}/template/cache/$subdir_regular$vztmpl_notdir",
+    'filesystem_path for vztmpl in subdirs');
 is ($plugin->filesystem_path($scfg_with_path, $raw_image_volname),
     "$scfg_with_path->{path}/images/$raw_image_volname",
     'filesystem_path for raw images');
@@ -142,12 +169,40 @@ my $paths = [
 ];
 add_test_files($paths);
 
+my $storage_dir_rec = File::Temp->newdir();
+my $recursive_paths = [
+    "$storage_dir_rec/template/cache",
+    "$storage_dir_rec/template/iso",
+    "$storage_dir_rec/images/$vmid",
+    "$storage_dir_rec/template/cache/$subdir_regular",
+    "$storage_dir_rec/template/cache/$subdirs_named_iso",
+    "$storage_dir_rec/template/cache/$subdir_named_cache",
+    "$storage_dir_rec/template/iso/$subdir_regular",
+    "$storage_dir_rec/template/iso/$subdirs_named_iso",
+    "$storage_dir_rec/template/iso/$subdir_named_cache",
+];
+add_test_files($recursive_paths);
+
 note 'Tests without recursion';
 # note "Temp dir is:\n", `tree $storage_dir_no_rec`;
 my $scfg_no_rec = { path => $storage_dir_no_rec };
 test_list_volumes($scfg_no_rec, [$iso_type], [$iso_volname]);
 test_list_volumes($scfg_no_rec, [$vztmpl_type], [$vztmpl_volname]);
 
+note 'Tests with recursion';
+# note "Temp dir is:\n", `tree $storage_dir_rec`;
+my $scfg_rec = { path => $storage_dir_rec };
+my $expected_volnames_iso = [$iso_volname, $iso_volname_with_subdirs,
+   "$iso_type/$subdir_named_cache$iso_notdir",
+   "$iso_type/$subdirs_named_iso$iso_notdir"];
+my $expected_volnames_vztmpl = [$vztmpl_volname, $vztmpl_volname_with_subdirs,
+   "$vztmpl_type/$subdir_named_cache$vztmpl_notdir",
+   "$vztmpl_type/$subdirs_named_iso$vztmpl_notdir"];
+test_list_volumes($scfg_rec, [$iso_type], $expected_volnames_iso,
+	{ recursive => 1 });
+test_list_volumes($scfg_rec, [$vztmpl_type],$expected_volnames_vztmpl,
+	{ recursive => 1 });
+
 my $scfg_with_type = { path => $storage_dir_no_rec, type => 'dir' };
 my $plugin_mock = Test::MockModule->new('PVE::Cluster');
 $plugin_mock->redefine('get_vmlist' => sub { return undef });
-- 
2.20.1




More information about the pve-devel mailing list