On Fri, Jul 19, 2019 at 10:40:52AM +0200, Dietmar Maurer wrote: > This cleanup improve code reuse, and allows plugins to override list_volumes. > > Note: This changes the template_list return value into an array. > --- > > Changes in v2: > > - remove debug statements > - move implementaion to Plugin.pm for max. compatibility with old code > - cleanup regex (use i flag) > - bump APIVER an d APIAGE > - sort result inside volume_list (for all plugins) > - only list supported/enabled content > > We need to adopt the template_list call in > - pveam,
pveam uses volume_list only? > - PVE/QemuServer.pm 7316f > - PVE/LXC.pm 1832f see below > > > PVE/Storage.pm | 155 ++++----------------------------- > PVE/Storage/DirPlugin.pm | 4 +- > PVE/Storage/Plugin.pm | 77 ++++++++++++++++ > test/run_test_zfspoolplugin.pl | 2 +- > 4 files changed, 98 insertions(+), 140 deletions(-) > > diff --git a/PVE/Storage.pm b/PVE/Storage.pm > index 5925c69..c438374 100755 > --- a/PVE/Storage.pm > +++ b/PVE/Storage.pm > @@ -36,11 +36,11 @@ use PVE::Storage::ZFSPlugin; > use PVE::Storage::DRBDPlugin; > > # Storage API version. Icrement it on changes in storage API interface. > -use constant APIVER => 2; > +use constant APIVER => 3; > # Age is the number of versions we're backward compatible with. > # This is like having 'current=APIVER' and age='APIAGE' in libtool, > # see > https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html > -use constant APIAGE => 1; > +use constant APIAGE => 2; > > # load standard plugins > PVE::Storage::DirPlugin->register(); > @@ -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); > +} sorry for not catching this in v1 already, but this previously allowed listing templates from all storages if $storeid was undefined, now it requires $storeid. since this is only used for completion, where this behaviour is actually nice for pct create 12345 <TAB> completing all available templates on all storages, why not keep the old output format, and add the usual loop like so: ----8<---- die "unknown template type '$tt'\n" if !($tt eq 'iso' || $tt eq 'vztmpl' || $tt eq 'backup' || $tt eq 'snippets'); 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); $res->{$storeid} = volume_list($cfg, $storeid, undef, $ tt); } return $res; ---->8---- this would keep the interface for the completion helpers identical, which also keeps the changes limited to a single repo. > + > sub volume_list { > my ($cfg, $storeid, $vmid, $content) = @_; > > @@ -932,33 +831,15 @@ 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); > - } > + $cts = [ grep { defined($scfg->{content}->{$_}) } @$cts ]; > + > + 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); > + > + @$res = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @$res; > > return $res; > } > diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm > index e24ee09..39760a8 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 { > diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm > index 842b4d2..eb2d86a 100644 > --- a/PVE/Storage/Plugin.pm > +++ b/PVE/Storage/Plugin.pm > @@ -902,6 +902,83 @@ sub list_images { > return $res; > } > > +# list templates ($tt = <iso|vztmpl|backup|snippets>) > +my $template_list = sub { if you send a v3 with the above change, we could also give this a better name. what it actually does is list content in a subdir of the storage? subdir_list_content subdir_list_files list_subdir_files subdir_file_list get_subdir_files (since we already of get_subdir) > + 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!/([^/]+\.iso)$!i; > + > + $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) = @_; > + > + 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; > + } > + } > + > + 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 []; this change would become unnecessary > > my $res = Dumper PVE::Storage::template_list($cfg, $storagename, > "vztmpl"); > if ( $hash ne $res ) { > -- > 2.20.1 > > _______________________________________________ > pve-devel mailing list > pve-devel@pve.proxmox.com > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
