[pve-devel] [PATCH pve-storage] PVE/Storage/Plugin.pm: new list_volumes plugin method
Thomas Lamprecht
t.lamprecht at proxmox.com
Fri Jul 19 08:48:37 CEST 2019
Subject message should be for human consumption, i.e.:
s!PVE/Storage/Plugin.pm!storage plugin:!
(shorter and easier to read, if one wants to see the paths touched they can
use git's '--stat' option)
On 7/19/19 8:02 AM, Dietmar Maurer wrote:
> This cleanup improve code reuse, and allows plugins to override list_volumes.
Besides the missing APIVER and APIAGE bump (see below) and a few additional,
but small, comments (see also inline): look OK
>
> Note: This changes the template_list return value into an array.
> ---
> PVE/Storage.pm | 147 +++------------------------------
> PVE/Storage/DirPlugin.pm | 84 ++++++++++++++++++-
> PVE/Storage/Plugin.pm | 23 ++++++
> test/run_test_zfspoolplugin.pl | 2 +-
> 4 files changed, 118 insertions(+), 138 deletions(-)
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 5925c69..18ce8b6 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -769,116 +769,6 @@ sub vdisk_free {
> $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
> }
>
> -# lists all files in the snippets directory
> -sub snippets_list {
> - my ($cfg, $storeid) = @_;
> -
> - my $ids = $cfg->{ids};
> -
> - storage_check_enabled($cfg, $storeid) if ($storeid);
> -
> - my $res = {};
> -
> - foreach my $sid (keys %$ids) {
> - next if $storeid && $storeid ne $sid;
> - next if !storage_check_enabled($cfg, $sid, undef, 1);
> -
> - my $scfg = $ids->{$sid};
> - next if !$scfg->{content}->{snippets};
> -
> - activate_storage($cfg, $sid);
> -
> - if ($scfg->{path}) {
> - my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> - my $path = $plugin->get_subdir($scfg, 'snippets');
> -
> - foreach my $fn (<$path/*>) {
> - next if -d $fn;
> -
> - push @{$res->{$sid}}, {
> - volid => "$sid:snippets/". basename($fn),
> - format => 'snippet',
> - size => -s $fn // 0,
> - };
> - }
> - }
> -
> - if ($res->{$sid}) {
> - @{$res->{$sid}} = sort {$a->{volid} cmp $b->{volid} } @{$res->{$sid}};
> - }
> - }
> -
> - return $res;
> -}
> -
> -#list iso or openvz template ($tt = <iso|vztmpl|backup>)
> -sub template_list {
> - my ($cfg, $storeid, $tt) = @_;
> -
> - die "unknown template type '$tt'\n"
> - if !($tt eq 'iso' || $tt eq 'vztmpl' || $tt eq 'backup');
> -
> - my $ids = $cfg->{ids};
> -
> - storage_check_enabled($cfg, $storeid) if ($storeid);
> -
> - my $res = {};
> -
> - # query the storage
> -
> - foreach my $sid (keys %$ids) {
> - next if $storeid && $storeid ne $sid;
> -
> - my $scfg = $ids->{$sid};
> - my $type = $scfg->{type};
> -
> - next if !storage_check_enabled($cfg, $sid, undef, 1);
> -
> - next if $tt eq 'iso' && !$scfg->{content}->{iso};
> - next if $tt eq 'vztmpl' && !$scfg->{content}->{vztmpl};
> - next if $tt eq 'backup' && !$scfg->{content}->{backup};
> -
> - activate_storage($cfg, $sid);
> -
> - if ($scfg->{path}) {
> - my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> -
> - my $path = $plugin->get_subdir($scfg, $tt);
> -
> - foreach my $fn (<$path/*>) {
> -
> - my $info;
> -
> - if ($tt eq 'iso') {
> - next if $fn !~ m!/([^/]+\.[Ii][Ss][Oo])$!;
> -
> - $info = { volid => "$sid:iso/$1", format => 'iso' };
> -
> - } elsif ($tt eq 'vztmpl') {
> - next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
> -
> - $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
> -
> - } elsif ($tt eq 'backup') {
> - next if $fn !~ m!/([^/]+\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo))$!;
> -
> - $info = { volid => "$sid:backup/$1", format => $2 };
> - }
> -
> - $info->{size} = -s $fn // 0;
> -
> - push @{$res->{$sid}}, $info;
> - }
> -
> - }
> -
> - @{$res->{$sid}} = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @{$res->{$sid}} if $res->{$sid};
> - }
> -
> - return $res;
> -}
> -
> -
> sub vdisk_list {
> my ($cfg, $storeid, $vmid, $vollist) = @_;
>
> @@ -923,6 +813,15 @@ sub vdisk_list {
> return $res;
> }
>
> +sub template_list {
> + my ($cfg, $storeid, $tt) = @_;
> +
> + die "unknown template type '$tt'\n"
> + if !($tt eq 'iso' || $tt eq 'vztmpl' || $tt eq 'backup' || $tt eq 'snippets');
> +
> + return volume_list($cfg, $storeid, undef, $tt);
> +}
> +
> sub volume_list {
> my ($cfg, $storeid, $vmid, $content) = @_;
>
> @@ -932,33 +831,11 @@ sub volume_list {
>
> my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>
> - my $res = [];
> - foreach my $ct (@$cts) {
> - my $data;
> - if ($ct eq 'images') {
> - $data = vdisk_list($cfg, $storeid, $vmid);
> - } elsif ($ct eq 'iso' && !defined($vmid)) {
> - $data = template_list($cfg, $storeid, 'iso');
> - } elsif ($ct eq 'vztmpl'&& !defined($vmid)) {
> - $data = template_list ($cfg, $storeid, 'vztmpl');
> - } elsif ($ct eq 'backup') {
> - $data = template_list ($cfg, $storeid, 'backup');
> - foreach my $item (@{$data->{$storeid}}) {
> - if (defined($vmid)) {
> - @{$data->{$storeid}} = grep { $_->{volid} =~ m/\S+-$vmid-\S+/ } @{$data->{$storeid}};
> - }
> - }
> - } elsif ($ct eq 'snippets') {
> - $data = snippets_list($cfg, $storeid);
> - }
> + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>
> - next if !$data || !$data->{$storeid};
> + activate_storage($cfg, $storeid);
>
> - foreach my $item (@{$data->{$storeid}}) {
> - $item->{content} = $ct;
> - push @$res, $item;
> - }
> - }
> + my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts);
>
> return $res;
> }
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index e24ee09..6d5fb6d 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -21,7 +21,7 @@ sub plugindata {
> { images => 1, rootdir => 1 }],
> format => [ { raw => 1, qcow2 => 1, vmdk => 1, subvol => 1 } , 'raw' ],
> };
> -}
> +}
>
> sub properties {
> return {
> @@ -114,7 +114,7 @@ sub activate_storage {
> "directory is expected to be a mount point but is not mounted: '$mp'\n";
> }
>
> - $class->SUPER::activate_storage($storeid, $scfg, $cache);
> + $class->SUPER::activate_storage($storeid, $scfg, $cache);
> }
>
> sub check_config {
> @@ -127,4 +127,84 @@ sub check_config {
> return $opts;
> }
>
> +# list templates ($tt = <iso|vztmpl|backup|snippets>)
> +my $template_list = sub {
> + my ($sid, $path, $tt, $vmid) = @_;
> +
> + my $res = [];
> +
> + foreach my $fn (<$path/*>) {
> +
> + next if -d $fn;
> +
> + my $info;
> +
> + if ($tt eq 'iso') {
> + next if $fn !~ m!/([^/]+\.[Ii][Ss][Oo])$!;
I know you just moving this, but couldn't this use the 'i' flag
for making the check case insensitive?
> +
> + $info = { volid => "$sid:iso/$1", format => 'iso' };
> +
> + } elsif ($tt eq 'vztmpl') {
> + next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
> +
> + $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
> +
> + } elsif ($tt eq 'backup') {
> + next if $fn !~ m!/([^/]+\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo))$!;
> + next if defined($vmid) && $fn !~ m/\S+-$vmid-\S+/;
> +
> + $info = { volid => "$sid:backup/$1", format => $2 };
> +
> + } elsif ($tt eq 'snippets') {
> +
> + $info = {
> + volid => "$sid:snippets/". basename($fn),
> + format => 'snippet',
> + };
> + }
> +
> + $info->{size} = -s $fn // 0;
> +
> + push @$res, $info;
> + }
> +
> + return $res;
> +};
> +
> +sub list_volumes {
> + my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> +
> + use Data::Dumper;
please remove above, debugging left over
> + my $res = [];
> +
> + foreach my $ct (@$content_types) {
> + my $data;
> +
> + my $path = $class->get_subdir($scfg, $ct);
> +
> + if ($ct eq 'images') {
> + $data = $class->list_images($storeid, $scfg, $vmid);
> + } elsif ($ct eq 'iso' && !defined($vmid)) {
> + $data = $template_list->($storeid, $path, 'iso');
> + } elsif ($ct eq 'vztmpl'&& !defined($vmid)) {
> + $data = $template_list->($storeid, $path, 'vztmpl');
> + } elsif ($ct eq 'backup') {
> + $data = $template_list->($storeid, $path, 'backup', $vmid);
> + } elsif ($ct eq 'snippets') {
> + $data = $template_list->($storeid, $path, 'snippets');
> + }
> +
> + next if !$data;
> +
> + foreach my $item (@$data) {
> + $item->{content} = $ct;
> + push @$res, $item;
> + }
> + }> +
> + @$res = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @$res;
> +
> + return $res;
> +}
> +
> 1;
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 842b4d2..e940195 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -902,6 +902,29 @@ sub list_images {
> return $res;
> }
>
> +sub list_volumes {
> + my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
This needs a bump in "APIVER" but as it's no breaking change (has
fallback to old way) you can also bump "APIAGE" so other users of
the storage plugin ABI can know when they can use the new
"list_volumes" but are not marked as broken when they have an older
version themself..
> +
> + my $res = [];
> +
> + foreach my $ct (@$content_types) {
> + my $data;
> +
> + # default implementation just considers images
> + if ($ct eq 'images') {
> + $data = $class->list_images($storeid, $scfg, $vmid);
> + }
> + next if !$data;
> +
> + foreach my $item (@$data) {
> + $item->{content} = $ct;
> + push @$res, $item;
> + }
> + }
> +
> + return $res;
> +}
> +
> sub status {
> my ($class, $storeid, $scfg, $cache) = @_;
>
> diff --git a/test/run_test_zfspoolplugin.pl b/test/run_test_zfspoolplugin.pl
> index 63b1456..2058508 100755
> --- a/test/run_test_zfspoolplugin.pl
> +++ b/test/run_test_zfspoolplugin.pl
> @@ -309,7 +309,7 @@ my $test15 = sub {
>
> print "\nrun test15 \"template_list and vdisk_list\"\n";
>
> - my $hash = Dumper {};
> + my $hash = Dumper [];
>
> my $res = Dumper PVE::Storage::template_list($cfg, $storagename, "vztmpl");
> if ( $hash ne $res ) {
>
More information about the pve-devel
mailing list