[pve-devel] [PATCH storage v2 1/3] Recursive search for iso and vztmpl
Dominic Jäger
d.jaeger at proxmox.com
Tue May 19 11:58:16 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>
---
v2:
Adapt to Alwin's zstd patch
- Adapt tests
- Use file::stat
Dominik's Feedback
- Regex: Fix rule for double dots, no useless ORs, ? instead of *
- No full sub for recursion
- No unfamiliar names like 'notdir'
- Syntax: More parentheses, no $$, no 'not'
Additional
- Syntax: More concise, stricter linewidth, clearer names
- Add volid to the helper function parse_path and use it in both
get_subdir_files and get_subdir_files_recursive.
The comment might be a bit verbose, but having a "glossary" for newcomers
did not seem too bad to me.
PVE/API2/Storage/Content.pm | 9 +++-
PVE/API2/Storage/Status.pm | 13 ++---
PVE/Storage.pm | 12 ++++-
PVE/Storage/Plugin.pm | 102 +++++++++++++++++++++++++++++++----
test/filesystem_path_test.pm | 18 +++++++
test/list_volumes_test.pm | 56 ++++++++++++++++++-
test/parse_volname_test.pm | 42 ++++++++++++++-
7 files changed, 230 insertions(+), 22 deletions(-)
diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index f2e3e57..667be68 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..f3af879 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -403,25 +403,26 @@ __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 =~ $PVE::Storage::forbidden_double_dots_re;
+ $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 f1e3b19..3c3f24c 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -104,6 +104,14 @@ PVE::Storage::Plugin->init();
my $UDEVADM = '/sbin/udevadm';
our $iso_extension_re = qr/\.(?:iso|img)/i;
+{
+ # '..' is forbidden at the beginning, between two '/' and at the end
+ my $dots = quotemeta('..');
+ my $beginning = qr!^$dots/!;
+ my $between = qr!/$dots/!;
+ my $end = qr!/$dots$!;
+ our $forbidden_double_dots_re = qr!(?:$beginning|$between|$end)!;
+}
# PVE::Storage utility functions
@@ -937,7 +945,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);
@@ -951,7 +959,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 cec136e..3e09af9 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -418,6 +418,9 @@ sub parse_name_dir {
die "unable to parse volume filename '$name'\n";
}
+# for iso and vztmpl returns
+# [0] format
+# [1] basename+suffix
sub parse_volname {
my ($class, $volname) = @_;
@@ -431,9 +434,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 '..'\n" if $volname =~ $PVE::Storage::forbidden_double_dots_re;
return ('iso', $1);
- } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
+ } elsif ($volname =~ m!^vztmpl/(?:.+/)?([^/]+\.tar\.[gx]z)$!) {
+ die "volname must not contain two dots '..'\n" if $volname =~ $PVE::Storage::forbidden_double_dots_re;
return ('vztmpl', $1);
} elsif ($volname =~ m!^rootdir/(\d+)$!) {
return ('rootdir', $1, $1);
@@ -492,7 +497,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;
}
@@ -915,6 +927,72 @@ sub list_images {
return $res;
}
+
+# Input:
+# $path ... a filesystem path (e.g. /root/iso/proxmox-ve.iso)
+# $type ... <iso|vztmpl>
+# $storage_path ... filesystem path of the storage (e.g. /root/iso)
+# $storage_id ... identifier of the storage (e.g. local-iso)
+# Returns:
+# If $path fits $type
+# [0] format (e.g. iso)
+# [1] volname (e.g. iso/proxmox-ve.iso)
+# [2] volid (e.g. local-iso:iso/proxmox-ve.iso)
+# undef otherwise
+sub parse_path {
+ my ($path, $type, $storage_path, $storage_id) = @_;
+ die "path must not contain two dots '..'\n"
+ if $path =~ $PVE::Storage::forbidden_double_dots_re;
+
+ if ($type eq 'iso') {
+ if ($path =~ m!$storage_path/(.+$PVE::Storage::iso_extension_re)$!i) {
+ my $volname = "$type/$1";
+ return [$type, $volname, "$storage_id:$volname"];
+ }
+ return undef;
+ } elsif ($type eq 'vztmpl') {
+ if ($path =~ m!$storage_path/(.+\.tar\.([gx]z))$!) {
+ my $volname = "$type/$1";
+ return ["t$2", $volname, "$storage_id:$volname"];
+ }
+ return undef;
+ } else {
+ die "Invalid parameter type=$type";
+ }
+}
+
+# $type = <iso|vztmpl>
+my $get_subdir_files_recursive;
+$get_subdir_files_recursive = sub {
+ my ($sid, $path, $type, $storage_path) = @_;
+ die "Invalid parameter" if !($type eq 'iso' || $type eq 'vztmpl');
+ die "Path must not contain two dots '..'\n"
+ if $path =~ $PVE::Storage::forbidden_double_dots_re;
+ # Each call needs to know the original storage path
+ $storage_path = $path if !defined($storage_path);
+
+ my $st = File::stat::stat($path);
+ die "Could not get status info for file '$path'" if !$st;
+
+ 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 (S_ISDIR($st->mode)) {
+ foreach my $fn (<$path/*>) {
+ push @result, $get_subdir_files_recursive->($sid, $fn, $type, $storage_path);
+ }
+ } else {
+ my $properties = parse_path($path, $type, $storage_path, $sid);
+ if ($properties) { # Ignore other files, e.g. with ending .iso.torrent
+ my $file = { volid => $properties->[2], format => $properties->[0] };
+ $file->{size} = $st->size;
+ $file->{ctime} = $st->ctime;
+ push @result, $file;
+ }
+ }
+ return @result;
+};
+
# list templates ($tt = <iso|vztmpl|backup|snippets>)
my $get_subdir_files = sub {
my ($sid, $path, $tt, $vmid) = @_;
@@ -932,12 +1010,14 @@ my $get_subdir_files = sub {
if ($tt eq 'iso') {
next if $fn !~ m!/([^/]+$PVE::Storage::iso_extension_re)$!i;
- $info = { volid => "$sid:iso/$1", format => 'iso' };
+ my $properties = parse_path($fn, $tt, $path, $sid);
+ $info = { volid => $properties->[2], format => $properties->[0] };
} elsif ($tt eq 'vztmpl') {
next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
- $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
+ my $properties = parse_path($fn, $tt, $path, $sid);
+ $info = { volid => $properties->[2], format => $properties->[0] };
} elsif ($tt eq 'backup') {
next if defined($vmid) && $fn !~ m/\S+-$vmid-\S+/;
@@ -975,7 +1055,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();
@@ -987,10 +1067,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_recursive->($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/filesystem_path_test.pm b/test/filesystem_path_test.pm
index c1b6d90..35a1d4e 100644
--- a/test/filesystem_path_test.pm
+++ b/test/filesystem_path_test.pm
@@ -56,6 +56,24 @@ my $tests = [
'iso'
],
},
+ {
+ volname => 'iso/a/b/c/my-awesome-proxmox.iso',
+ snapname => undef,
+ expected => [
+ "$path/template/iso/a/b/c/my-awesome-proxmox.iso",
+ undef,
+ 'iso'
+ ],
+ },
+ {
+ volname => 'vztmpl/a/b/c/my-awesome-template.tar.gz',
+ snapname => undef,
+ expected => [
+ "$path/template/cache/a/b/c/my-awesome-template.tar.gz",
+ undef,
+ 'vztmpl'
+ ],
+ },
{
volname => "backup/vzdump-qemu-1234-2020_03_30-21_12_40.vma",
snapname => undef,
diff --git a/test/list_volumes_test.pm b/test/list_volumes_test.pm
index 02edc35..cbf7ec3 100644
--- a/test/list_volumes_test.pm
+++ b/test/list_volumes_test.pm
@@ -395,6 +395,55 @@ my @tests = (
},
],
},
+ {
+ description => 'VMID: none, nested iso and vztmpl',
+ vmid => undef,
+ files => [
+ "$storage_dir/template/iso/test.iso",
+ "$storage_dir/template/iso/a/b/c/test.iso",
+ "$storage_dir/template/iso/a/b.b/c/test.iso",
+ "$storage_dir/template/cache/a/b/c/test.tar.gz",
+ "$storage_dir/template/cache/a/b.b/c/test.tar.gz",
+ ],
+ param => {recursive => 1},
+ expected => [
+ {
+ 'content' => 'vztmpl',
+ 'format' => 'tgz',
+ 'size' => DEFAULT_SIZE,
+ 'ctime' => DEFAULT_CTIME,
+ 'volid' => 'local:vztmpl/a/b/c/test.tar.gz',
+ },
+ {
+ 'content' => 'vztmpl',
+ 'format' => 'tgz',
+ 'size' => DEFAULT_SIZE,
+ 'ctime' => DEFAULT_CTIME,
+ 'volid' => 'local:vztmpl/a/b.b/c/test.tar.gz',
+ },
+ {
+ 'content' => 'iso',
+ 'format' => 'iso',
+ 'size' => DEFAULT_SIZE,
+ 'ctime' => DEFAULT_CTIME,
+ 'volid' => 'local:iso/a/b/c/test.iso',
+ },
+ {
+ 'content' => 'iso',
+ 'format' => 'iso',
+ 'size' => DEFAULT_SIZE,
+ 'ctime' => DEFAULT_CTIME,
+ 'volid' => 'local:iso/a/b.b/c/test.iso',
+ },
+ {
+ 'content' => 'iso',
+ 'format' => 'iso',
+ 'size' => DEFAULT_SIZE,
+ 'ctime' => DEFAULT_CTIME,
+ 'volid' => 'local:iso/test.iso',
+ },
+ ],
+ },
{
description => 'VMID: none, parent, non-matching',
# string instead of vmid in folder
@@ -427,8 +476,10 @@ my @tests = (
"$storage_dir/images/ssss/base-4321-disk-0.raw",
"$storage_dir/images/ssss/vm-1234-disk-0.qcow2",
"$storage_dir/template/iso/yet-again-a-installation-disk.dvd",
+ "$storage_dir/template/iso/../../double_points_are_forbidden.iso",
"$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz",
"$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2",
+ "$storage_dir/template/cache/../../double_points_are_forbidden.tar.gz",
"$storage_dir/private/subvol-19254-disk-0/19254",
"$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
"$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",
@@ -501,6 +552,7 @@ plan tests => $plan + 1;
my $expected = $tt->{expected};
my $description = $tt->{description};
my $parent = $tt->{parent};
+ my $param = $tt->{param};
# prepare environment
my $num = 0; #parent disks
@@ -521,10 +573,10 @@ plan tests => $plan + 1;
}
my $got;
- eval { $got = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types) };
+ eval { $got = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types, $param) };
$got = $@ if $@;
- is_deeply($got, $expected, $description) || diag(explain($got));
+ is_deeply($got, $expected, $description) || diag("Got\n", explain($got), "but expected\n", explain($expected));
# clean up after each test case, otherwise
# we get wrong results from leftover files
diff --git a/test/parse_volname_test.pm b/test/parse_volname_test.pm
index d6ac885..d6fb9ea 100644
--- a/test/parse_volname_test.pm
+++ b/test/parse_volname_test.pm
@@ -31,11 +31,26 @@ my $tests = [
volname => 'iso/some-installation-disk.iso',
expected => ['iso', 'some-installation-disk.iso'],
},
+ {
+ description => 'ISO image, iso, nested',
+ volname => 'iso/a/b/c/some-installation-disk.iso',
+ expected => ['iso', 'some-installation-disk.iso'],
+ },
{
description => 'ISO image, img',
volname => 'iso/some-other-installation-disk.img',
expected => ['iso', 'some-other-installation-disk.img'],
},
+ {
+ description => 'ISO image, img, nested',
+ volname => 'iso/a/b/c/some-other-installation-disk.img',
+ expected => ['iso', 'some-other-installation-disk.img'],
+ },
+ {
+ description => 'ISO image, img, nested with dots',
+ volname => 'iso/a/b.b/c/some-other-installation-disk.img',
+ expected => ['iso', 'some-other-installation-disk.img'],
+ },
#
# container templates
#
@@ -44,11 +59,21 @@ my $tests = [
volname => 'vztmpl/debian-10.0-standard_10.0-1_amd64.tar.gz',
expected => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.gz'],
},
+ {
+ description => 'Container template tar.gz nested',
+ volname => 'vztmpl/a/b/c/debian-10.0-standard_10.0-1_amd64.tar.gz',
+ expected => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.gz'],
+ },
{
description => 'Container template tar.xz',
volname => 'vztmpl/debian-10.0-standard_10.0-1_amd64.tar.xz',
expected => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.xz'],
},
+ {
+ description => 'Container template tar.xz nested',
+ volname => 'vztmpl/a/b/c/debian-10.0-standard_10.0-1_amd64.tar.xz',
+ expected => ['vztmpl', 'debian-10.0-standard_10.0-1_amd64.tar.xz'],
+ },
#
# container rootdir
#
@@ -93,6 +118,21 @@ my $tests = [
volname => 'iso/yet-again-a-installation-disk.dvd',
expected => "unable to parse directory volume name 'iso/yet-again-a-installation-disk.dvd'\n",
},
+ {
+ description => 'Failed match: ISO image, forbidden double dots between two /',
+ volname => 'iso/a/../c/some-installation-disk.iso',
+ expected => "volname must not contain two dots '..'\n",
+ },
+ {
+ description => 'Failed match: ISO image, forbidden double dots in the beginning',
+ volname => '../iso/some-installation-disk.iso',
+ expected => "unable to parse directory volume name '../iso/some-installation-disk.iso'\n",
+ },
+ {
+ description => 'Failed match: ISO image, forbidden double dots in the end',
+ volname => 'iso/some-installation-disk.iso..',
+ expected => "unable to parse directory volume name 'iso/some-installation-disk.iso..'\n",
+ },
{
description => 'Failed match: Container template, zip.gz',
volname => 'vztmpl/debian-10.0-standard_10.0-1_amd64.zip.gz',
@@ -239,7 +279,7 @@ foreach my $t (@$tests) {
eval { $got = [ PVE::Storage::Plugin->parse_volname($volname) ] };
$got = $@ if $@;
- is_deeply($got, $expected, $description);
+ is_deeply($got, $expected, $description) || diag("Got\n", explain($got), "but expected\n", explain($expected));
$seen_vtype->{@$expected[0]} = 1 if ref $expected eq 'ARRAY';
}
--
2.20.1
More information about the pve-devel
mailing list