comments inline
On 4/2/20 1:34 PM, Dominic Jäger wrote:
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.jae...@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/\.\./;
i'd argue that this check is wrong...
i get what you intend but what about files named like:
my.very.cool...iso.iso
would also be filtered out but is a very valid filename
'..' is only forbidden at the beginning, the end or between two '/'
so e.g.
m!(?:^\.\./|/\.\./|/\.\.$)!
(well, this is certainly not a readable regex^^; also i am
certainly missing a case, i think...)
+ $filename =~ s/^[\/|\\]*//; # files are uploaded relative to storage
path
are '|' intentionally removed here? in [] the characters are or'd anyway
+ $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)
nit: again the 'notdir' here...
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)$!) {
nit:
(?:.+/)* could also be (?:.+/)?
since this will not repeat anyway,
since the .+ is greedy and will include '/',
the ? makes it clearer that this is just optional
+ die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;
see above
return ('iso', $1);
- } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
+ } elsif ($volname =~ m!^vztmpl/(?:.+/)*([^/]+\.tar\.[gx]z)$!) {
same note about */?
+ die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;
see above
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/\.\./;
see above
+ 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 {
any special reason to make this a full-fledged sub
instead of a private my $foo = sub {}; ?
if it is about recursive access just do:
my $foo;
$foo = sub {
$foo->();
};
+ my ($sid, $path, $type, $storage_path) = @_;
+ die "Invalid parameter" if not ($type eq 'iso' || $type eq 'vztmpl');
any reason for the 'not' vs '!' ? (not that it's wrong, just not our style)
+ die "Path must not contain ../" if $path =~ m!.*\.\./.*!;
ha, its better here, but the .* front and back are useless and its also
wrong: /myweirddirectory../
+
+ # Each call needs to know the original storage path
+ if (!defined $storage_path) {
nit: please use parenthesis for defined
+ $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] };
please use $foo->[0]
see our style guideline:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices
+ $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 });
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
