[PATCH pve-storage v1 06/54] plugin api: replace helpers w/ standalone subs, bump API version & age

["Max R. Carrara" <m.carrara@proxmox.com>]

Replace the `get_vtype_subdirs()` and `get_subdir()` helpers in
PVE::Storage::Plugin with standalone helper subroutines
`plugin_get_default_vtype_subdirs()` and `plugin_get_vtype_subdir()`
in PVE::Storage::Common.

This is mostly to prevent third-party plugins from relying on these
helpers as if they were part of the storage plugin API; the signature
of `get_subdir()` in particular looks as if it belongs to an API
method, even though it is only used as a helper in PVE::Storage,
PVE::Storage::Plugin, and PVE::Storage::BTRFSPlugin.

Therefore, make it explicit that these subroutines are really just
helpers by replacing them with slightly altered versions in
PVE::Storage::Common. Add docstrings for these replacements as well.

Additionally, note that PVE::Storage::ESXiPlugin is the only plugin
that explicitly overrides `get_subdir()`, treating it as if it were an
API method, but because that plugin is not filesystem-based (in the
sense that it does not declare the `path` property in its options),
calling `get_subdir()` would throw an exception anyhow. Also, it
should be mentioned that all methods that could call into
`get_subdir()` have been overridden inside ESXiPlugin anyway.

Therefore, the override of `get_subdir()` in PVE::Storage::ESXiPlugin
is superfluous, so remove it.

Additionally, replace all occurrences of `get_vtype_subdirs()` and
`get_subdir()` across the repository with their newly introduced
counterparts `plugin_get_default_vtype_subdirs()` and
`plugin_get_vtype_subdir()`. Adapt any comments and test names as well
in accordance with these changes.

Finally, increment APIAGE and APIVER in PVE::Storage and add a
corresponding entry to the API changelog.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 ApiChangeLog                        | 22 ++++++++++++
 src/PVE/Storage.pm                  | 31 +++++++++--------
 src/PVE/Storage/BTRFSPlugin.pm      | 15 +++++----
 src/PVE/Storage/Common.pm           | 52 +++++++++++++++++++++++++++++
 src/PVE/Storage/ESXiPlugin.pm       |  6 ----
 src/PVE/Storage/Plugin.pm           | 47 ++++++++++----------------
 src/test/get_subdir_test.pm         | 12 ++++---
 src/test/parse_volname_test.pm      |  6 +++-
 src/test/path_to_volume_id_test.pm  |  6 +++-
 src/test/run_volume_access_tests.pl |  5 ++-
 10 files changed, 140 insertions(+), 62 deletions(-)

diff --git a/ApiChangeLog b/ApiChangeLog
index de411927..d1d3e1c3 100644
--- a/ApiChangeLog
+++ b/ApiChangeLog
@@ -6,6 +6,28 @@ without breaking anything unaware of it.)

 Future changes should be documented in here.

+## Version 14:
+
+* Replace plugin helper `get_vtype_subdirs()` with standalone helper subroutine
+  `PVE::Storage::Common::plugin_get_default_vtype_subdirs()`
+
+  The `get_vtype_subdirs()` subroutine in `PVE::Storage::Plugin` was an
+  internal helper with a public signature. In order to make it clear that this
+  is a helper and not meant to be part of the storage plugin API, it is
+  replaced with the more explicit
+  `PVE::Storage::Common::plugin_get_default_vtype_subdirs()` subroutine.
+
+* Replace plugin method `get_subdir()` with standalone helper subroutine
+  `PVE::Storage::Common::plugin_get_vtype_subdir()`
+
+  The `get_subdir()` method in `PVE::Storage::Plugin` was an internal helper
+  method with a public signature. Moreover, its signature could mistakenly
+  suggest that this method was part of the storage plugin API, which it was not
+  originally meant to be.
+
+  In order to make it clear that this is just a helper, it is replaced with the
+  more explicit `PVE::Storage::Common::plugin_get_vtype_subdir()` subroutine.
+
 ## Version 13:

 * Add new parameter $hints to the `activate_volume()` and `map_volume()` plugin methods
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 6e87bac3..ef2e7887 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -22,6 +22,9 @@ use PVE::JSONSchema;
 use PVE::INotify;
 use PVE::RPCEnvironment;
 use PVE::SSHInfo;
+use PVE::Storage::Common qw(
+    plugin_get_vtype_subdir
+);
 use PVE::RESTEnvironment qw(log_warn);

 use PVE::Storage::Plugin;
@@ -41,11 +44,11 @@ use PVE::Storage::BTRFSPlugin;
 use PVE::Storage::ESXiPlugin;

 # Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 13;
+use constant APIVER => 14;
 # 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 => 4;
+use constant APIAGE => 5;

 our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];

@@ -530,7 +533,7 @@ sub get_image_dir {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});

-    my $path = $plugin->get_subdir($scfg, 'images');
+    my $path = plugin_get_vtype_subdir($scfg, 'images');

     return $vmid ? "$path/$vmid" : $path;
 }
@@ -541,7 +544,7 @@ sub get_private_dir {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});

-    my $path = $plugin->get_subdir($scfg, 'rootdir');
+    my $path = plugin_get_vtype_subdir($scfg, 'rootdir');

     return $vmid ? "$path/$vmid" : $path;
 }
@@ -552,7 +555,7 @@ sub get_iso_dir {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});

-    return $plugin->get_subdir($scfg, 'iso');
+    return plugin_get_vtype_subdir($scfg, 'iso');
 }

 sub get_import_dir {
@@ -561,7 +564,7 @@ sub get_import_dir {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});

-    return $plugin->get_subdir($scfg, 'import');
+    return plugin_get_vtype_subdir($scfg, 'import');
 }

 sub get_vztmpl_dir {
@@ -570,7 +573,7 @@ sub get_vztmpl_dir {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});

-    return $plugin->get_subdir($scfg, 'vztmpl');
+    return plugin_get_vtype_subdir($scfg, 'vztmpl');
 }

 sub get_backup_dir {
@@ -579,7 +582,7 @@ sub get_backup_dir {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});

-    return $plugin->get_subdir($scfg, 'backup');
+    return plugin_get_vtype_subdir($scfg, 'backup');
 }

 # library implementation
@@ -713,12 +716,12 @@ sub path_to_volume_id {
         my $scfg = $ids->{$sid};
         next if !$scfg->{path};
         my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-        my $imagedir = $plugin->get_subdir($scfg, 'images');
-        my $isodir = $plugin->get_subdir($scfg, 'iso');
-        my $tmpldir = $plugin->get_subdir($scfg, 'vztmpl');
-        my $backupdir = $plugin->get_subdir($scfg, 'backup');
-        my $snippetsdir = $plugin->get_subdir($scfg, 'snippets');
-        my $importdir = $plugin->get_subdir($scfg, 'import');
+        my $imagedir = plugin_get_vtype_subdir($scfg, 'images');
+        my $isodir = plugin_get_vtype_subdir($scfg, 'iso');
+        my $tmpldir = plugin_get_vtype_subdir($scfg, 'vztmpl');
+        my $backupdir = plugin_get_vtype_subdir($scfg, 'backup');
+        my $snippetsdir = plugin_get_vtype_subdir($scfg, 'snippets');
+        my $importdir = plugin_get_vtype_subdir($scfg, 'import');

         if ($path =~ m!^\Q$imagedir\E/(\d+)/([^/\s]+)$!) {
             my $vmid = $1;
diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index e68d2bf9..67b442f6 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -11,6 +11,9 @@ use File::Path qw(mkpath);
 use IO::Dir;
 use POSIX qw(EEXIST);

+use PVE::Storage::Common qw(
+    plugin_get_vtype_subdir
+);
 use PVE::Tools qw(run_command dir_glob_foreach);

 use PVE::Storage::DirPlugin;
@@ -188,7 +191,7 @@ sub filesystem_path {

     my ($vtype, $name, $vmid, undef, undef, $isBase, $format) = $class->parse_volname($volname);

-    my $path = $class->get_subdir($scfg, $vtype);
+    my $path = plugin_get_vtype_subdir($scfg, $vtype);

     $path .= "/$vmid" if $vtype eq 'images';

@@ -294,7 +297,7 @@ sub clone_image {
         return PVE::Storage::DirPlugin::clone_image(@_);
     }

-    my $imagedir = $class->get_subdir($scfg, 'images');
+    my $imagedir = plugin_get_vtype_subdir($scfg, 'images');
     $imagedir .= "/$vmid";
     mkpath $imagedir;

@@ -327,7 +330,7 @@ sub alloc_image {

     # From Plugin.pm:

-    my $imagedir = $class->get_subdir($scfg, 'images') . "/$vmid";
+    my $imagedir = plugin_get_vtype_subdir($scfg, 'images') . "/$vmid";

     mkpath $imagedir;

@@ -643,7 +646,7 @@ sub volume_has_feature {

 sub list_images {
     my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
-    my $imagedir = $class->get_subdir($scfg, 'images');
+    my $imagedir = plugin_get_vtype_subdir($scfg, 'images');

     my $res = [];

@@ -844,7 +847,7 @@ sub volume_import {
         $volname = $class->find_free_diskname($storeid, $scfg, $vmid, $volume_format, 1);
     }

-    my $imagedir = $class->get_subdir($scfg, $vtype);
+    my $imagedir = plugin_get_vtype_subdir($scfg, $vtype);
     $imagedir .= "/$vmid" if $vtype eq 'images';

     my $tmppath = "$imagedir/recv.$vmid.tmp";
@@ -975,7 +978,7 @@ sub rename_volume {
         if !$target_volname;
     $target_volname = "$target_vmid/$target_volname";

-    my $basedir = $class->get_subdir($scfg, 'images');
+    my $basedir = plugin_get_vtype_subdir($scfg, 'images');

     mkpath "${basedir}/${target_vmid}";
     my $source_dir = raw_name_to_dir($source_volname);
diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
index 76784e9e..52bd627d 100644
--- a/src/PVE/Storage/Common.pm
+++ b/src/PVE/Storage/Common.pm
@@ -17,6 +17,9 @@ our @EXPORT_OK = qw(
     qemu_img_info
     qemu_img_measure
     qemu_img_resize
+
+    plugin_get_default_vtype_subdirs
+    plugin_get_vtype_subdir
 );

 use constant {
@@ -24,6 +27,16 @@ use constant {
     FALLOC_FL_PUNCH_HOLE => 0x02, # see linux/falloc.h
 };

+my $DEFAULT_VTYPE_SUBDIRS = {
+    images => 'images',
+    rootdir => 'private',
+    iso => 'template/iso',
+    vztmpl => 'template/cache',
+    backup => 'dump',
+    snippets => 'snippets',
+    import => 'import',
+};
+
 =head1 NAME

 PVE::Storage::Common - Shared functions and utilities for storage plugins and storage operations
@@ -283,4 +296,43 @@ sub qemu_img_resize {
     run_command($cmd, timeout => $timeout);
 }

+=head2 FUNCTIONS FOR STORAGE PLUGINS AND CONFIGURATIONS
+
+=cut
+
+=head3 plugin_get_default_vtype_subdirs
+
+    my $vtype_subdirs = plugin_get_default_vtype_subdirs()
+
+Returns a hashref containing the default sub-directories for every volume type.
+
+=cut
+
+sub plugin_get_default_vtype_subdirs : prototype() () {
+    return { $DEFAULT_VTYPE_SUBDIRS->%* }; # shallow copy
+}
+
+=head3 plugin_get_vtype_subdir
+
+    my $subdir = plugin_get_vtype_subdir($scfg, $vtype)
+
+Returns the sub-directory for the volume type C<$vtype> of a given storage
+configuration C<$scfg>.
+
+Raises an exception if the storage config has no C<path> attribute or
+if the given C<$vtype> does not exist.
+
+=cut
+
+sub plugin_get_vtype_subdir : prototype($$) ($scfg, $vtype) {
+    my $path = $scfg->{path};
+
+    die "storage definition has no path\n" if !$path;
+    die "unknown vtype '$vtype'\n" if !exists($DEFAULT_VTYPE_SUBDIRS->{$vtype});
+
+    my $subdir = $scfg->{"content-dirs"}->{$vtype} // $DEFAULT_VTYPE_SUBDIRS->{$vtype};
+
+    return "$path/$subdir";
+}
+
 1;
diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm
index f89e427f..da5abb04 100644
--- a/src/PVE/Storage/ESXiPlugin.pm
+++ b/src/PVE/Storage/ESXiPlugin.pm
@@ -605,12 +605,6 @@ sub volume_has_feature {
     return undef;
 }

-sub get_subdir {
-    my ($class, $scfg, $vtype) = @_;
-
-    die "no subdirectories available for storage $class\n";
-}
-
 package PVE::Storage::ESXiPlugin::Manifest;

 use strict;
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 07233db3..55268b29 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -15,7 +15,10 @@ use PVE::Tools qw(run_command);
 use PVE::JSONSchema qw(get_standard_option register_standard_option);
 use PVE::Cluster qw(cfs_register_file);

-use PVE::Storage::Common;
+use PVE::Storage::Common qw(
+    plugin_get_default_vtype_subdirs
+    plugin_get_vtype_subdir
+);

 use JSON;

@@ -836,31 +839,16 @@ sub parse_volname {
     die "unable to parse directory volume name '$volname'\n";
 }

-my $vtype_subdirs = {
-    images => 'images',
-    rootdir => 'private',
-    iso => 'template/iso',
-    vztmpl => 'template/cache',
-    backup => 'dump',
-    snippets => 'snippets',
-    import => 'import',
-};
-
+# FIXME: remove on the next APIAGE reset.
 sub get_vtype_subdirs {
-    return $vtype_subdirs;
+    return plugin_get_default_vtype_subdirs();
 }

+# FIXME: remove on the next APIAGE reset.
 sub get_subdir {
     my ($class, $scfg, $vtype) = @_;

-    my $path = $scfg->{path};
-
-    die "storage definition has no path\n" if !$path;
-    die "unknown vtype '$vtype'\n" if !exists($vtype_subdirs->{$vtype});
-
-    my $subdir = $scfg->{"content-dirs"}->{$vtype} // $vtype_subdirs->{$vtype};
-
-    return "$path/$subdir";
+    return plugin_get_vtype_subdir($scfg, $vtype);
 }

 my sub get_snap_name {
@@ -885,7 +873,7 @@ sub filesystem_path {
     die "can't snapshot this image format\n"
         if defined($snapname) && $format !~ m/^(qcow2|qed)$/;

-    my $dir = $class->get_subdir($scfg, $vtype);
+    my $dir = plugin_get_vtype_subdir($scfg, $vtype);

     $dir .= "/$vmid" if $vtype eq 'images';

@@ -1016,7 +1004,7 @@ sub clone_image {

     die "clone_image only works on base images\n" if !$isBase;

-    my $imagedir = $class->get_subdir($scfg, 'images');
+    my $imagedir = plugin_get_vtype_subdir($scfg, 'images');
     $imagedir .= "/$vmid";

     mkpath $imagedir;
@@ -1049,7 +1037,7 @@ sub clone_image {
 sub alloc_image {
     my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;

-    my $imagedir = $class->get_subdir($scfg, 'images');
+    my $imagedir = plugin_get_vtype_subdir($scfg, 'images');
     $imagedir .= "/$vmid";

     mkpath $imagedir;
@@ -1608,7 +1596,7 @@ sub volume_has_feature {
 sub list_images {
     my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;

-    my $imagedir = $class->get_subdir($scfg, 'images');
+    my $imagedir = plugin_get_vtype_subdir($scfg, 'images');

     my $format_info = $class->get_formats($scfg, $storeid);
     my $fmts = join('|', sort keys $format_info->{valid}->%*);
@@ -1756,7 +1744,7 @@ sub list_volumes {
         if ($type eq 'images' || $type eq 'rootdir') {
             $data = $class->list_images($storeid, $scfg, $vmid);
         } elsif ($scfg->{path}) {
-            my $path = $class->get_subdir($scfg, $type);
+            my $path = plugin_get_vtype_subdir($scfg, $type);

             if ($type eq 'iso' && !defined($vmid)) {
                 $data = get_subdir_files($storeid, $path, 'iso');
@@ -1909,13 +1897,14 @@ sub activate_storage {
             # FIXME The mkdir option is deprecated. Remove with PVE 9?
             && (!defined($scfg->{mkdir}) || $scfg->{mkdir})
         ) {
-            for my $vtype (sort keys %$vtype_subdirs) {
+            my $vtype_subdirs = plugin_get_default_vtype_subdirs();
+            for my $vtype (sort keys $vtype_subdirs->%*) {
                 # OpenVZMigrate uses backup (dump) dir
                 if (
                     defined($scfg->{content}->{$vtype})
                     || ($vtype eq 'backup' && defined($scfg->{content}->{'rootdir'}))
                 ) {
-                    my $subdir = $class->get_subdir($scfg, $vtype);
+                    my $subdir = plugin_get_vtype_subdir($scfg, $vtype);
                     mkpath $subdir if $subdir ne $path;
                 }
             }
@@ -1924,7 +1913,7 @@ sub activate_storage {
         # check that content dirs are pairwise inequal
         my $resolved_subdirs = {};
         for my $vtype (sort keys $scfg->{content}->%*) {
-            my $subdir = $class->get_subdir($scfg, $vtype);
+            my $subdir = plugin_get_vtype_subdir($scfg, $vtype);
             my $abs_subdir = abs_path($subdir);
             next if !defined($abs_subdir);

@@ -2294,7 +2283,7 @@ sub rename_volume {
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
         if !$target_volname;

-    my $basedir = $class->get_subdir($scfg, 'images');
+    my $basedir = plugin_get_vtype_subdir($scfg, 'images');

     mkpath "${basedir}/${target_vmid}";

diff --git a/src/test/get_subdir_test.pm b/src/test/get_subdir_test.pm
index 5fb54458..843c8c39 100644
--- a/src/test/get_subdir_test.pm
+++ b/src/test/get_subdir_test.pm
@@ -5,16 +5,20 @@ use warnings;

 use lib qw(..);

+use PVE::Storage::Common qw(
+    plugin_get_default_vtype_subdirs
+    plugin_get_vtype_subdir
+);
 use PVE::Storage::Plugin;
 use Test::More;

 my $scfg_with_path = { path => '/some/path' };
-my $vtype_subdirs = PVE::Storage::Plugin::get_vtype_subdirs();
+my $vtype_subdirs = plugin_get_default_vtype_subdirs();

 # each test is comprised of the following array keys:
 # [0] => storage config; positive with path key
 # [1] => storage type;  see $vtype_subdirs
-# [2] => expected return from get_subdir
+# [2] => expected return from plugin_get_vtype_subdir
 my $tests = [
     # failed matches
     [$scfg_with_path, 'none', "unknown vtype 'none'\n"],
@@ -45,10 +49,10 @@ foreach my $tt (@$tests) {
     my ($scfg, $type, $expected) = @$tt;

     my $got;
-    eval { $got = PVE::Storage::Plugin->get_subdir($scfg, $type) };
+    eval { $got = plugin_get_vtype_subdir($scfg, $type) };
     $got = $@ if $@;

-    is($got, $expected, "get_subdir for $type") || diag(explain($got));
+    is($got, $expected, "plugin_get_vtype_subdir for $type") || diag(explain($got));
 }

 done_testing();
diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm
index 5c9478ac..2ff4c6d0 100644
--- a/src/test/parse_volname_test.pm
+++ b/src/test/parse_volname_test.pm
@@ -6,6 +6,9 @@ use warnings;
 use lib qw(..);

 use PVE::Storage;
+use PVE::Storage::Common qw(
+    plugin_get_default_vtype_subdirs
+);
 use Test::More;

 my $vmid = 1234;
@@ -302,7 +305,8 @@ foreach my $virt (keys %$non_bkp_suffix) {
 plan tests => scalar @$tests + 1;

 my $seen_vtype;
-my $vtype_subdirs = { map { $_ => 1 } keys %{ PVE::Storage::Plugin::get_vtype_subdirs() } };
+my $vtype_subdirs =
+    { map { $_ => 1 } keys %{ plugin_get_default_vtype_subdirs() } };

 foreach my $t (@$tests) {
     my $description = $t->{description};
diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm
index dfa51e64..e7e36037 100644
--- a/src/test/path_to_volume_id_test.pm
+++ b/src/test/path_to_volume_id_test.pm
@@ -6,6 +6,9 @@ use warnings;
 use lib qw(..);

 use PVE::Storage;
+use PVE::Storage::Common qw(
+    plugin_get_default_vtype_subdirs
+);

 use Test::More;

@@ -237,7 +240,8 @@ my @tests = (
 plan tests => scalar @tests + 1;

 my $seen_vtype;
-my $vtype_subdirs = { map { $_ => 1 } keys %{ PVE::Storage::Plugin::get_vtype_subdirs() } };
+my $vtype_subdirs =
+    { map { $_ => 1 } keys %{ plugin_get_default_vtype_subdirs() } };

 foreach my $tt (@tests) {
     my $file = $tt->{volname};
diff --git a/src/test/run_volume_access_tests.pl b/src/test/run_volume_access_tests.pl
index 34487083..31bacaa9 100755
--- a/src/test/run_volume_access_tests.pl
+++ b/src/test/run_volume_access_tests.pl
@@ -10,6 +10,9 @@ use lib ('.', '..');

 use PVE::RPCEnvironment;
 use PVE::Storage;
+use PVE::Storage::Common qw(
+    plugin_get_default_vtype_subdirs
+);
 use PVE::Storage::Plugin;

 my $storage_cfg = <<'EOF';
@@ -64,7 +67,7 @@ $pve_cluster_module->mock(
 my $rpcenv = PVE::RPCEnvironment->init('pub');
 $rpcenv->init_request();

-my @types = sort keys PVE::Storage::Plugin::get_vtype_subdirs()->%*;
+my @types = sort keys plugin_get_default_vtype_subdirs()->%*;
 my $all_types = { map { $_ => 1 } @types };

 my @tests = (
--
2.47.3