[pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Max Carrara]

RFC: Tighter API Control for Storage Plugins - v1
=================================================

Since this has been cooking for a while I've decided to send this in as
an RFC in order to get some early feedback in.

Note that this is quite experimental and also a little more complex;
I'll try my best to explain my reasoning for the changes in this RFC
below.

Any feedback on this is greatly appreciated!

Introduction
------------

While working on a refactor of the Storage Plugin API, I've often hit
cases where I needed to move a subroutine around, but couldn't
confidently do so due to the code being rather old and the nature of
Perl as a language.

I had instances where things would spontaneously break here and there
after moving an inocuous-looking helper subroutine, due to it actually
being used in a different module / plugin, or worse, in certain cases in
a completely different package, because a plugin's helper sub ended
up being spontaneously used there.

To remedy this, I had originally added a helper subroutine for emitting
deprecation warnings. The plan back then was to leave the subroutines at
their original locations and have them call their replacement under the
hood while emitting a warning. Kind of like so:

  # PVE::Storage::SomePlugin

  sub some_helper_sub {
      my ($arg) = @_;

      # Emit deprecation warning here to hopefully catch any unexpected
      # callsites
      warn get_deprecation_warning(
          "PVE::Storage::Common::some_helper_sub"
      );

      # Call relocated helper here
      return PVE::Storage::Common::some_helper_sub($arg);
  }

This approach was decent-ish, but didn't *actually* guard against any
mishaps, unfortunately. With this approach, there is no guarantee that
the callsite will actually be caught, especially if e.g. a third-party
storage plugin was also using that subroutine and the plugin author
didn't bother checking or reading their CI logs.

What I instead wanted to have was some "strong guarantee" that a
subroutine absolutely cannot be used anymore after a certain point, e.g.
after a certain API version or similar.

This gave me an idea: What if there was a way to check this at
"compile-time" in Perl instead of dynamically emitting warnings and
hoping that somebody eventually sees them?

Instead of just focusing on helper subroutines, I decided to expand on
this idea further and make it so that the storage API can actually
enforce API-conformance at "compile-time".

Right now, when a third-party plugin says it has API level X, we simply
trust that it has API level X. It's up to the plugin author to ensure
that it actually does -- and it's also up to the administrator to test
the plugin and ensure that nothing breaks when it's being used.

This is not a strong guarantee, mainly because a plugin author may e.g.
simply forget to adapt a method in their plugin, the administrator might
install a plugin carelessly, etc. At the same time, we don't have a
strong guarantee that *our* plugins conform to the API as well; in other
words, it's hard to make any bigger changes without being confident that
nothing broke in the process.

Overview of what this RFC Proposes to add
-----------------------------------------

- Patch 01: Separate storage API version handling from main code and
  introduce general subroutine deprecation mechanism

- Patch 02: Rework plugin loading and registration mechanism in order to
  allow adding compile-time checks

- Patch 03: Actually add compile-time check that prevents plugins from
  overriding certain methods (which exactly still needs to be decided)

- Patch 04: Use the new check from patch 03, replacing some FIXME
  comments as example

- Patch 05 & 06: Add convenience checks for SectionConfig conformity;
  these are mostly there to improve the "developer experience" to make
  developing a third-party plugin a little easier for third parties

The exact changes are much better explained in each of the patches'
commit message. Patches 01 - 04 are where most of the bulk work is being
done; patch 05 & 06 just show how the plugin registration mechanism can
be expanded.

Closing Thoughts
----------------

This still needs quite a bit of polishing and could perhaps use
PVE::Path [3] once it's merged. As I said in the beginning, I wanted to
send this out in order to get some feedback in first before continuing
to iterate on this, so any feedback is highly appreciated!

The other question is whether we actually want something like this; I
personally would be in favour of stricter checks in our Perl code, but
at the same time, I don't want to overengineer things.

The last thing I want to add (should this get greenlighted) is a
repertoire of tests that actually verify whether the API checks work as
intended with third party modules. Since this is quite hard and quite
elaborate to test, I decided to omit that here for now, as it would just
bloat the RFC (and potentially detract from the main idea).

References
----------

[1]: https://lore.proxmox.com/pve-devel/20240717094034.124857-8-m.carrara@proxmox.com/
[2]: https://perldoc.perl.org/Attribute::Handlers
[3]: https://lore.proxmox.com/pve-devel/20250109144818.430185-1-m.carrara@proxmox.com/

Summary of Changes
------------------

Max Carrara (6):
  version: introduce PVE::Storage::Version
  rework plugin loading and registration mechanism
  introduce compile-time checks for prohibited sub overrides
  plugin: replace fixme comments for deprecated methods with attribute
  introduce check for `type` method and duplicate types
  introduce check for duplicate plugin properties

 src/PVE/Storage.pm              | 647 ++++++++++++++++++++++++++++----
 src/PVE/Storage/CIFSPlugin.pm   |   4 -
 src/PVE/Storage/CephFSPlugin.pm |   4 -
 src/PVE/Storage/DirPlugin.pm    |   5 +-
 src/PVE/Storage/Makefile        |   1 +
 src/PVE/Storage/NFSPlugin.pm    |   4 -
 src/PVE/Storage/PBSPlugin.pm    |   4 -
 src/PVE/Storage/Plugin.pm       | 147 +++++++-
 src/PVE/Storage/Version.pm      | 433 +++++++++++++++++++++
 9 files changed, 1146 insertions(+), 103 deletions(-)
 create mode 100644 src/PVE/Storage/Version.pm

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC v1 pve-storage 1/6] version: introduce PVE::Storage::Version

[Max Carrara]

The purpose of this module is to separate the handling of the Storage
API's version from the remaining code while also providing additional
mechanisms to handle changes to the Storage API more gracefully.

The first such mechanism is the `pveStorageDeprecateAt` attribute,
which, when added to a subroutine, marks the subroutine as deprecated
at a specific version. Should the lowest supported API version
increase beyond the version passed to the attribute, the module will
fail to compile (during Perl's CHECK phase).

This provides a more robust mechanism instead of having to rely on
"FIXME" comments or similar.

Unfortunately attributes can't conveniently be exported using the
`Exporter` module or similar mechanisms, and they can neither be
referenced via their fully qualified path. The attribute's name is
therefore intentionally made long in order to not conflict with any
other possible attributes in the UNIVERSAL namespace.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage.pm         |  10 +--
 src/PVE/Storage/Makefile   |   1 +
 src/PVE/Storage/Version.pm | 180 +++++++++++++++++++++++++++++++++++++
 3 files changed, 185 insertions(+), 6 deletions(-)
 create mode 100644 src/PVE/Storage/Version.pm

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 3b4f041..df4d62f 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -24,6 +24,8 @@ use PVE::RPCEnvironment;
 use PVE::SSHInfo;
 use PVE::RESTEnvironment qw(log_warn);
 
+use PVE::Storage::Version;
+
 use PVE::Storage::Plugin;
 use PVE::Storage::DirPlugin;
 use PVE::Storage::LVMPlugin;
@@ -41,12 +43,8 @@ use PVE::Storage::PBSPlugin;
 use PVE::Storage::BTRFSPlugin;
 use PVE::Storage::ESXiPlugin;
 
-# Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 10;
-# 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 APIVER => PVE::Storage::Version::APIVER;
+use constant APIAGE => PVE::Storage::Version::APIAGE;
 
 our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
 
diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
index ce3fd68..5315f69 100644
--- a/src/PVE/Storage/Makefile
+++ b/src/PVE/Storage/Makefile
@@ -1,5 +1,6 @@
 SOURCES= \
 	Common.pm \
+	Version.pm \
 	Plugin.pm \
 	DirPlugin.pm \
 	LVMPlugin.pm \
diff --git a/src/PVE/Storage/Version.pm b/src/PVE/Storage/Version.pm
new file mode 100644
index 0000000..a9216c9
--- /dev/null
+++ b/src/PVE/Storage/Version.pm
@@ -0,0 +1,180 @@
+=head1 NAME
+
+C<PVE::Storage::Version> - Storage API Version Management
+
+=head1 DESCRIPTION
+
+This module is concerned with tracking and managing the version of the Storage
+API interface C<L<PVE::Storage::Plugin>>. Furthermore, this module also provides
+a mechanism to I<deprecate> subroutines that are part of the Storage API via a
+custom I<attribute> called C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>>.
+
+For more information on this mechanism, see L</"DEPRECATING SUBROUTINES"> below.
+
+B<NOTE:> Importing this module will define the C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>>
+attribute inside the C<UNIVERSAL> (global) namespace. Ensure that no other
+symbols have the same name as this attribute in order to avoid conflicts.
+
+=cut
+
+package PVE::Storage::Version;
+
+use strict;
+use warnings;
+
+use v5.36;
+
+use Attribute::Handlers;
+use Carp;
+use Scalar::Util qw(reftype);
+
+# NOTE: This module must not import PVE::Storage or any of its submodules,
+# either directly or transitively.
+#
+# Otherwise there's a risk of introducing nasty cyclical imports and/or causing
+# obscure errors during compile time, which may be very hard to debug.
+
+=head1 CONSTANTS
+
+=head3 APIVER
+
+The version of the Storage API.
+
+This version must be incremented if changes to the C<L<PVE::Storage::Plugin>>
+API interface are made.
+
+=head3 APIAGE
+
+The API age is the number of versions we're backward compatible with.
+
+This is similar to having C<current='APIVER'> and C<age='APIAGE'> in C<libtool>.
+For more information, see L<here|https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html>.
+
+Generally, you'll want to increment this when incrementing C<L<< APIVER|/APIVER >>>.
+Resetting C<APIAGE> to C<0> means that backward compatibility is broken and should
+only be done when appropriate (e.g. during a release of a new major version of PVE).
+
+=cut
+
+use constant APIVER => 10;
+use constant APIAGE => 1;
+
+my $MIN_VERSION = (APIVER - APIAGE);
+
+
+=head1 DEPRECATING SUBROUTINES
+
+Upon importing this module, the C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>>
+attribute becomes available. This attribute ought to be used similar to how
+prototypes are used in Perl and requires a C<version> and a C<message> key to
+be passed:
+
+    sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") {
+	# [...]
+    }
+
+Most likely you'll need more space for the C<message>, in which case the
+attribute may simply span multiple lines:
+
+    sub some_sub : pveStorageDeprecateAt(
+	version => 42,
+	message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
+	    . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium"
+	    . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl"
+	    . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel"
+	    . " suscipit lorem varius. Mauris et erat posuere, gravida dui"
+	    . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet,"
+	    . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id"
+	    . " diam venenatis, nec placerat felis viverra.",
+    ) {
+	# [...]
+    }
+
+As soon as the passed C<version> is below the I<lowest supported version>
+(C<L<< APIVER|/APIVER >>> minus C<L<< APIAGE|/APIAGE >>>), the attribute raises
+an exception during the C<CHECK> phase of the Perl interpreter and also displays
+the passed C<message>. This happens before any runtime code is executed.
+
+Should the passed C<version> be equal to the lowest supported version, a warning
+containing the passed C<message> is emitted whenever the subroutine is called
+instead. This ensures that implementors of custom plugins are made aware of
+subroutines being deprecated, should there be any in use.
+
+The C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>> attribute
+may only be used inside C<L<PVE::Storage>> or its submodules and throws an error
+if used elsewhere.
+
+This mechanism's sole purpose is to enforce deprecation of subroutines during
+"compile time", forcing the subroutine to be removed once the API version is
+incremented too far or the API age is reset.
+
+=head1 ATTRIBUTES
+
+=head3 UNIVERSAL::pveStorageDeprecateAt
+
+This attribute marks subroutines as deprecated at a specific C<version> and will
+emit an error with the given C<message> if the lowest supported Storage API
+version exceeds the C<version> passed to the attribute.
+
+    sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") {
+	# [...]
+    }
+
+    sub some_sub : pveStorageDeprecateAt(
+	version => 42,
+	message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
+	    . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium"
+	    . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl"
+	    . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel"
+	    . " suscipit lorem varius. Mauris et erat posuere, gravida dui"
+	    . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet,"
+	    . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id"
+	    . " diam venenatis, nec placerat felis viverra.",
+    ) {
+	# [...]
+    }
+
+Should the passed C<version> be equal to the currently lowest supported API
+version, a warning containing the passed C<message> is emitted instead.
+
+=cut
+
+sub UNIVERSAL::pveStorageDeprecateAt : ATTR(CODE,CHECK) {
+    my ($package, $symbol, $referent, $attr_name, $attr_data, $phase, $filename, $line) = @_;
+
+    confess "'$attr_name' attribute may only be used inside PVE::Storage and its submodules"
+	if $package !~ m/^PVE::Storage/;
+
+    my $data = { $attr_data->@* };
+
+    my ($version, $message) = ($data->{version}, $data->{message});
+    my $reftype = reftype($referent);
+
+    confess "Referent is not a reference"
+	. " -- '$attr_name' attribute may only be used for subroutines"
+	if !defined($reftype);
+
+    confess "Unexpected reftype '$reftype'"
+	. " -- '$attr_name' attribute may only be used for subroutines"
+	if !($reftype eq 'CODE' || $reftype eq 'ANON');
+
+    confess "Either version or message not defined"
+	. " -- usage: $attr_name(version => 42, message => \"...\")"
+	if !defined($version) || !defined($message);
+
+    confess "Storage API version '$version' not supported: $message"
+	if $version < $MIN_VERSION;
+
+    no warnings 'redefine';
+    *$symbol = sub {
+	my $symbol_name = *{$symbol}{"NAME"};
+	carp "Warning: subroutine '$symbol_name' is deprecated - $message"
+	    if $version == $MIN_VERSION;
+	$referent->(@_);
+    };
+
+    return;
+}
+
+
+1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC v1 pve-storage 2/6] rework plugin loading and registration mechanism

[Max Carrara]

This commit changes how plugins, both inbuilt and custom, are loaded
and registered by the `PVE::Storage` module from the ground up.

This is done for mainly two reasons:

 1. Unifying the plugin loading mechanism so that inbuilt and custom
    plugins are subjected to (almost) the same checks.

 2. Making it possible to easily extend the loading / registering
    mechanism for additional compile-time (BEGIN phase [1]) checks.

The new plugin registration mechanism essentially consists of four
stages:

 1. Load plugins into the Perl interpreter
 2. Perform pre-registration checks
 3. Registration
 4. Perform post-registration checks

Each stage will perform its specific actions on inbuilt plugins first
and on custom plugins afterwards.

In general, should a stage fail for an inbuilt plugin, an exception is
thrown. For custom plugins a warning is emitted instead and the plugin
won't be considered in the subsequent stage(s).

The new registration mechanism is equivalent to the old one, with a
few minor, but notable exceptions:

 1. Stage 1. will also verify whether each plugin (inbuilt or custom)
    is installed inside `/usr/share/perl5/` and not anywhere else in
    order to guarantee that there are no conflicts while loading.

    Example: A particularly impertinent custom plugin *may* install
    itself to another path in `@INC`, such as
    `/usr/local/lib/x86_64-linux-gnu/perl/5.36.0` e.g., and mask
    another inbuilt or custom plugin by having the exact same
    namespace.

    While unlikely, this is now guarded against.

 2. The new registration machinery also checks whether inbuilt
    plugins derive from `PVE::Storage::Plugin`.

 3. Inbuilt plugins are no longer imported using `use`, the same logic
    using `require` is now used for both inbuilt and custom plugins.

 4. The registration now happens inside the `import()` subroutine,
    which makes it more predictable when plugins are actually loaded.

    This means that plugins from here on out are only registered when
    running `use PVE::Storage;` and not when using `require
    PVE::Storage;`. The difference here is somewhat subtle; to
    elaborate, the statement `use PVE::Storage;` is equivalent to the
    following [2]:

        BEGIN {
            require "PVE/Storage.pm";
            'PVE::Storage'->import();
        }

    Because `PVE::Storage` isn't imported using a plain `require`
    anywhere in our code, this change is safe to make.

[1]: https://perldoc.perl.org/perlmod#BEGIN%2C-UNITCHECK%2C-CHECK%2C-INIT-and-END
[2]: https://perldoc.perl.org/functions/use

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage.pm | 559 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 489 insertions(+), 70 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index df4d62f..0c8ba64 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -10,6 +10,8 @@ use IO::Socket::IP;
 use IPC::Open3;
 use File::Basename;
 use File::Path;
+use File::Spec;
+use Carp;
 use Cwd 'abs_path';
 use Socket;
 use Time::Local qw(timelocal);
@@ -27,82 +29,12 @@ use PVE::RESTEnvironment qw(log_warn);
 use PVE::Storage::Version;
 
 use PVE::Storage::Plugin;
-use PVE::Storage::DirPlugin;
-use PVE::Storage::LVMPlugin;
-use PVE::Storage::LvmThinPlugin;
-use PVE::Storage::NFSPlugin;
-use PVE::Storage::CIFSPlugin;
-use PVE::Storage::ISCSIPlugin;
-use PVE::Storage::RBDPlugin;
-use PVE::Storage::CephFSPlugin;
-use PVE::Storage::ISCSIDirectPlugin;
-use PVE::Storage::GlusterfsPlugin;
-use PVE::Storage::ZFSPoolPlugin;
-use PVE::Storage::ZFSPlugin;
-use PVE::Storage::PBSPlugin;
-use PVE::Storage::BTRFSPlugin;
-use PVE::Storage::ESXiPlugin;
 
 use constant APIVER => PVE::Storage::Version::APIVER;
 use constant APIAGE => PVE::Storage::Version::APIAGE;
 
 our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
 
-# load standard plugins
-PVE::Storage::DirPlugin->register();
-PVE::Storage::LVMPlugin->register();
-PVE::Storage::LvmThinPlugin->register();
-PVE::Storage::NFSPlugin->register();
-PVE::Storage::CIFSPlugin->register();
-PVE::Storage::ISCSIPlugin->register();
-PVE::Storage::RBDPlugin->register();
-PVE::Storage::CephFSPlugin->register();
-PVE::Storage::ISCSIDirectPlugin->register();
-PVE::Storage::GlusterfsPlugin->register();
-PVE::Storage::ZFSPoolPlugin->register();
-PVE::Storage::ZFSPlugin->register();
-PVE::Storage::PBSPlugin->register();
-PVE::Storage::BTRFSPlugin->register();
-PVE::Storage::ESXiPlugin->register();
-
-# load third-party plugins
-if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
-    dir_glob_foreach('/usr/share/perl5/PVE/Storage/Custom', '.*\.pm$', sub {
-	my ($file) = @_;
-	my $modname = 'PVE::Storage::Custom::' . $file;
-	$modname =~ s!\.pm$!!;
-	$file = 'PVE/Storage/Custom/' . $file;
-
-	eval {
-	    require $file;
-
-	    # Check perl interface:
-	    die "not derived from PVE::Storage::Plugin\n" if !$modname->isa('PVE::Storage::Plugin');
-	    die "does not provide an api() method\n" if !$modname->can('api');
-	    # Check storage API version and that file is really storage plugin.
-	    my $version = $modname->api();
-	    die "implements an API version newer than current ($version > " . APIVER . ")\n"
-		if $version > APIVER;
-	    my $min_version = (APIVER - APIAGE);
-	    die "API version too old, please update the plugin ($version < $min_version)\n"
-		if $version < $min_version;
-	    # all OK, do import and register (i.e., "use")
-	    import $file;
-	    $modname->register();
-
-	    # If we got this far and the API version is not the same, make some noise:
-	    warn "Plugin \"$modname\" is implementing an older storage API, an upgrade is recommended\n"
-		if $version != APIVER;
-	};
-	if ($@) {
-	    warn "Error loading storage plugin \"$modname\": $@";
-	}
-    });
-}
-
-# initialize all plugins
-PVE::Storage::Plugin->init();
-
 # the following REs indicate the number or capture groups via the trailing digit
 # CAUTION don't forget to update the digits accordingly after messing with the capture groups
 
@@ -124,6 +56,493 @@ our $OVA_CONTENT_RE_1 = qr/${SAFE_CHAR_WITH_WHITESPACE_CLASS_RE}+\.(qcow2|raw|vm
 # FIXME remove with PVE 9.0, add versioned breaks for pve-manager
 our $vztmpl_extension_re = $VZTMPL_EXT_RE_1;
 
+# PVE::Storage plugin registration machinery
+
+my $plugin_register_state = {
+    index => {},
+    plugins_initialised => 0,
+};
+
+=head3 get_standard_plugin_names()
+
+Returns an array or a reference to an array of the module names of our inbuilt
+storage plugins, depending on the context of the caller.
+
+    # both valid
+    my $standard_plugins = get_standard_plugin_names();
+    my @standard_plugins = get_standard_plugin_names();
+
+=cut
+
+my sub get_standard_plugin_names : prototype() {
+    my @standard_plugins = qw(
+	PVE::Storage::DirPlugin
+	PVE::Storage::LVMPlugin
+	PVE::Storage::LvmThinPlugin
+	PVE::Storage::NFSPlugin
+	PVE::Storage::CIFSPlugin
+	PVE::Storage::ISCSIPlugin
+	PVE::Storage::RBDPlugin
+	PVE::Storage::CephFSPlugin
+	PVE::Storage::ISCSIDirectPlugin
+	PVE::Storage::GlusterfsPlugin
+	PVE::Storage::ZFSPoolPlugin
+	PVE::Storage::ZFSPlugin
+	PVE::Storage::PBSPlugin
+	PVE::Storage::BTRFSPlugin
+	PVE::Storage::ESXiPlugin
+    );
+
+    return @standard_plugins if wantarray;
+    return \@standard_plugins;
+}
+
+=head3 get_custom_plugin_names()
+
+Returns an array or a reference to an array of the module names of custom
+storage plugins inside C</usr/share/perl5/PVE/Storage/Custom>, depending on the
+context of the caller.
+
+    # both valid
+    my $custom_plugins = get_custom_plugin_names();
+    my @custom_plugins = get_custom_plugin_names();
+
+=cut
+
+my sub get_custom_plugin_names : prototype() {
+    my @custom_plugins = ();
+
+    if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
+	dir_glob_foreach('/usr/share/perl5/PVE/Storage/Custom', '.*\.pm$', sub {
+	    my ($file) = @_;
+
+	    my $modname = 'PVE::Storage::Custom::' . $file;
+	    $modname =~ s!\.pm$!!;
+
+	    push(@custom_plugins, $modname);
+	});
+    }
+
+    return @custom_plugins if wantarray;
+    return \@custom_plugins;
+}
+
+=head3 get_indexed_plugins(%params)
+
+Returns the names of the currently indexed storage plugins, optionally allowing
+certain filters flags to be passed. Returns an array or a reference to an array
+depending on the context of the caller.
+
+    my @all_plugins = get_indexed_plugins();
+
+    my $standard_plugins = get_indexed_plugins(is_custom => 0);
+    my @custom_plugins = get_indexed_plugins(is_custom => 1);
+
+    my @registered_custom_plugins = get_indexed_plugins(is_custom => 1, is_registered => 1);
+
+The following key-value pairs may optionally be passed and will cause this
+subroutine to filter out any plugin names for which they don't apply:
+
+=over
+
+=item * C<< is_custom => [0|1] >>
+
+Whether to only return names of custom plugin names or inbuilt ones.
+
+=item * C<< is_checked => [0|1]>>
+
+Whether to only return plugins that passed or haven't passed the
+pre-registration check.
+
+=item * C<< is_registered => [0|1] >>
+
+Whether to only return plugins that were or weren't registered.
+
+=back
+
+=cut
+
+my sub get_indexed_plugins : prototype(;%) {
+    my (%params) = @_;
+
+    my $plugin_index = $plugin_register_state->{index};
+
+    my $test_index_for_flag = sub {
+	my ($plugin_name, $flag) = @_;
+
+	return 0 if !defined($plugin_index->{$_});
+	return 0 if !defined($plugin_index->{$_}->{$flag});
+
+	return $plugin_index->{$_}->{$flag} == $params{$flag};
+    };
+
+    my @plugins = keys $plugin_index->%*;
+
+    @plugins = grep { $test_index_for_flag->($_, 'is_custom') } @plugins
+	if defined($params{is_custom});
+
+    @plugins = grep { $test_index_for_flag->($_, 'is_checked') } @plugins
+	if defined($params{is_checked});
+
+    @plugins = grep { $test_index_for_flag->($_, 'is_registered') } @plugins
+	if defined($params{is_registered});
+
+    return @plugins if wantarray;
+    return @plugins;
+}
+
+=head3 do_try_load_plugin(%params)
+
+Actual loading mechanism used inside C<L<< try_load_plugins()|/"try_load_plugins()" >>>.
+Attempts to load the plugin provided via the C<name> key. Optionally, C<< is_custom => 1 >>
+may be passed if a custom plugin is being loaded.
+
+    do_try_load_plugin(name => 'PVE::Storage::DirPlugin');
+
+    do_try_load_plugin(name => 'PVE::Storage::Custom::FooPlugin', is_custom => 1);
+
+The given plugin is loaded dynamically via Perl's C<require> and then added to
+the plugin index for further operations, such as pre- and post-registration
+checks as well as the actual registration itself.
+
+This subroutine performs additional sanity checks and will raise an exception if
+the plugin doesn't exist in C</usr/share/perl5/> or if the plugin was already loaded.
+
+=cut
+
+my sub do_try_load_plugin : prototype(%) {
+    my (%params) = @_;
+
+    my $plugin_name = $params{name};
+    my $is_custom = $params{is_custom};
+
+    croak "no plugin name provided - must be fully qualified namespace, e.g. Foo::Bar::Baz"
+	if !defined($plugin_name);
+
+    my $base_path = '/usr/share/perl5/';
+
+    my $plugin_filepath = File::Spec->catdir(split('::', $plugin_name)) . '.pm';
+    my ($_volume, $plugin_dir, $plugin_filename) = File::Spec->splitpath($plugin_filepath);
+
+    my $plugin_filepath_abs = File::Spec->catfile($base_path, $plugin_dir, $plugin_filename);
+
+    croak "plugin '$plugin_name' does not exist at path '$plugin_filepath_abs'"
+	if (! -e $plugin_filepath_abs);
+
+    eval {
+	require $plugin_filepath;
+    };
+    croak "Failed to load plugin '$plugin_name' - $@" if $@;
+
+    my $plugin_index = $plugin_register_state->{index};
+
+    confess "Plugin '$plugin_name' is already loaded" if exists($plugin_index->{$plugin_name});
+
+    $plugin_index->{$plugin_name} = {
+	absolute_path => $plugin_filepath_abs,
+	is_custom => $is_custom || 0,
+	is_checked => 0,
+	is_registered => 0,
+    };
+
+    return;
+}
+
+=head3 try_load_plugins()
+
+Attempts to load all available plugins, with inbuilt ("standard") plugins being
+loaded first and custom plugins loaded afterwards.
+
+All standard plugins must successfully load, otherwise an exception is thrown.
+If a custom plugin fails to load, a warning is emitted instead.
+
+=cut
+
+my sub try_load_plugins : prototype() {
+    my @errs = ();
+
+    for my $plugin (get_standard_plugin_names()) {
+	eval {
+	    do_try_load_plugin(name => $plugin);
+	};
+
+	push(@errs, $@) if $@;
+    }
+
+    confess("Encountered unexpected error(s) while loading plugins:\n", join("\n", @errs), "\n\n... ")
+	if scalar(@errs);
+
+    for my $plugin (get_custom_plugin_names()) {
+	eval {
+	    do_try_load_plugin(name => $plugin, is_custom => 1);
+	};
+
+	warn "Error loading storage plugin \"$plugin\" - $@" if $@;
+    }
+
+    return;
+}
+
+=head3 do_pre_register_check_plugin(%params)
+
+Used within C<L<< pre_register_check_plugins()|/"pre_register_check_plugins()" >>>
+to perform the actual pre-registration checks for the plugin given by the C<name>
+key.
+
+    do_pre_register_check_plugin(name => 'PVE::Storage::DirPlugin');
+
+This checks whether the plugin is derived from C<L<PVE::Storage::Plugin>>. If the
+plugin is a custom plugin, its API conformance is also verified:
+
+=over
+
+=item * The plugin must provide an C<api()> method returning its API version
+
+=item * Its API version must not be newer than the current storage API version
+
+=item * Its API version must not be older than the storage API age allows
+
+=back
+
+=cut
+
+my sub do_pre_register_check_plugin : prototype(%) {
+    my (%params) = @_;
+
+    my $plugin_name = $params{name};
+
+    my $plugin_index = $plugin_register_state->{index};
+
+    croak "Plugin '$plugin_name' does not exist in index"
+	if !defined($plugin_index->{$plugin_name});
+
+    my $is_custom = $plugin_index->{$plugin_name}->{is_custom};
+
+    my $check_derives_base = sub {
+	# so that we may call methods on $plugin_name
+	no strict 'refs'; ## no critic
+
+	die "not derived from PVE::Storage::Plugin\n"
+	    if !$plugin_name->isa('PVE::Storage::Plugin');
+    };
+
+    my $check_plugin_api = sub {
+	# so that we may call methods on $plugin_name
+	no strict 'refs'; ## no critic
+
+	die "does not provide an 'api()' method\n"
+	    if !$plugin_name->can('api');
+
+	my $version = $plugin_name->api();
+	my $min_version = (APIVER - APIAGE);
+
+	die "implements an API version newer than current ($version > @{[APIVER]})\n"
+	    if $version > APIVER;
+
+	die "API version too old, please update the plugin ($version < $min_version)\n"
+	    if $version < $min_version;
+    };
+
+    eval {
+	$check_derives_base->();
+	$check_plugin_api->() if $is_custom;
+    };
+
+    croak "Pre-registration check failed for plugin '$plugin_name' - $@" if $@;
+
+    $plugin_index->{$plugin_name}->{is_checked} = 1;
+
+    return;
+}
+
+=head3 pre_register_check_plugins()
+
+Performs various pre-registration checks for all plugins.
+
+Standard plugins must always pass all checks. If a custom plugin fails a check,
+a warning is emitted instead and it will not be registered later on.
+
+For more details on which checks are performed, see
+C<L<< do_pre_register_check_plugin()|/"do_pre_register_check_plugin(%params)" >>>.
+
+=cut
+
+my sub pre_register_check_plugins : prototype() {
+    my @errs = ();
+
+    my @standard_plugins = get_indexed_plugins(is_custom => 0);
+    for my $plugin (@standard_plugins) {
+	eval {
+	    do_pre_register_check_plugin(name => $plugin);
+	};
+
+	push(@errs, $@) if $@;
+    }
+
+    if (scalar(@errs)) {
+	my $err_msg = "Encountered unexpected error while performing"
+	    . " pre-registration check for inbuilt plugins:\n";
+
+	confess($err_msg, join("\n", @errs), "\n\n... ");
+    }
+
+    my @custom_plugins = get_indexed_plugins(is_custom => 1);
+    for my $plugin (@custom_plugins) {
+	eval {
+	    do_pre_register_check_plugin(name => $plugin);
+	};
+
+	if ($@) {
+	    warn "Encountered unexpected error while performing pre-registration"
+		. " check for custom plugin \"$plugin\":\n$@\n";
+	    warn "Plugin will not be registered.\n";
+	}
+    }
+
+    return;
+}
+
+=head3 try_register_plugins()
+
+Attempts to register all plugins of the plugin index for which the
+L<pre-registration check|/"pre_register_check_plugins()"> was successful, with
+standard plugins being registered first and custom plugins afterwards.
+
+In detail, for each plugin, the C<import()> subroutine is first run, followed
+by C<L<< register()|PVE::SectionConfig/register >>>.
+
+For all standard plugins both invocations must succeed, otherwise an exception
+is thrown. If a call to either subroutine fails for a custom plugin, a warning
+is emitted instead and the custom plugin remains unregistered.
+
+=cut
+
+my sub try_register_plugins : prototype() {
+    my @errs = ();
+
+    my $plugin_index = $plugin_register_state->{index};
+
+    my $try_import = sub {
+	my ($plugin_name) = @_;
+
+	# so that we may call methods on $plugin_name
+	no strict 'refs'; ## no critic
+	$plugin_name->import();
+    };
+
+    my $try_register = sub {
+	my ($plugin_name) = @_;
+
+	# so that we may call methods on $plugin_name
+	no strict 'refs'; ## no critic
+	$plugin_name->register();
+
+	$plugin_index->{$plugin_name}->{is_registered} = 1;
+    };
+
+    my @standard_plugins = get_indexed_plugins(is_custom => 0, is_checked => 1);
+    for my $plugin (@standard_plugins) {
+	eval {
+	    $try_import->($plugin);
+	};
+
+	if ($@) {
+	    push(@errs, "Encountered unexpected error while running '$plugin\->import()' - $@");
+	    next;
+	}
+
+	eval {
+	    $try_register->($plugin);
+	};
+
+	if ($@) {
+	    push(@errs, "Encountered unexpected error while running '$plugin\->register()' - $@");
+	    next;
+	}
+    }
+
+    confess join("\n\n", @errs, "\n\n... ") if scalar(@errs);
+
+    my @custom_plugins = get_indexed_plugins(is_custom => 1, is_checked => 1);
+    for my $plugin (@custom_plugins) {
+	eval {
+	    $try_import->($plugin);
+	};
+
+	if ($@) {
+	    carp "Failed to run 'import' on custom plugin '$plugin' - $@";
+	    carp "Please check if the plugin is installed correctly.";
+	    carp "Continuing to register remaining custom plugins (if any).";
+
+	    next;
+	}
+
+	eval {
+	    $try_register->($plugin);
+	};
+
+	if ($@) {
+	    carp "Failed to register custom plugin '$plugin' - $@";
+	    carp "Please verify whether the plugin is installed correctly.";
+	    carp "Continuing to register remaining custom plugins (if any).";
+
+	    next;
+	}
+    }
+
+    return;
+}
+
+=head3 post_register_check_plugins()
+
+Used to perform various checks on each plugin after the plugins' registration.
+
+This subroutine currently only emits a warning if a custom plugin's API version
+is older than the current one.
+
+=cut
+
+my sub post_register_check_plugins : prototype() {
+    my @custom_plugins = get_indexed_plugins(is_custom => 1, is_registered => 1);
+    for my $plugin (@custom_plugins) {
+	# so that we may call methods on $plugin
+	no strict 'refs'; ## no critic
+
+	my $version = $plugin->api();
+	warn "Plugin \"$plugin\" is implementing an older storage API, an upgrade is recommended\n"
+	    if $version != APIVER;
+    }
+
+    return;
+}
+
+# NOTE: This is *not* like Exporter's import (that you get with e.g. `use parent qw(Exporter);`)
+# which supports @EXPORT_OK etc.
+#
+# If there is ever a need to support exporting subroutines (via `Exporter`),
+# see the following: https://perldoc.perl.org/Exporter#Exporting-Without-Using-Exporter's-import-Method
+# Beware that this requires you to remove any keys that ought not be passed
+# to exporter from @_.
+
+sub import {
+    my ($package, %params) = @_;
+
+    # this sub may be called more than once, so avoid registering plugins again
+    if ($plugin_register_state->{plugins_initialised} == 1) {
+	return;
+    }
+
+    try_load_plugins();
+    pre_register_check_plugins();
+    try_register_plugins();
+    post_register_check_plugins();
+
+    PVE::Storage::Plugin->init();
+    $plugin_register_state->{plugins_initialised} = 1;
+
+    return;
+}
+
 #  PVE::Storage utility functions
 
 sub config {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC v1 pve-storage 3/6] introduce compile-time checks for prohibited sub overrides

[Max Carrara]

Add a way to prohibit overriding certain subroutines ("symbols") at
"compile time" -- during Perl's BEGIN phase [1].

This is done by extending the previously introduced plugin loading
machinery with an additional check.

In essence, the new `prohibitOverrideAt` attribute handler can be
added to subroutines within `PVE::Storage::Plugin` together with a
version and a message. Each "tagged" sub's name is stored inside a
hash during the BEGIN phase [1], which the import machinery then uses
to check whether the subroutine is overridden in any of the plugins or
not.

This check will only fail if the specified API version was reached
(hence the name `prohibitOverrideAt`). A warning is emitted one
version prior. The APIAGE is also taken into account.

The reason for adding this is to make more drastic changes to the
storage plugin API visible and noticeable instead of relying on
comments that have to be checked manually by a developer. This not
only forces third-party plugin authors to adhere more strictly to our
API, but also makes it easier to keep our own plugins in check.

For example, when a method is expected to be removed, the attribute
can simply be added to it with the API version of its expected
removal. Once that API version is actually reached (while taking
APIAGE into account!) the import machinery's check will throw an
exception for each plugin that still overrides this method.

To keep the check available, the body of the method can be removed
instead of removing the method as a whole.

[1]: https://perldoc.perl.org/perlmod#BEGIN%2C-UNITCHECK%2C-CHECK%2C-INIT-and-END

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Suggested-by: Wolfang Bumiller <w.bumiller@proxmox.com>
---
 src/PVE/Storage.pm         |  19 +++
 src/PVE/Storage/Plugin.pm  | 133 +++++++++++++++++++
 src/PVE/Storage/Version.pm | 253 +++++++++++++++++++++++++++++++++++++
 3 files changed, 405 insertions(+)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 0c8ba64..a9568b1 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -302,6 +302,8 @@ plugin is a custom plugin, its API conformance is also verified:
 
 =item * Its API version must not be older than the storage API age allows
 
+=item * No deprecated or prohibited methods may be overridden
+
 =back
 
 =cut
@@ -343,9 +345,26 @@ my sub do_pre_register_check_plugin : prototype(%) {
 	    if $version < $min_version;
     };
 
+    my $check_symbols = sub {
+	my $prohibited_symbols = PVE::Storage::Plugin::get_prohibited_symbols();
+
+	PVE::Storage::Version::raise_on_prohibited_symbols(
+	    plugin => $plugin_name,
+	    plugin_parent => 'PVE::Storage::Plugin',
+	    prohibited_symbols => $prohibited_symbols,
+	);
+
+	PVE::Storage::Version::warn_on_soon_prohibited_symbols(
+	    plugin => $plugin_name,
+	    plugin_parent => 'PVE::Storage::Plugin',
+	    prohibited_symbols => $prohibited_symbols,
+	);
+    };
+
     eval {
 	$check_derives_base->();
 	$check_plugin_api->() if $is_custom;
+	$check_symbols->();
     };
 
     croak "Pre-registration check failed for plugin '$plugin_name' - $@" if $@;
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 65cf43f..bcc4eea 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -3,6 +3,8 @@ package PVE::Storage::Plugin;
 use strict;
 use warnings;
 
+use Attribute::Handlers;
+use Carp;
 use Cwd qw(abs_path);
 use Encode qw(decode);
 use Fcntl ':mode';
@@ -10,6 +12,7 @@ use File::chdir;
 use File::Path;
 use File::Basename;
 use File::stat qw();
+use Scalar::Util qw(reftype);
 
 use PVE::Tools qw(run_command);
 use PVE::JSONSchema qw(get_standard_option register_standard_option);
@@ -27,6 +30,136 @@ use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
 use constant LOG_EXT => ".log";
 use constant NOTES_EXT => ".notes";
 
+=head1 ATTRIBUTES
+
+=cut
+
+my $override_prohibited_methods;
+
+# necessary so that the hash becomes available as early as possible
+BEGIN {
+    $override_prohibited_methods = {};
+}
+
+=head3 prohibitOverrideAt(...)
+
+An attribute handler used to I<prohibit> a subroutine ("symbol") from being
+overridden from a particular API C<version> onwards. The handler also requires
+a C<message> that explains why the subroutine may not be overridden.
+
+    sub some_subroutine : prohibitOverrideAt(
+	version => 42,
+	message => "Overriding this subroutine in child plugins leads to unexpected behaviour.",
+    ) {
+	# ...
+    }
+
+This handler stores all prohibited symbols. You may return a nested hash of all
+symbols using C<L<< get_prohibited_symbols()|/"get_prohibited_symbols()" >>>.
+
+=cut
+
+sub prohibitOverrideAt : ATTR(CODE,BEGIN) {
+    my ($package, $symbol, $referent, $attr_name, $attr_data, $phase, $filename, $line) = @_;
+
+    confess "'$attr_name' attribute may only be used inside the PVE::Storage::Plugin module"
+	if $package ne 'PVE::Storage::Plugin';
+
+    my $data = {};
+    $data = { $attr_data->@* } if defined($attr_data);
+
+    my ($version, $message) = ($data->{version}, $data->{message});
+    my $reftype = reftype($referent);
+
+    confess "Referent is not a reference"
+	. " -- '$attr_name' attribute may only be used for subroutines"
+	if !defined($reftype);
+
+    confess "Unexpected reftype '$reftype'"
+	. " -- '$attr_name' attribute may only be used for subroutines"
+	if !($reftype eq 'CODE' || $reftype eq 'ANON');
+
+    confess "Either version or message not defined"
+	. " -- usage: $attr_name(version => 42, message => \"...\")"
+	if !defined($version) || !defined($message);
+
+    my $symbol_name = *{$symbol}{"NAME"};
+
+    confess "Symbol name is undef in $filename line $line\n"
+	if !defined($symbol_name);
+
+    confess "Symbol name already exists in tracked symbols in $filename line $line\n"
+	if exists($override_prohibited_methods->{$symbol_name});
+
+    $override_prohibited_methods->{$symbol_name} = {
+	version => $version,
+	message => $message,
+	package => $package,
+	symbol => $symbol,
+	referent => $referent,
+	reftype => $reftype,
+	attr_name => $attr_name,
+	attr_data => $attr_data,
+	phase => $phase,
+	filename => $filename,
+	line => $line,
+    };
+
+    return;
+}
+
+=head1 HELPERS
+
+=head3 get_prohibited_symbols()
+
+Returns a shallow copy of the nested hash that contains all of the symbols
+that were marked as I<prohibited> by the C<L<< prohibitOverrideAt|/prohibitOverrideAt(...) >>> attribute.
+
+The C<version> and C<message> keys that were passed to the attribute handler
+are passed along to the nested hash:
+
+    {
+	symbol_name => {
+	    version => 10,
+	    message => '...',
+	    # ...
+	},
+	# ...
+    }
+
+The remaining keys are taken from the attribute handler's parameters and are
+passed along for e.g. better error messaging:
+
+    {
+	symbol_name => {
+	    version => 10,
+	    message => '...',
+	    package => '...',         # the name of the package in which the attribute was invoked
+	    symbol => $symbol,        # reference to the symbol table entry (typeglob)
+	    referent => $referent,    # reference to the actual symbol (subroutine)
+	    reftype => $reftype,      # the type of the 'referent', usually 'CODE'
+	    attr_name => '...',       # the name of the attribute that created this hash, usually 'prohibitOverrideAt'
+	    attr_data => $attr_data,  # the data that was passed to the attribute
+	    phase => 'BEGIN',         # in which phase the attribute was invoked in, usually 'BEGIN' or 'CHECK'
+	    filename => '...',        # the filename in which the attribute was invoked
+	    line => $line,            # the line on which the attribute was invoked
+	},
+	# ...
+    }
+
+=cut
+
+sub get_prohibited_symbols : prohibitOverrideAt(
+    version => 0,
+    message => "This subroutine is for internal use only."
+) {
+    # shallow copy
+    return {
+	map { $_, $override_prohibited_methods->{$_} }
+	keys $override_prohibited_methods->%*
+    };
+}
+
 our @COMMON_TAR_FLAGS = qw(
     --one-file-system
     -p --sparse --numeric-owner --acls
diff --git a/src/PVE/Storage/Version.pm b/src/PVE/Storage/Version.pm
index a9216c9..5efb185 100644
--- a/src/PVE/Storage/Version.pm
+++ b/src/PVE/Storage/Version.pm
@@ -176,5 +176,258 @@ sub UNIVERSAL::pveStorageDeprecateAt : ATTR(CODE,CHECK) {
     return;
 }
 
+=head1 FUNCTIONS
+
+=head2 Dealing With Prohibited Symbols
+
+When a symbol of a storage plugin is considered I<prohibited>, it means that no
+I<child plugin> may override or extend the symbol.
+
+In other words, it prevents plugins from overriding methods and helpers they
+shouldn't override to begin with.
+
+Currently, the base storage plugin C<L<PVE::Storage::Plugin>> is the only such
+plugin that defines prohibited symbols. These are returned as a nested hash via
+C<L<< get_prohibited_symbols()|PVE::Storage::Plugin/get_prohibited_symbols() >>>,
+which is used by functions like C<L<< find_overridden_prohibited_symbols()|/"find_overridden_prohibited_symbols(%params)" >>>
+and C<L<< raise_on_prohibited_symbols()|/"raise_on_prohibited_symbols(%params)" >>>.
+
+For details on how this hash is constructed, see: C<L<< prohibitOverrideAt()|PVE::Storage::Plugin/prohibitOverrideAt(...) >>>.
+
+=cut
+
+my sub get_plugin_symbols : prototype($$) {
+    my ($plugin, $plugin_parent) = @_;
+
+    my $is_derived_from_parent = eval {
+	no strict 'refs'; ## no critic
+	$plugin->isa($plugin_parent);
+    };
+
+    confess $@ if $@;
+    confess "'$plugin' does not derive from '$plugin_parent'" if !$is_derived_from_parent;
+
+    my $plugin_symbols = eval {
+	my $mod_name = $plugin . '::';
+
+	no strict 'refs'; ## no critic
+
+	# shallow copy
+	return { map { $_, $mod_name->{$_} } keys $mod_name->%* };
+    };
+
+    confess $@ if $@;
+
+    return $plugin_symbols;
+}
+
+=head3 find_overridden_prohibited_symbols(%params)
+
+Returns an array or a reference to an array of the names of symbols that were
+overridden by the given plugin despite being prohibited.
+
+    my @symbols = find_overridden_prohibited_symbols(
+	plugin => 'PVE::Storage::SomePlugin',
+	plugin_parent => 'PVE::Storage::Plugin',
+	prohibited_symbols => $prohibited_symbols,
+    );
+
+=cut
+
+sub find_overridden_prohibited_symbols : prototype(%) {
+    my (%params) = @_;
+
+    my $plugin = $params{plugin}
+	or confess "'plugin' key not provided";
+    my $plugin_parent = $params{plugin_parent}
+	or confess "'plugin_parent' key not provided";
+    my $prohibited_symbols = $params{prohibited_symbols}
+	or confess "'prohibited_symbols' key not provided";
+
+    my $plugin_symbols = get_plugin_symbols($plugin, $plugin_parent);
+
+    my @found_symbols = ();
+
+    for my $symbol (keys $plugin_symbols->%*) {
+	next if !defined($prohibited_symbols->{$symbol});
+
+	my $refers_to_same_as_parent = eval {
+	    no strict 'refs'; ## no critic
+
+	    my $can_plugin = $plugin->UNIVERSAL::can($symbol);
+	    my $can_parent = $plugin_parent->UNIVERSAL::can($symbol);
+
+	    defined($can_plugin) && defined($can_parent) && ($can_plugin == $can_parent)
+	};
+
+	push(@found_symbols, $symbol) if !$refers_to_same_as_parent;
+    }
+
+    return @found_symbols if wantarray;
+    return \@found_symbols;
+}
+
+=head3 raise_on_prohibited_symbols(%params)
+
+Raises an exception if the given C<plugin> overrides or extends any of the
+C<prohibited_symbols> of the C<plugin_parent>.
+
+    raise_on_prohibited_symbols(
+	plugin => 'PVE::Storage::SomePlugin',
+	plugin_parent => 'PVE::Storage::Plugin',
+	prohibited_symbols => $prohibited_symbols,
+    );
+
+If multiple prohibited symbols are overridden, the exception will consist of
+multiple messages instead of one.
+
+=cut
+
+sub raise_on_prohibited_symbols : prototype(%) {
+    my (%params) = @_;
+
+    my $plugin = $params{plugin}
+	or confess "'plugin' key not provided";
+    my $plugin_parent = $params{plugin_parent}
+	or confess "'plugin_parent' key not provided";
+    my $prohibited_symbols = $params{prohibited_symbols}
+	or confess "'prohibited_symbols' key not provided";
+
+    my @overridden_symbols = find_overridden_prohibited_symbols(
+	plugin => $plugin,
+	plugin_parent => $plugin_parent,
+	prohibited_symbols => $prohibited_symbols,
+    );
+
+    my @errs = ();
+
+    for my $symbol (@overridden_symbols) {
+	# sanity check
+	confess "expected overridden symbol to be prohibited"
+	    if !defined($prohibited_symbols->{$symbol});
+
+	my $symbol_hash = $prohibited_symbols->{$symbol};
+
+	my $version = $symbol_hash->{version};
+
+	my $is_version_too_old = ($version // -1) <= $MIN_VERSION;
+
+	next if !$is_version_too_old;
+
+	# error message is formed in a way that uses as much information as
+	# possible, since keys of $symbol_hash aren't strictly required to be
+	# defined
+
+	my $err_msg = "$plugin defines prohibited symbol \"$symbol\"";
+
+	my ($message, $filename, $reftype, $line) = (
+	    $symbol_hash->{message},
+	    $symbol_hash->{filename},
+	    $symbol_hash->{reftype},
+	    $symbol_hash->{line},
+	);
+
+	if (defined($version)) {
+	    $err_msg .= " - not allowed since version $version"
+		. " (API version: @{[APIVER]}, minimum supported API version: $MIN_VERSION)";
+	}
+
+	$err_msg .= ": $message" if defined($message);
+
+	if (defined($filename)) {
+	    $err_msg .= "\nOriginally defined in $filename";
+
+	    $err_msg .= " as reftype '$reftype'" if defined($reftype);
+	    $err_msg .= " at line $line" if defined($line);
+	}
+
+	push(@errs, $err_msg);
+    }
+
+    croak(join("\n", @errs), "\n") if scalar(@errs);
+
+    return;
+}
+
+=head3 warn_on_soon_prohibited_symbols(%params)
+
+Emits a warning for each of the C<prohibited_symbols> of the C<plugin_parent>
+that the given C<plugin> overrides, if the symbol will soon be prohibited
+(e.g. when APIAGE is reset and/or APIVER is bumped).
+
+    warn_on_soon_prohibited_symbols(
+	plugin => 'PVE::Storage::SomePlugin',
+	plugin_parent => 'PVE::Storage::Plugin',
+	prohibited_symbols => $prohibited_symbols,
+    );
+
+=cut
+
+sub warn_on_soon_prohibited_symbols : prototype(%) {
+    my (%params) = @_;
+
+    my $plugin = $params{plugin}
+	or confess "'plugin' key not provided";
+    my $plugin_parent = $params{plugin_parent}
+	or confess "'plugin_parent' key not provided";
+    my $prohibited_symbols = $params{prohibited_symbols}
+	or confess "'prohibited_symbols' key not provided";
+
+    # How many versions to warn beforehand
+    my $warn_version_offset = 1;
+
+    my @overridden_symbols = find_overridden_prohibited_symbols(
+	plugin => $plugin,
+	plugin_parent => $plugin_parent,
+	prohibited_symbols => $prohibited_symbols,
+    );
+
+    for my $symbol (@overridden_symbols) {
+	# sanity check
+	confess "expected overridden symbol to be prohibited"
+	    if !defined($prohibited_symbols->{$symbol});
+
+	my $symbol_hash = $prohibited_symbols->{$symbol};
+
+	my $version = $symbol_hash->{version};
+
+	next if !defined($version);
+
+	my $is_version_old = $version >= $MIN_VERSION
+	    && $version <= $MIN_VERSION + $warn_version_offset;
+
+	next if !$is_version_old;
+
+	# warning is formed in a way that uses as much information as possible,
+	# since keys of $symbol_hash aren't strictly required to be defined
+
+	my $warning = "\"$plugin\" defines symbol \"$symbol\"";
+
+	my ($message, $filename, $reftype, $line) = (
+	    $symbol_hash->{message},
+	    $symbol_hash->{filename},
+	    $symbol_hash->{reftype},
+	    $symbol_hash->{line},
+	);
+
+	if (defined($version)) {
+	    $warning .= " which may not be used anymore from API version $version onward"
+		. " (API version: @{[APIVER]}, minimum supported API version: $MIN_VERSION)";
+	}
+
+	$warning .= ": $message" if defined($message);
+
+	if (defined($filename)) {
+	    $warning .= "\nOriginally defined in $filename";
+
+	    $warning .= " as reftype '$reftype'" if defined($reftype);
+	    $warning .= " at line $line" if defined($line);
+	}
+
+	warn $warning, "\n\n";
+    }
+
+    return;
+}
 
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC v1 pve-storage 4/6] plugin: replace fixme comments for deprecated methods with attribute

[Max Carrara]

This makes it so that the `get_volume_notes` and `update_volume_notes`
methods use the previously introduced `prohibitOverrideAt` attribute
handler instead of relying on FIXME comments.

Because the comments originally said to "remove on the next APIAGE
reset", the `version` for the two attributes is set to 10 (the current
APIVER). This means that compilation will fail as soon as APIAGE is
reset to 0 (or APIVER is bumped without bumping APIAGE).

This can be tested by running `make test` inside the `src/` directory
of the repository. Without changing the APIAGE, warnings will be
emitted. Setting the APIAGE to 0 and then running `make test` again
will throw an exception before running any other code.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/CIFSPlugin.pm   |  4 ----
 src/PVE/Storage/CephFSPlugin.pm |  4 ----
 src/PVE/Storage/DirPlugin.pm    |  5 +----
 src/PVE/Storage/NFSPlugin.pm    |  4 ----
 src/PVE/Storage/PBSPlugin.pm    |  4 ----
 src/PVE/Storage/Plugin.pm       | 14 ++++++++------
 6 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/src/PVE/Storage/CIFSPlugin.pm b/src/PVE/Storage/CIFSPlugin.pm
index 475065a..89a9981 100644
--- a/src/PVE/Storage/CIFSPlugin.pm
+++ b/src/PVE/Storage/CIFSPlugin.pm
@@ -292,15 +292,11 @@ sub check_connection {
     return 1;
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::get_volume_notes($class, @_);
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::update_volume_notes($class, @_);
diff --git a/src/PVE/Storage/CephFSPlugin.pm b/src/PVE/Storage/CephFSPlugin.pm
index 36c64ea..934ba8b 100644
--- a/src/PVE/Storage/CephFSPlugin.pm
+++ b/src/PVE/Storage/CephFSPlugin.pm
@@ -239,15 +239,11 @@ sub deactivate_storage {
     }
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::get_volume_notes($class, @_);
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::update_volume_notes($class, @_);
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index fb23e0a..79c7a94 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -9,6 +9,7 @@ use File::Path;
 use IO::File;
 use POSIX;
 
+use PVE::Storage::Version;
 use PVE::Storage::Plugin;
 use PVE::GuestImport::OVF;
 use PVE::JSONSchema qw(get_standard_option);
@@ -127,8 +128,6 @@ my $get_volume_notes_impl = sub {
     return '';
 };
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
     return $get_volume_notes_impl->($class, $scfg, $storeid, $volname, $timeout);
@@ -153,8 +152,6 @@ my $update_volume_notes_impl = sub {
     return;
 };
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
     return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $notes, $timeout);
diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
index 72e9c6d..0dfdc95 100644
--- a/src/PVE/Storage/NFSPlugin.pm
+++ b/src/PVE/Storage/NFSPlugin.pm
@@ -201,15 +201,11 @@ sub check_connection {
     return 1;
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::get_volume_notes($class, @_);
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::update_volume_notes($class, @_);
diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 0808bcc..f0f1b06 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -850,8 +850,6 @@ sub deactivate_volume {
     return 1;
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
@@ -862,8 +860,6 @@ sub get_volume_notes {
     return $data->{notes};
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
 
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index bcc4eea..0c80859 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1217,17 +1217,19 @@ sub file_size_info {
     return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use get_volume_attribute instead.
-sub get_volume_notes {
+sub get_volume_notes : prohibitOverrideAt(
+    version => 10,
+    message => "Deprecated, use get_volume_attribute instead."
+) {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
     die "volume notes are not supported for $class";
 }
 
-# FIXME remove on the next APIAGE reset.
-# Deprecated, use update_volume_attribute instead.
-sub update_volume_notes {
+sub update_volume_notes : prohibitOverrideAt(
+    version => 10,
+    message => "Deprecated, use update_volume_attribute instead.",
+) {
     my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
 
     die "volume notes are not supported for $class";
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC v1 pve-storage 5/6] introduce check for `type` method and duplicate types

[Max Carrara]

Check whether a plugin defines a `type` method (as required by
PVE::SectionConfig). Throw and exception if it doesn't.

Also check for the same type being declared more than once.

All this is added to make implementing a plugin a little more
developer-friendly through more expressive error messages. Both cases
are otherwise also handled by the `register` method of
PVE::SectionConfig.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage.pm | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index a9568b1..4cf591f 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -312,6 +312,7 @@ my sub do_pre_register_check_plugin : prototype(%) {
     my (%params) = @_;
 
     my $plugin_name = $params{name};
+    my $plugin_types = $params{plugin_types};
 
     my $plugin_index = $plugin_register_state->{index};
 
@@ -345,6 +346,26 @@ my sub do_pre_register_check_plugin : prototype(%) {
 	    if $version < $min_version;
     };
 
+    my $check_plugin_type = sub {
+	# so that we may call methods on $plugin_name
+	no strict 'refs'; ## no critic
+
+	# Method is inherited, no need to check for its existence.
+	# However, if not overridden, it will `die`
+	my $type = eval {
+	    $plugin_name->type()
+	};
+
+	die "plugin does not override \"type()\" method\n"
+	    if $@;
+
+	my $source_plugin = $plugin_types->{$type};
+	die "plugin with type \"$type\" already exists: $source_plugin\n"
+	    if defined($source_plugin);
+
+	$plugin_types->{$type} = $plugin_name;
+    };
+
     my $check_symbols = sub {
 	my $prohibited_symbols = PVE::Storage::Plugin::get_prohibited_symbols();
 
@@ -364,6 +385,7 @@ my sub do_pre_register_check_plugin : prototype(%) {
     eval {
 	$check_derives_base->();
 	$check_plugin_api->() if $is_custom;
+	$check_plugin_type->();
 	$check_symbols->();
     };
 
@@ -389,10 +411,13 @@ C<L<< do_pre_register_check_plugin()|/"do_pre_register_check_plugin(%params)" >>
 my sub pre_register_check_plugins : prototype() {
     my @errs = ();
 
+    # { type() => plugin_name }
+    my $plugin_types = {};
+
     my @standard_plugins = get_indexed_plugins(is_custom => 0);
     for my $plugin (@standard_plugins) {
 	eval {
-	    do_pre_register_check_plugin(name => $plugin);
+	    do_pre_register_check_plugin(name => $plugin, plugin_types => $plugin_types);
 	};
 
 	push(@errs, $@) if $@;
@@ -408,7 +433,7 @@ my sub pre_register_check_plugins : prototype() {
     my @custom_plugins = get_indexed_plugins(is_custom => 1);
     for my $plugin (@custom_plugins) {
 	eval {
-	    do_pre_register_check_plugin(name => $plugin);
+	    do_pre_register_check_plugin(name => $plugin, plugin_types => $plugin_types);
 	};
 
 	if ($@) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC v1 pve-storage 6/6] introduce check for duplicate plugin properties

[Max Carrara]

For each property that a plugin defines, check if the property is
already defined by another plugin before registering plugins.

This is mostly done for the sake of developer-friendliness. The same
check is made in `PVE::SectionConfig::init()` -- the difference here
is that the plugin which defined the property is preserved in the
error message.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage.pm | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 4cf591f..55a8668 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -313,6 +313,7 @@ my sub do_pre_register_check_plugin : prototype(%) {
 
     my $plugin_name = $params{name};
     my $plugin_types = $params{plugin_types};
+    my $plugin_properties = $params{plugin_properties};
 
     my $plugin_index = $plugin_register_state->{index};
 
@@ -366,6 +367,25 @@ my sub do_pre_register_check_plugin : prototype(%) {
 	$plugin_types->{$type} = $plugin_name;
     };
 
+    my $check_plugin_properties = sub {
+	# so that we may call methods on $plugin_name
+	no strict 'refs'; ## no critic
+
+	# method is inherited and doesn't throw
+	my $properties = $plugin_name->properties();
+
+	my @errs = ();
+	for my $property (keys $properties->%*) {
+	    my $source_plugin = $plugin_properties->{$property};
+	    push(@errs, "property \"$property\" already exists - defined by $source_plugin")
+		if defined($source_plugin);
+
+	    $plugin_properties->{$property} = $plugin_name;
+	}
+
+	die(join("\n", @errs), "\n") if scalar(@errs);
+    };
+
     my $check_symbols = sub {
 	my $prohibited_symbols = PVE::Storage::Plugin::get_prohibited_symbols();
 
@@ -386,6 +406,7 @@ my sub do_pre_register_check_plugin : prototype(%) {
 	$check_derives_base->();
 	$check_plugin_api->() if $is_custom;
 	$check_plugin_type->();
+	$check_plugin_properties->();
 	$check_symbols->();
     };
 
@@ -414,10 +435,17 @@ my sub pre_register_check_plugins : prototype() {
     # { type() => plugin_name }
     my $plugin_types = {};
 
+    # { property_name => plugin_name }
+    my $plugin_properties = {};
+
     my @standard_plugins = get_indexed_plugins(is_custom => 0);
     for my $plugin (@standard_plugins) {
 	eval {
-	    do_pre_register_check_plugin(name => $plugin, plugin_types => $plugin_types);
+	    do_pre_register_check_plugin(
+		name => $plugin,
+		plugin_types => $plugin_types,
+		plugin_properties => $plugin_properties,
+	    );
 	};
 
 	push(@errs, $@) if $@;
@@ -433,7 +461,11 @@ my sub pre_register_check_plugins : prototype() {
     my @custom_plugins = get_indexed_plugins(is_custom => 1);
     for my $plugin (@custom_plugins) {
 	eval {
-	    do_pre_register_check_plugin(name => $plugin, plugin_types => $plugin_types);
+	    do_pre_register_check_plugin(
+		name => $plugin,
+		plugin_types => $plugin_types,
+		plugin_properties => $plugin_properties,
+	    );
 	};
 
 	if ($@) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Wolfgang Bumiller]

Overall thoughts - this is a bit much at once...

1) It doesn't build. The loader fails, claiming that `DirPlugin` is not
derived from `PVE::Storage::Plugin`. Presumably because you only
`require` it which does not set up the `@ISA`...
(Followed by a bunch of "Plugin 'foo' is already loaded" errors)

2) I don't think we really need/want to rework the loading this much.
We don't really gain much.

Using a plugin *list* instead of manually having a `use AllPlugins;`
and `AllPlugins->register()` blocks might be worth it, but this is too
much.

But we definitely don't need a `wantarray`-using `sub` returning a list
of standard plugins.
Just use `my/our @PLUGINS = qw(The List);` please.

3) Apparently our style guide needs updating. While some things aren't
explicit in there, they should be:
Eg. you have a *lot* of postfix-`if` on multi-line expressions which
should rather be regular prefix-if blocks.

Also: `pveStorageDeprecateAt` - our style guide says `snake_case` for
everything other than package names.

4) You POD-document private `my sub`s. This does not fit our current
code and I'm not convinced it is really worth it, but then there's a
lot of code there I think could just be dropped (eg. the
`get_standard_plugin_names()` sub is documented with POD while it
should just be a list, while right above it there's a
`$plugin_register_state` with not even a comment explaining its
contents.

5) Generally: Please stop using `wantarray` where it has no actual
benefit. A private `my sub` used exactly once really does not need
this.


On Thu, Jan 30, 2025 at 03:51:18PM +0100, Max Carrara wrote:
> RFC: Tighter API Control for Storage Plugins - v1
> =================================================
> 
> Since this has been cooking for a while I've decided to send this in as
> an RFC in order to get some early feedback in.
> 
> Note that this is quite experimental and also a little more complex;
> I'll try my best to explain my reasoning for the changes in this RFC
> below.
> 
> Any feedback on this is greatly appreciated!
> 
> Introduction
> ------------
> 
> While working on a refactor of the Storage Plugin API, I've often hit
> cases where I needed to move a subroutine around, but couldn't
> confidently do so due to the code being rather old and the nature of
> Perl as a language.
> 
> I had instances where things would spontaneously break here and there
> after moving an inocuous-looking helper subroutine, due to it actually
> being used in a different module / plugin, or worse, in certain cases in
> a completely different package, because a plugin's helper sub ended
> up being spontaneously used there.
> 
> To remedy this, I had originally added a helper subroutine for emitting
> deprecation warnings. The plan back then was to leave the subroutines at
> their original locations and have them call their replacement under the
> hood while emitting a warning. Kind of like so:
> 
>   # PVE::Storage::SomePlugin
> 
>   sub some_helper_sub {
>       my ($arg) = @_;
> 
>       # Emit deprecation warning here to hopefully catch any unexpected
>       # callsites
>       warn get_deprecation_warning(
>           "PVE::Storage::Common::some_helper_sub"
>       );
> 
>       # Call relocated helper here
>       return PVE::Storage::Common::some_helper_sub($arg);
>   }
> 
> This approach was decent-ish, but didn't *actually* guard against any
> mishaps, unfortunately. With this approach, there is no guarantee that
> the callsite will actually be caught, especially if e.g. a third-party
> storage plugin was also using that subroutine and the plugin author
> didn't bother checking or reading their CI logs.
> 
> What I instead wanted to have was some "strong guarantee" that a
> subroutine absolutely cannot be used anymore after a certain point, e.g.
> after a certain API version or similar.

I don't think accidentally-public private helpers should be considered
part of the API. We can just deprecate them immediately, remove them
"soon". They aren't part of the `PVE::Storage`<->`Plugin` surface after
all.

They may be using a private sub in `PVE::QemuServer` too - an attribute
to warn on that (eg. just check `caller` against `__PACKAGE__`) should
be "good enough" - a compile time solution would be way too much code.
We don't need to write our own linters here.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 1/6] version: introduce PVE::Storage::Version

[Wolfgang Bumiller]

On Thu, Jan 30, 2025 at 03:51:19PM +0100, Max Carrara wrote:
> The purpose of this module is to separate the handling of the Storage
> API's version from the remaining code while also providing additional
> mechanisms to handle changes to the Storage API more gracefully.
> 
> The first such mechanism is the `pveStorageDeprecateAt` attribute,
> which, when added to a subroutine, marks the subroutine as deprecated
> at a specific version. Should the lowest supported API version
> increase beyond the version passed to the attribute, the module will
> fail to compile (during Perl's CHECK phase).
> 
> This provides a more robust mechanism instead of having to rely on
> "FIXME" comments or similar.
> 
> Unfortunately attributes can't conveniently be exported using the
> `Exporter` module or similar mechanisms, and they can neither be
> referenced via their fully qualified path. The attribute's name is
> therefore intentionally made long in order to not conflict with any
> other possible attributes in the UNIVERSAL namespace.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/Storage.pm         |  10 +--
>  src/PVE/Storage/Makefile   |   1 +
>  src/PVE/Storage/Version.pm | 180 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 185 insertions(+), 6 deletions(-)
>  create mode 100644 src/PVE/Storage/Version.pm
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 3b4f041..df4d62f 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -24,6 +24,8 @@ use PVE::RPCEnvironment;
>  use PVE::SSHInfo;
>  use PVE::RESTEnvironment qw(log_warn);
>  
> +use PVE::Storage::Version;
> +
>  use PVE::Storage::Plugin;
>  use PVE::Storage::DirPlugin;
>  use PVE::Storage::LVMPlugin;
> @@ -41,12 +43,8 @@ use PVE::Storage::PBSPlugin;
>  use PVE::Storage::BTRFSPlugin;
>  use PVE::Storage::ESXiPlugin;
>  
> -# Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 10;
> -# 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 APIVER => PVE::Storage::Version::APIVER;
> +use constant APIAGE => PVE::Storage::Version::APIAGE;
>  
>  our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
>  
> diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
> index ce3fd68..5315f69 100644
> --- a/src/PVE/Storage/Makefile
> +++ b/src/PVE/Storage/Makefile
> @@ -1,5 +1,6 @@
>  SOURCES= \
>  	Common.pm \
> +	Version.pm \
>  	Plugin.pm \
>  	DirPlugin.pm \
>  	LVMPlugin.pm \
> diff --git a/src/PVE/Storage/Version.pm b/src/PVE/Storage/Version.pm
> new file mode 100644
> index 0000000..a9216c9
> --- /dev/null
> +++ b/src/PVE/Storage/Version.pm
> @@ -0,0 +1,180 @@
> +=head1 NAME
> +
> +C<PVE::Storage::Version> - Storage API Version Management
> +
> +=head1 DESCRIPTION
> +
> +This module is concerned with tracking and managing the version of the Storage
> +API interface C<L<PVE::Storage::Plugin>>. Furthermore, this module also provides
> +a mechanism to I<deprecate> subroutines that are part of the Storage API via a
> +custom I<attribute> called C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>>.
> +
> +For more information on this mechanism, see L</"DEPRECATING SUBROUTINES"> below.
> +
> +B<NOTE:> Importing this module will define the C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>>
> +attribute inside the C<UNIVERSAL> (global) namespace. Ensure that no other
> +symbols have the same name as this attribute in order to avoid conflicts.
> +
> +=cut
> +
> +package PVE::Storage::Version;
> +
> +use strict;
> +use warnings;
> +
> +use v5.36;

^  activates signatures, use them.

> +
> +use Attribute::Handlers;
> +use Carp;
> +use Scalar::Util qw(reftype);
> +
> +# NOTE: This module must not import PVE::Storage or any of its submodules,
> +# either directly or transitively.
> +#
> +# Otherwise there's a risk of introducing nasty cyclical imports and/or causing
> +# obscure errors during compile time, which may be very hard to debug.
> +
> +=head1 CONSTANTS
> +
> +=head3 APIVER
> +
> +The version of the Storage API.
> +
> +This version must be incremented if changes to the C<L<PVE::Storage::Plugin>>
> +API interface are made.
> +
> +=head3 APIAGE
> +
> +The API age is the number of versions we're backward compatible with.
> +
> +This is similar to having C<current='APIVER'> and C<age='APIAGE'> in C<libtool>.
> +For more information, see L<here|https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html>.
> +
> +Generally, you'll want to increment this when incrementing C<L<< APIVER|/APIVER >>>.
> +Resetting C<APIAGE> to C<0> means that backward compatibility is broken and should
> +only be done when appropriate (e.g. during a release of a new major version of PVE).
> +
> +=cut
> +
> +use constant APIVER => 10;
> +use constant APIAGE => 1;
> +
> +my $MIN_VERSION = (APIVER - APIAGE);
> +
> +
> +=head1 DEPRECATING SUBROUTINES
> +
> +Upon importing this module, the C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>>
> +attribute becomes available. This attribute ought to be used similar to how
> +prototypes are used in Perl and requires a C<version> and a C<message> key to
> +be passed:
> +
> +    sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") {
> +	# [...]
> +    }
> +
> +Most likely you'll need more space for the C<message>, in which case the
> +attribute may simply span multiple lines:
> +
> +    sub some_sub : pveStorageDeprecateAt(
> +	version => 42,
> +	message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
> +	    . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium"
> +	    . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl"
> +	    . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel"
> +	    . " suscipit lorem varius. Mauris et erat posuere, gravida dui"
> +	    . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet,"
> +	    . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id"
> +	    . " diam venenatis, nec placerat felis viverra.",
> +    ) {
> +	# [...]
> +    }

One example with "..." as message should be good enough.

> +
> +As soon as the passed C<version> is below the I<lowest supported version>
> +(C<L<< APIVER|/APIVER >>> minus C<L<< APIAGE|/APIAGE >>>), the attribute raises
> +an exception during the C<CHECK> phase of the Perl interpreter and also displays
> +the passed C<message>. This happens before any runtime code is executed.
> +
> +Should the passed C<version> be equal to the lowest supported version, a warning
> +containing the passed C<message> is emitted whenever the subroutine is called
> +instead. This ensures that implementors of custom plugins are made aware of
> +subroutines being deprecated, should there be any in use.
> +
> +The C<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>> attribute
> +may only be used inside C<L<PVE::Storage>> or its submodules and throws an error
> +if used elsewhere.
> +
> +This mechanism's sole purpose is to enforce deprecation of subroutines during
> +"compile time", forcing the subroutine to be removed once the API version is
> +incremented too far or the API age is reset.
> +
> +=head1 ATTRIBUTES
> +
> +=head3 UNIVERSAL::pveStorageDeprecateAt
> +
> +This attribute marks subroutines as deprecated at a specific C<version> and will
> +emit an error with the given C<message> if the lowest supported Storage API
> +version exceeds the C<version> passed to the attribute.
> +
> +    sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") {
> +	# [...]
> +    }
> +
> +    sub some_sub : pveStorageDeprecateAt(
> +	version => 42,
> +	message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
> +	    . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium"
> +	    . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl"
> +	    . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel"
> +	    . " suscipit lorem varius. Mauris et erat posuere, gravida dui"
> +	    . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet,"
> +	    . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id"
> +	    . " diam venenatis, nec placerat felis viverra.",
> +    ) {
> +	# [...]
> +    }
> +
> +Should the passed C<version> be equal to the currently lowest supported API
> +version, a warning containing the passed C<message> is emitted instead.
> +
> +=cut
> +
> +sub UNIVERSAL::pveStorageDeprecateAt : ATTR(CODE,CHECK) {
> +    my ($package, $symbol, $referent, $attr_name, $attr_data, $phase, $filename, $line) = @_;
> +
> +    confess "'$attr_name' attribute may only be used inside PVE::Storage and its submodules"
> +	if $package !~ m/^PVE::Storage/;

(Could end in `($|:)`, otherwise this allows `PVE::StorageNotReally`)

> +
> +    my $data = { $attr_data->@* };
> +
> +    my ($version, $message) = ($data->{version}, $data->{message});
> +    my $reftype = reftype($referent);
> +
> +    confess "Referent is not a reference"
> +	. " -- '$attr_name' attribute may only be used for subroutines"
> +	if !defined($reftype);

^ Style issues mentioned in my cover letter reply.

> +
> +    confess "Unexpected reftype '$reftype'"
> +	. " -- '$attr_name' attribute may only be used for subroutines"
> +	if !($reftype eq 'CODE' || $reftype eq 'ANON');
> +
> +    confess "Either version or message not defined"
> +	. " -- usage: $attr_name(version => 42, message => \"...\")"
> +	if !defined($version) || !defined($message);
> +
> +    confess "Storage API version '$version' not supported: $message"
> +	if $version < $MIN_VERSION;
> +
> +    no warnings 'redefine';
> +    *$symbol = sub {
> +	my $symbol_name = *{$symbol}{"NAME"};
> +	carp "Warning: subroutine '$symbol_name' is deprecated - $message"
> +	    if $version == $MIN_VERSION;
> +	$referent->(@_);
> +    };
> +
> +    return;
> +}
> +
> +
> +1;
> -- 
> 2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 2/6] rework plugin loading and registration mechanism

[Wolfgang Bumiller]

On Thu, Jan 30, 2025 at 03:51:20PM +0100, Max Carrara wrote:
> This commit changes how plugins, both inbuilt and custom, are loaded
> and registered by the `PVE::Storage` module from the ground up.
> 
> This is done for mainly two reasons:
> 
>  1. Unifying the plugin loading mechanism so that inbuilt and custom
>     plugins are subjected to (almost) the same checks.
> 
>  2. Making it possible to easily extend the loading / registering
>     mechanism for additional compile-time (BEGIN phase [1]) checks.
> 
> The new plugin registration mechanism essentially consists of four
> stages:
> 
>  1. Load plugins into the Perl interpreter
>  2. Perform pre-registration checks
>  3. Registration
>  4. Perform post-registration checks
> 
> Each stage will perform its specific actions on inbuilt plugins first
> and on custom plugins afterwards.
> 
> In general, should a stage fail for an inbuilt plugin, an exception is
> thrown. For custom plugins a warning is emitted instead and the plugin
> won't be considered in the subsequent stage(s).
> 
> The new registration mechanism is equivalent to the old one, with a
> few minor, but notable exceptions:
> 
>  1. Stage 1. will also verify whether each plugin (inbuilt or custom)
>     is installed inside `/usr/share/perl5/` and not anywhere else in
>     order to guarantee that there are no conflicts while loading.

And also breaks testing changes with `perl -I`/PERLLIB env vars.

> 
>     Example: A particularly impertinent custom plugin *may* install
>     itself to another path in `@INC`, such as
>     `/usr/local/lib/x86_64-linux-gnu/perl/5.36.0` e.g., and mask
>     another inbuilt or custom plugin by having the exact same
>     namespace.
> 
>     While unlikely, this is now guarded against.
> 
>  2. The new registration machinery also checks whether inbuilt
>     plugins derive from `PVE::Storage::Plugin`.

That's not new?

> 
>  3. Inbuilt plugins are no longer imported using `use`, the same logic
>     using `require` is now used for both inbuilt and custom plugins.

As mentioned in the cover letter, this seems to trip up your point 1.

> 
>  4. The registration now happens inside the `import()` subroutine,
>     which makes it more predictable when plugins are actually loaded.
> 
>     This means that plugins from here on out are only registered when
>     running `use PVE::Storage;` and not when using `require
>     PVE::Storage;`. The difference here is somewhat subtle; to
>     elaborate, the statement `use PVE::Storage;` is equivalent to the
>     following [2]:
> 
>         BEGIN {
>             require "PVE/Storage.pm";
>             'PVE::Storage'->import();
>         }
> 
>     Because `PVE::Storage` isn't imported using a plain `require`
>     anywhere in our code, this change is safe to make.
> 
> [1]: https://perldoc.perl.org/perlmod#BEGIN%2C-UNITCHECK%2C-CHECK%2C-INIT-and-END
> [2]: https://perldoc.perl.org/functions/use
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/Storage.pm | 559 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 489 insertions(+), 70 deletions(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index df4d62f..0c8ba64 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -10,6 +10,8 @@ use IO::Socket::IP;
>  use IPC::Open3;
>  use File::Basename;
>  use File::Path;
> +use File::Spec;
> +use Carp;
>  use Cwd 'abs_path';
>  use Socket;
>  use Time::Local qw(timelocal);
> @@ -27,82 +29,12 @@ use PVE::RESTEnvironment qw(log_warn);
>  use PVE::Storage::Version;
>  
>  use PVE::Storage::Plugin;
> -use PVE::Storage::DirPlugin;
> -use PVE::Storage::LVMPlugin;
> -use PVE::Storage::LvmThinPlugin;
> -use PVE::Storage::NFSPlugin;
> -use PVE::Storage::CIFSPlugin;
> -use PVE::Storage::ISCSIPlugin;
> -use PVE::Storage::RBDPlugin;
> -use PVE::Storage::CephFSPlugin;
> -use PVE::Storage::ISCSIDirectPlugin;
> -use PVE::Storage::GlusterfsPlugin;
> -use PVE::Storage::ZFSPoolPlugin;
> -use PVE::Storage::ZFSPlugin;
> -use PVE::Storage::PBSPlugin;
> -use PVE::Storage::BTRFSPlugin;
> -use PVE::Storage::ESXiPlugin;
>  
>  use constant APIVER => PVE::Storage::Version::APIVER;
>  use constant APIAGE => PVE::Storage::Version::APIAGE;
>  
>  our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
>  
> -# load standard plugins
> -PVE::Storage::DirPlugin->register();
> -PVE::Storage::LVMPlugin->register();
> -PVE::Storage::LvmThinPlugin->register();
> -PVE::Storage::NFSPlugin->register();
> -PVE::Storage::CIFSPlugin->register();
> -PVE::Storage::ISCSIPlugin->register();
> -PVE::Storage::RBDPlugin->register();
> -PVE::Storage::CephFSPlugin->register();
> -PVE::Storage::ISCSIDirectPlugin->register();
> -PVE::Storage::GlusterfsPlugin->register();
> -PVE::Storage::ZFSPoolPlugin->register();
> -PVE::Storage::ZFSPlugin->register();
> -PVE::Storage::PBSPlugin->register();
> -PVE::Storage::BTRFSPlugin->register();
> -PVE::Storage::ESXiPlugin->register();
> -
> -# load third-party plugins
> -if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
> -    dir_glob_foreach('/usr/share/perl5/PVE/Storage/Custom', '.*\.pm$', sub {
> -	my ($file) = @_;
> -	my $modname = 'PVE::Storage::Custom::' . $file;
> -	$modname =~ s!\.pm$!!;
> -	$file = 'PVE/Storage/Custom/' . $file;
> -
> -	eval {
> -	    require $file;
> -
> -	    # Check perl interface:
> -	    die "not derived from PVE::Storage::Plugin\n" if !$modname->isa('PVE::Storage::Plugin');
> -	    die "does not provide an api() method\n" if !$modname->can('api');
> -	    # Check storage API version and that file is really storage plugin.
> -	    my $version = $modname->api();
> -	    die "implements an API version newer than current ($version > " . APIVER . ")\n"
> -		if $version > APIVER;
> -	    my $min_version = (APIVER - APIAGE);
> -	    die "API version too old, please update the plugin ($version < $min_version)\n"
> -		if $version < $min_version;
> -	    # all OK, do import and register (i.e., "use")
> -	    import $file;
> -	    $modname->register();
> -
> -	    # If we got this far and the API version is not the same, make some noise:
> -	    warn "Plugin \"$modname\" is implementing an older storage API, an upgrade is recommended\n"
> -		if $version != APIVER;
> -	};
> -	if ($@) {
> -	    warn "Error loading storage plugin \"$modname\": $@";
> -	}
> -    });
> -}

^ The above was so much shorter... (and worked :P)

> -
> -# initialize all plugins
> -PVE::Storage::Plugin->init();
> -
>  # the following REs indicate the number or capture groups via the trailing digit
>  # CAUTION don't forget to update the digits accordingly after messing with the capture groups
>  
> @@ -124,6 +56,493 @@ our $OVA_CONTENT_RE_1 = qr/${SAFE_CHAR_WITH_WHITESPACE_CLASS_RE}+\.(qcow2|raw|vm
>  # FIXME remove with PVE 9.0, add versioned breaks for pve-manager
>  our $vztmpl_extension_re = $VZTMPL_EXT_RE_1;
>  
> +# PVE::Storage plugin registration machinery
> +
> +my $plugin_register_state = {
> +    index => {},

^ A comment explaining the contents there might be useful.

> +    plugins_initialised => 0,
> +};
> +
> +=head3 get_standard_plugin_names()
> +
> +Returns an array or a reference to an array of the module names of our inbuilt
> +storage plugins, depending on the context of the caller.
> +
> +    # both valid
> +    my $standard_plugins = get_standard_plugin_names();
> +    my @standard_plugins = get_standard_plugin_names();

More POD clutter for something unnecessary 😬.

> +
> +=cut
> +
> +my sub get_standard_plugin_names : prototype() {

This should not be a sub.

> +    my @standard_plugins = qw(
> +	PVE::Storage::DirPlugin
> +	PVE::Storage::LVMPlugin
> +	PVE::Storage::LvmThinPlugin
> +	PVE::Storage::NFSPlugin
> +	PVE::Storage::CIFSPlugin
> +	PVE::Storage::ISCSIPlugin
> +	PVE::Storage::RBDPlugin
> +	PVE::Storage::CephFSPlugin
> +	PVE::Storage::ISCSIDirectPlugin
> +	PVE::Storage::GlusterfsPlugin
> +	PVE::Storage::ZFSPoolPlugin
> +	PVE::Storage::ZFSPlugin
> +	PVE::Storage::PBSPlugin
> +	PVE::Storage::BTRFSPlugin
> +	PVE::Storage::ESXiPlugin
> +    );
> +
> +    return @standard_plugins if wantarray;
> +    return \@standard_plugins;
> +}
> +
> +=head3 get_custom_plugin_names()
> +
> +Returns an array or a reference to an array of the module names of custom
> +storage plugins inside C</usr/share/perl5/PVE/Storage/Custom>, depending on the
> +context of the caller.
> +
> +    # both valid
> +    my $custom_plugins = get_custom_plugin_names();
> +    my @custom_plugins = get_custom_plugin_names();

😬

and still not convinced private oneshot subs should use POD.

> +
> +=cut
> +
> +my sub get_custom_plugin_names : prototype() {
> +    my @custom_plugins = ();
> +
> +    if ( -d '/usr/share/perl5/PVE/Storage/Custom' ) {
> +	dir_glob_foreach('/usr/share/perl5/PVE/Storage/Custom', '.*\.pm$', sub {
> +	    my ($file) = @_;
> +
> +	    my $modname = 'PVE::Storage::Custom::' . $file;
> +	    $modname =~ s!\.pm$!!;
> +
> +	    push(@custom_plugins, $modname);
> +	});
> +    }
> +
> +    return @custom_plugins if wantarray;
> +    return \@custom_plugins;
> +}
> +
> +=head3 get_indexed_plugins(%params)
> +
> +Returns the names of the currently indexed storage plugins, optionally allowing
> +certain filters flags to be passed. Returns an array or a reference to an array
> +depending on the context of the caller.
> +
> +    my @all_plugins = get_indexed_plugins();
> +
> +    my $standard_plugins = get_indexed_plugins(is_custom => 0);
> +    my @custom_plugins = get_indexed_plugins(is_custom => 1);
> +
> +    my @registered_custom_plugins = get_indexed_plugins(is_custom => 1, is_registered => 1);
> +
> +The following key-value pairs may optionally be passed and will cause this
> +subroutine to filter out any plugin names for which they don't apply:
> +
> +=over
> +
> +=item * C<< is_custom => [0|1] >>
> +
> +Whether to only return names of custom plugin names or inbuilt ones.
> +
> +=item * C<< is_checked => [0|1]>>
> +
> +Whether to only return plugins that passed or haven't passed the
> +pre-registration check.
> +
> +=item * C<< is_registered => [0|1] >>
> +
> +Whether to only return plugins that were or weren't registered.
> +
> +=back
> +
> +=cut
> +
> +my sub get_indexed_plugins : prototype(;%) {
> +    my (%params) = @_;
> +
> +    my $plugin_index = $plugin_register_state->{index};
> +
> +    my $test_index_for_flag = sub {
> +	my ($plugin_name, $flag) = @_;
> +
> +	return 0 if !defined($plugin_index->{$_});
> +	return 0 if !defined($plugin_index->{$_}->{$flag});
> +
> +	return $plugin_index->{$_}->{$flag} == $params{$flag};
> +    };
> +
> +    my @plugins = keys $plugin_index->%*;
> +
> +    @plugins = grep { $test_index_for_flag->($_, 'is_custom') } @plugins
> +	if defined($params{is_custom});

^ The helper-closure could do the entire `grep` statement to make this
just be

    $grep_for_flags->($flag) for (qw(is_checked ...));

at which point we could drop the closure and just do

    for my $flag (qw(is_custom is_checked is_registered) {
        # contents of helper sub
    }

IMO more readable.

> +
> +    @plugins = grep { $test_index_for_flag->($_, 'is_checked') } @plugins
> +	if defined($params{is_checked});
> +
> +    @plugins = grep { $test_index_for_flag->($_, 'is_registered') } @plugins
> +	if defined($params{is_registered});
> +
> +    return @plugins if wantarray;
> +    return @plugins;

^ no

> +}
> +
> +=head3 do_try_load_plugin(%params)
> +
> +Actual loading mechanism used inside C<L<< try_load_plugins()|/"try_load_plugins()" >>>.
> +Attempts to load the plugin provided via the C<name> key. Optionally, C<< is_custom => 1 >>
> +may be passed if a custom plugin is being loaded.
> +
> +    do_try_load_plugin(name => 'PVE::Storage::DirPlugin');
> +
> +    do_try_load_plugin(name => 'PVE::Storage::Custom::FooPlugin', is_custom => 1);
> +
> +The given plugin is loaded dynamically via Perl's C<require> and then added to
> +the plugin index for further operations, such as pre- and post-registration
> +checks as well as the actual registration itself.
> +
> +This subroutine performs additional sanity checks and will raise an exception if
> +the plugin doesn't exist in C</usr/share/perl5/> or if the plugin was already loaded.
> +
> +=cut
> +
> +my sub do_try_load_plugin : prototype(%) {
> +    my (%params) = @_;
> +
> +    my $plugin_name = $params{name};
> +    my $is_custom = $params{is_custom};
> +
> +    croak "no plugin name provided - must be fully qualified namespace, e.g. Foo::Bar::Baz"
> +	if !defined($plugin_name);
> +
> +    my $base_path = '/usr/share/perl5/';
> +
> +    my $plugin_filepath = File::Spec->catdir(split('::', $plugin_name)) . '.pm';
> +    my ($_volume, $plugin_dir, $plugin_filename) = File::Spec->splitpath($plugin_filepath);
> +
> +    my $plugin_filepath_abs = File::Spec->catfile($base_path, $plugin_dir, $plugin_filename);
> +
> +    croak "plugin '$plugin_name' does not exist at path '$plugin_filepath_abs'"
> +	if (! -e $plugin_filepath_abs);
> +
> +    eval {
> +	require $plugin_filepath;
> +    };
> +    croak "Failed to load plugin '$plugin_name' - $@" if $@;
> +
> +    my $plugin_index = $plugin_register_state->{index};
> +
> +    confess "Plugin '$plugin_name' is already loaded" if exists($plugin_index->{$plugin_name});
> +
> +    $plugin_index->{$plugin_name} = {
> +	absolute_path => $plugin_filepath_abs,
> +	is_custom => $is_custom || 0,
> +	is_checked => 0,
> +	is_registered => 0,
> +    };
> +
> +    return;
> +}
> +
> +=head3 try_load_plugins()
> +
> +Attempts to load all available plugins, with inbuilt ("standard") plugins being
> +loaded first and custom plugins loaded afterwards.
> +
> +All standard plugins must successfully load, otherwise an exception is thrown.
> +If a custom plugin fails to load, a warning is emitted instead.
> +
> +=cut
> +
> +my sub try_load_plugins : prototype() {
> +    my @errs = ();
> +
> +    for my $plugin (get_standard_plugin_names()) {
> +	eval {
> +	    do_try_load_plugin(name => $plugin);
> +	};
> +
> +	push(@errs, $@) if $@;
> +    }
> +
> +    confess("Encountered unexpected error(s) while loading plugins:\n", join("\n", @errs), "\n\n... ")
> +	if scalar(@errs);
> +
> +    for my $plugin (get_custom_plugin_names()) {
> +	eval {
> +	    do_try_load_plugin(name => $plugin, is_custom => 1);
> +	};
> +
> +	warn "Error loading storage plugin \"$plugin\" - $@" if $@;
> +    }
> +
> +    return;
> +}
> +
> +=head3 do_pre_register_check_plugin(%params)
> +
> +Used within C<L<< pre_register_check_plugins()|/"pre_register_check_plugins()" >>>
> +to perform the actual pre-registration checks for the plugin given by the C<name>
> +key.
> +
> +    do_pre_register_check_plugin(name => 'PVE::Storage::DirPlugin');
> +
> +This checks whether the plugin is derived from C<L<PVE::Storage::Plugin>>. If the
> +plugin is a custom plugin, its API conformance is also verified:
> +
> +=over
> +
> +=item * The plugin must provide an C<api()> method returning its API version
> +
> +=item * Its API version must not be newer than the current storage API version
> +
> +=item * Its API version must not be older than the storage API age allows
> +
> +=back
> +
> +=cut
> +
> +my sub do_pre_register_check_plugin : prototype(%) {
> +    my (%params) = @_;
> +
> +    my $plugin_name = $params{name};
> +
> +    my $plugin_index = $plugin_register_state->{index};
> +
> +    croak "Plugin '$plugin_name' does not exist in index"
> +	if !defined($plugin_index->{$plugin_name});
> +
> +    my $is_custom = $plugin_index->{$plugin_name}->{is_custom};
> +
> +    my $check_derives_base = sub {
> +	# so that we may call methods on $plugin_name
> +	no strict 'refs'; ## no critic
> +
> +	die "not derived from PVE::Storage::Plugin\n"
> +	    if !$plugin_name->isa('PVE::Storage::Plugin');
> +    };
> +
> +    my $check_plugin_api = sub {
> +	# so that we may call methods on $plugin_name
> +	no strict 'refs'; ## no critic
> +
> +	die "does not provide an 'api()' method\n"
> +	    if !$plugin_name->can('api');
> +
> +	my $version = $plugin_name->api();
> +	my $min_version = (APIVER - APIAGE);
> +
> +	die "implements an API version newer than current ($version > @{[APIVER]})\n"
> +	    if $version > APIVER;
> +
> +	die "API version too old, please update the plugin ($version < $min_version)\n"
> +	    if $version < $min_version;
> +    };
> +
> +    eval {
> +	$check_derives_base->();
> +	$check_plugin_api->() if $is_custom;
> +    };
> +
> +    croak "Pre-registration check failed for plugin '$plugin_name' - $@" if $@;
> +
> +    $plugin_index->{$plugin_name}->{is_checked} = 1;
> +
> +    return;
> +}
> +
> +=head3 pre_register_check_plugins()
> +
> +Performs various pre-registration checks for all plugins.
> +
> +Standard plugins must always pass all checks. If a custom plugin fails a check,
> +a warning is emitted instead and it will not be registered later on.
> +
> +For more details on which checks are performed, see
> +C<L<< do_pre_register_check_plugin()|/"do_pre_register_check_plugin(%params)" >>>.
> +
> +=cut
> +
> +my sub pre_register_check_plugins : prototype() {
> +    my @errs = ();
> +
> +    my @standard_plugins = get_indexed_plugins(is_custom => 0);
> +    for my $plugin (@standard_plugins) {
> +	eval {
> +	    do_pre_register_check_plugin(name => $plugin);
> +	};
> +
> +	push(@errs, $@) if $@;
> +    }
> +
> +    if (scalar(@errs)) {
> +	my $err_msg = "Encountered unexpected error while performing"
> +	    . " pre-registration check for inbuilt plugins:\n";
> +
> +	confess($err_msg, join("\n", @errs), "\n\n... ");
> +    }
> +
> +    my @custom_plugins = get_indexed_plugins(is_custom => 1);
> +    for my $plugin (@custom_plugins) {
> +	eval {
> +	    do_pre_register_check_plugin(name => $plugin);
> +	};
> +
> +	if ($@) {
> +	    warn "Encountered unexpected error while performing pre-registration"
> +		. " check for custom plugin \"$plugin\":\n$@\n";
> +	    warn "Plugin will not be registered.\n";
> +	}
> +    }
> +
> +    return;
> +}
> +
> +=head3 try_register_plugins()
> +
> +Attempts to register all plugins of the plugin index for which the
> +L<pre-registration check|/"pre_register_check_plugins()"> was successful, with
> +standard plugins being registered first and custom plugins afterwards.
> +
> +In detail, for each plugin, the C<import()> subroutine is first run, followed
> +by C<L<< register()|PVE::SectionConfig/register >>>.
> +
> +For all standard plugins both invocations must succeed, otherwise an exception
> +is thrown. If a call to either subroutine fails for a custom plugin, a warning
> +is emitted instead and the custom plugin remains unregistered.
> +
> +=cut
> +
> +my sub try_register_plugins : prototype() {
> +    my @errs = ();
> +
> +    my $plugin_index = $plugin_register_state->{index};
> +
> +    my $try_import = sub {
> +	my ($plugin_name) = @_;
> +
> +	# so that we may call methods on $plugin_name
> +	no strict 'refs'; ## no critic
> +	$plugin_name->import();

This needs to happen in `try_load_plugins` or
`pre_register_check_plugins`, otherwise `->isa()` won't work.

> +    };
> +
> +    my $try_register = sub {
> +	my ($plugin_name) = @_;
> +
> +	# so that we may call methods on $plugin_name
> +	no strict 'refs'; ## no critic
> +	$plugin_name->register();
> +
> +	$plugin_index->{$plugin_name}->{is_registered} = 1;
> +    };
> +
> +    my @standard_plugins = get_indexed_plugins(is_custom => 0, is_checked => 1);
> +    for my $plugin (@standard_plugins) {
> +	eval {
> +	    $try_import->($plugin);
> +	};
> +
> +	if ($@) {
> +	    push(@errs, "Encountered unexpected error while running '$plugin\->import()' - $@");
> +	    next;
> +	}
> +
> +	eval {
> +	    $try_register->($plugin);
> +	};
> +
> +	if ($@) {
> +	    push(@errs, "Encountered unexpected error while running '$plugin\->register()' - $@");
> +	    next;
> +	}
> +    }
> +
> +    confess join("\n\n", @errs, "\n\n... ") if scalar(@errs);
> +
> +    my @custom_plugins = get_indexed_plugins(is_custom => 1, is_checked => 1);
> +    for my $plugin (@custom_plugins) {
> +	eval {
> +	    $try_import->($plugin);
> +	};
> +
> +	if ($@) {
> +	    carp "Failed to run 'import' on custom plugin '$plugin' - $@";
> +	    carp "Please check if the plugin is installed correctly.";
> +	    carp "Continuing to register remaining custom plugins (if any).";
> +
> +	    next;
> +	}
> +
> +	eval {
> +	    $try_register->($plugin);
> +	};
> +
> +	if ($@) {
> +	    carp "Failed to register custom plugin '$plugin' - $@";
> +	    carp "Please verify whether the plugin is installed correctly.";
> +	    carp "Continuing to register remaining custom plugins (if any).";
> +
> +	    next;
> +	}
> +    }
> +
> +    return;
> +}
> +
> +=head3 post_register_check_plugins()
> +
> +Used to perform various checks on each plugin after the plugins' registration.
> +
> +This subroutine currently only emits a warning if a custom plugin's API version
> +is older than the current one.
> +
> +=cut
> +
> +my sub post_register_check_plugins : prototype() {
> +    my @custom_plugins = get_indexed_plugins(is_custom => 1, is_registered => 1);
> +    for my $plugin (@custom_plugins) {
> +	# so that we may call methods on $plugin
> +	no strict 'refs'; ## no critic
> +
> +	my $version = $plugin->api();
> +	warn "Plugin \"$plugin\" is implementing an older storage API, an upgrade is recommended\n"
> +	    if $version != APIVER;
> +    }
> +
> +    return;
> +}
> +
> +# NOTE: This is *not* like Exporter's import (that you get with e.g. `use parent qw(Exporter);`)
> +# which supports @EXPORT_OK etc.
> +#
> +# If there is ever a need to support exporting subroutines (via `Exporter`),
> +# see the following: https://perldoc.perl.org/Exporter#Exporting-Without-Using-Exporter's-import-Method
> +# Beware that this requires you to remove any keys that ought not be passed
> +# to exporter from @_.
> +
> +sub import {
> +    my ($package, %params) = @_;
> +
> +    # this sub may be called more than once, so avoid registering plugins again
> +    if ($plugin_register_state->{plugins_initialised} == 1) {
> +	return;
> +    }
> +
> +    try_load_plugins();
> +    pre_register_check_plugins();
> +    try_register_plugins();
> +    post_register_check_plugins();
> +
> +    PVE::Storage::Plugin->init();
> +    $plugin_register_state->{plugins_initialised} = 1;
> +
> +    return;
> +}
> +
>  #  PVE::Storage utility functions
>  
>  sub config {
> -- 
> 2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 3/6] introduce compile-time checks for prohibited sub overrides

[Wolfgang Bumiller]

I'd say this patch is the main thing to aid in API compatibility and
should be possible to do without all the refactoring before it?

So whether we want to do this (one way or another) should probably be
decided separately from the rest of the series.

The other deprecation attribute introduced in this series may be better
off as not-storage-specific and not bound to the storage API versions
IMO.

On Thu, Jan 30, 2025 at 03:51:21PM +0100, Max Carrara wrote:
> Add a way to prohibit overriding certain subroutines ("symbols") at
> "compile time" -- during Perl's BEGIN phase [1].
> 
> This is done by extending the previously introduced plugin loading
> machinery with an additional check.
> 
> In essence, the new `prohibitOverrideAt` attribute handler can be
> added to subroutines within `PVE::Storage::Plugin` together with a
> version and a message. Each "tagged" sub's name is stored inside a
> hash during the BEGIN phase [1], which the import machinery then uses
> to check whether the subroutine is overridden in any of the plugins or
> not.
> 
> This check will only fail if the specified API version was reached
> (hence the name `prohibitOverrideAt`). A warning is emitted one
> version prior. The APIAGE is also taken into account.
> 
> The reason for adding this is to make more drastic changes to the
> storage plugin API visible and noticeable instead of relying on
> comments that have to be checked manually by a developer. This not
> only forces third-party plugin authors to adhere more strictly to our
> API, but also makes it easier to keep our own plugins in check.
> 
> For example, when a method is expected to be removed, the attribute
> can simply be added to it with the API version of its expected
> removal. Once that API version is actually reached (while taking
> APIAGE into account!) the import machinery's check will throw an
> exception for each plugin that still overrides this method.
> 
> To keep the check available, the body of the method can be removed
> instead of removing the method as a whole.
> 
> [1]: https://perldoc.perl.org/perlmod#BEGIN%2C-UNITCHECK%2C-CHECK%2C-INIT-and-END
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> Suggested-by: Wolfang Bumiller <w.bumiller@proxmox.com>
> ---
>  src/PVE/Storage.pm         |  19 +++
>  src/PVE/Storage/Plugin.pm  | 133 +++++++++++++++++++
>  src/PVE/Storage/Version.pm | 253 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 405 insertions(+)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 0c8ba64..a9568b1 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -302,6 +302,8 @@ plugin is a custom plugin, its API conformance is also verified:
>  
>  =item * Its API version must not be older than the storage API age allows
>  
> +=item * No deprecated or prohibited methods may be overridden
> +
>  =back
>  
>  =cut
> @@ -343,9 +345,26 @@ my sub do_pre_register_check_plugin : prototype(%) {
>  	    if $version < $min_version;
>      };
>  
> +    my $check_symbols = sub {
> +	my $prohibited_symbols = PVE::Storage::Plugin::get_prohibited_symbols();
> +
> +	PVE::Storage::Version::raise_on_prohibited_symbols(
> +	    plugin => $plugin_name,
> +	    plugin_parent => 'PVE::Storage::Plugin',
> +	    prohibited_symbols => $prohibited_symbols,
> +	);
> +
> +	PVE::Storage::Version::warn_on_soon_prohibited_symbols(

^ weird name

> +	    plugin => $plugin_name,
> +	    plugin_parent => 'PVE::Storage::Plugin',
> +	    prohibited_symbols => $prohibited_symbols,
> +	);
> +    };
> +
>      eval {
>  	$check_derives_base->();
>  	$check_plugin_api->() if $is_custom;
> +	$check_symbols->();
>      };
>  
>      croak "Pre-registration check failed for plugin '$plugin_name' - $@" if $@;
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 65cf43f..bcc4eea 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -3,6 +3,8 @@ package PVE::Storage::Plugin;
>  use strict;
>  use warnings;
>  
> +use Attribute::Handlers;
> +use Carp;
>  use Cwd qw(abs_path);
>  use Encode qw(decode);
>  use Fcntl ':mode';
> @@ -10,6 +12,7 @@ use File::chdir;
>  use File::Path;
>  use File::Basename;
>  use File::stat qw();
> +use Scalar::Util qw(reftype);
>  
>  use PVE::Tools qw(run_command);
>  use PVE::JSONSchema qw(get_standard_option register_standard_option);
> @@ -27,6 +30,136 @@ use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
>  use constant LOG_EXT => ".log";
>  use constant NOTES_EXT => ".notes";
>  
> +=head1 ATTRIBUTES
> +
> +=cut
> +
> +my $override_prohibited_methods;
> +
> +# necessary so that the hash becomes available as early as possible
> +BEGIN {
> +    $override_prohibited_methods = {};
> +}
> +
> +=head3 prohibitOverrideAt(...)
> +
> +An attribute handler used to I<prohibit> a subroutine ("symbol") from being
> +overridden from a particular API C<version> onwards. The handler also requires
> +a C<message> that explains why the subroutine may not be overridden.
> +
> +    sub some_subroutine : prohibitOverrideAt(
> +	version => 42,
> +	message => "Overriding this subroutine in child plugins leads to unexpected behaviour.",
> +    ) {
> +	# ...
> +    }
> +
> +This handler stores all prohibited symbols. You may return a nested hash of all
> +symbols using C<L<< get_prohibited_symbols()|/"get_prohibited_symbols()" >>>.
> +
> +=cut
> +
> +sub prohibitOverrideAt : ATTR(CODE,BEGIN) {

^ snake_case, and `confess` style below

> +    my ($package, $symbol, $referent, $attr_name, $attr_data, $phase, $filename, $line) = @_;
> +
> +    confess "'$attr_name' attribute may only be used inside the PVE::Storage::Plugin module"
> +	if $package ne 'PVE::Storage::Plugin';
> +
> +    my $data = {};
> +    $data = { $attr_data->@* } if defined($attr_data);
> +
> +    my ($version, $message) = ($data->{version}, $data->{message});
> +    my $reftype = reftype($referent);
> +
> +    confess "Referent is not a reference"
> +	. " -- '$attr_name' attribute may only be used for subroutines"
> +	if !defined($reftype);
> +
> +    confess "Unexpected reftype '$reftype'"
> +	. " -- '$attr_name' attribute may only be used for subroutines"
> +	if !($reftype eq 'CODE' || $reftype eq 'ANON');
> +
> +    confess "Either version or message not defined"
> +	. " -- usage: $attr_name(version => 42, message => \"...\")"
> +	if !defined($version) || !defined($message);
> +
> +    my $symbol_name = *{$symbol}{"NAME"};
> +
> +    confess "Symbol name is undef in $filename line $line\n"
> +	if !defined($symbol_name);
> +
> +    confess "Symbol name already exists in tracked symbols in $filename line $line\n"
> +	if exists($override_prohibited_methods->{$symbol_name});
> +
> +    $override_prohibited_methods->{$symbol_name} = {
> +	version => $version,
> +	message => $message,
> +	package => $package,
> +	symbol => $symbol,
> +	referent => $referent,
> +	reftype => $reftype,
> +	attr_name => $attr_name,
> +	attr_data => $attr_data,
> +	phase => $phase,
> +	filename => $filename,
> +	line => $line,
> +    };
> +
> +    return;
> +}
> +
> +=head1 HELPERS
> +
> +=head3 get_prohibited_symbols()
> +
> +Returns a shallow copy of the nested hash that contains all of the symbols
> +that were marked as I<prohibited> by the C<L<< prohibitOverrideAt|/prohibitOverrideAt(...) >>> attribute.
> +
> +The C<version> and C<message> keys that were passed to the attribute handler
> +are passed along to the nested hash:
> +
> +    {
> +	symbol_name => {
> +	    version => 10,
> +	    message => '...',
> +	    # ...
> +	},
> +	# ...
> +    }
> +
> +The remaining keys are taken from the attribute handler's parameters and are
> +passed along for e.g. better error messaging:
> +
> +    {
> +	symbol_name => {
> +	    version => 10,
> +	    message => '...',
> +	    package => '...',         # the name of the package in which the attribute was invoked
> +	    symbol => $symbol,        # reference to the symbol table entry (typeglob)
> +	    referent => $referent,    # reference to the actual symbol (subroutine)
> +	    reftype => $reftype,      # the type of the 'referent', usually 'CODE'
> +	    attr_name => '...',       # the name of the attribute that created this hash, usually 'prohibitOverrideAt'
> +	    attr_data => $attr_data,  # the data that was passed to the attribute
> +	    phase => 'BEGIN',         # in which phase the attribute was invoked in, usually 'BEGIN' or 'CHECK'
> +	    filename => '...',        # the filename in which the attribute was invoked
> +	    line => $line,            # the line on which the attribute was invoked
> +	},
> +	# ...
> +    }
> +
> +=cut
> +
> +sub get_prohibited_symbols : prohibitOverrideAt(
> +    version => 0,
> +    message => "This subroutine is for internal use only."
> +) {

    return { %$override_prohibited_methods };

> +    # shallow copy
> +    return {
> +	map { $_, $override_prohibited_methods->{$_} }
> +	keys $override_prohibited_methods->%*
> +    };
> +}
> +
>  our @COMMON_TAR_FLAGS = qw(
>      --one-file-system
>      -p --sparse --numeric-owner --acls
> diff --git a/src/PVE/Storage/Version.pm b/src/PVE/Storage/Version.pm
> index a9216c9..5efb185 100644
> --- a/src/PVE/Storage/Version.pm
> +++ b/src/PVE/Storage/Version.pm
> @@ -176,5 +176,258 @@ sub UNIVERSAL::pveStorageDeprecateAt : ATTR(CODE,CHECK) {
>      return;
>  }
>  
> +=head1 FUNCTIONS
> +
> +=head2 Dealing With Prohibited Symbols
> +
> +When a symbol of a storage plugin is considered I<prohibited>, it means that no
> +I<child plugin> may override or extend the symbol.
> +
> +In other words, it prevents plugins from overriding methods and helpers they
> +shouldn't override to begin with.
> +
> +Currently, the base storage plugin C<L<PVE::Storage::Plugin>> is the only such
> +plugin that defines prohibited symbols. These are returned as a nested hash via
> +C<L<< get_prohibited_symbols()|PVE::Storage::Plugin/get_prohibited_symbols() >>>,
> +which is used by functions like C<L<< find_overridden_prohibited_symbols()|/"find_overridden_prohibited_symbols(%params)" >>>
> +and C<L<< raise_on_prohibited_symbols()|/"raise_on_prohibited_symbols(%params)" >>>.
> +
> +For details on how this hash is constructed, see: C<L<< prohibitOverrideAt()|PVE::Storage::Plugin/prohibitOverrideAt(...) >>>.
> +
> +=cut
> +
> +my sub get_plugin_symbols : prototype($$) {
> +    my ($plugin, $plugin_parent) = @_;
> +
> +    my $is_derived_from_parent = eval {
> +	no strict 'refs'; ## no critic
> +	$plugin->isa($plugin_parent);
> +    };
> +
> +    confess $@ if $@;
> +    confess "'$plugin' does not derive from '$plugin_parent'" if !$is_derived_from_parent;
> +
> +    my $plugin_symbols = eval {
> +	my $mod_name = $plugin . '::';
> +
> +	no strict 'refs'; ## no critic
> +
> +	# shallow copy
> +	return { map { $_, $mod_name->{$_} } keys $mod_name->%* };
> +    };
> +
> +    confess $@ if $@;
> +
> +    return $plugin_symbols;
> +}
> +
> +=head3 find_overridden_prohibited_symbols(%params)
> +
> +Returns an array or a reference to an array of the names of symbols that were
> +overridden by the given plugin despite being prohibited.
> +
> +    my @symbols = find_overridden_prohibited_symbols(
> +	plugin => 'PVE::Storage::SomePlugin',
> +	plugin_parent => 'PVE::Storage::Plugin',
> +	prohibited_symbols => $prohibited_symbols,
> +    );
> +
> +=cut
> +
> +sub find_overridden_prohibited_symbols : prototype(%) {
> +    my (%params) = @_;
> +
> +    my $plugin = $params{plugin}
> +	or confess "'plugin' key not provided";
> +    my $plugin_parent = $params{plugin_parent}
> +	or confess "'plugin_parent' key not provided";
> +    my $prohibited_symbols = $params{prohibited_symbols}
> +	or confess "'prohibited_symbols' key not provided";
> +
> +    my $plugin_symbols = get_plugin_symbols($plugin, $plugin_parent);
> +
> +    my @found_symbols = ();
> +
> +    for my $symbol (keys $plugin_symbols->%*) {
> +	next if !defined($prohibited_symbols->{$symbol});
> +
> +	my $refers_to_same_as_parent = eval {
> +	    no strict 'refs'; ## no critic
> +
> +	    my $can_plugin = $plugin->UNIVERSAL::can($symbol);
> +	    my $can_parent = $plugin_parent->UNIVERSAL::can($symbol);
> +
> +	    defined($can_plugin) && defined($can_parent) && ($can_plugin == $can_parent)
> +	};
> +
> +	push(@found_symbols, $symbol) if !$refers_to_same_as_parent;
> +    }
> +
> +    return @found_symbols if wantarray;
> +    return \@found_symbols;
> +}
> +
> +=head3 raise_on_prohibited_symbols(%params)
> +
> +Raises an exception if the given C<plugin> overrides or extends any of the
> +C<prohibited_symbols> of the C<plugin_parent>.
> +
> +    raise_on_prohibited_symbols(
> +	plugin => 'PVE::Storage::SomePlugin',
> +	plugin_parent => 'PVE::Storage::Plugin',
> +	prohibited_symbols => $prohibited_symbols,
> +    );
> +
> +If multiple prohibited symbols are overridden, the exception will consist of
> +multiple messages instead of one.
> +
> +=cut
> +
> +sub raise_on_prohibited_symbols : prototype(%) {
> +    my (%params) = @_;
> +
> +    my $plugin = $params{plugin}
> +	or confess "'plugin' key not provided";
> +    my $plugin_parent = $params{plugin_parent}
> +	or confess "'plugin_parent' key not provided";
> +    my $prohibited_symbols = $params{prohibited_symbols}
> +	or confess "'prohibited_symbols' key not provided";
> +
> +    my @overridden_symbols = find_overridden_prohibited_symbols(
> +	plugin => $plugin,
> +	plugin_parent => $plugin_parent,
> +	prohibited_symbols => $prohibited_symbols,
> +    );
> +
> +    my @errs = ();
> +
> +    for my $symbol (@overridden_symbols) {
> +	# sanity check
> +	confess "expected overridden symbol to be prohibited"
> +	    if !defined($prohibited_symbols->{$symbol});
> +
> +	my $symbol_hash = $prohibited_symbols->{$symbol};
> +
> +	my $version = $symbol_hash->{version};
> +
> +	my $is_version_too_old = ($version // -1) <= $MIN_VERSION;
> +
> +	next if !$is_version_too_old;
> +
> +	# error message is formed in a way that uses as much information as
> +	# possible, since keys of $symbol_hash aren't strictly required to be
> +	# defined
> +
> +	my $err_msg = "$plugin defines prohibited symbol \"$symbol\"";
> +
> +	my ($message, $filename, $reftype, $line) = (
> +	    $symbol_hash->{message},
> +	    $symbol_hash->{filename},
> +	    $symbol_hash->{reftype},
> +	    $symbol_hash->{line},
> +	);
> +
> +	if (defined($version)) {
> +	    $err_msg .= " - not allowed since version $version"
> +		. " (API version: @{[APIVER]}, minimum supported API version: $MIN_VERSION)";
> +	}
> +
> +	$err_msg .= ": $message" if defined($message);
> +
> +	if (defined($filename)) {
> +	    $err_msg .= "\nOriginally defined in $filename";
> +
> +	    $err_msg .= " as reftype '$reftype'" if defined($reftype);
> +	    $err_msg .= " at line $line" if defined($line);
> +	}
> +
> +	push(@errs, $err_msg);
> +    }
> +
> +    croak(join("\n", @errs), "\n") if scalar(@errs);
> +
> +    return;
> +}
> +
> +=head3 warn_on_soon_prohibited_symbols(%params)
> +
> +Emits a warning for each of the C<prohibited_symbols> of the C<plugin_parent>
> +that the given C<plugin> overrides, if the symbol will soon be prohibited
> +(e.g. when APIAGE is reset and/or APIVER is bumped).
> +
> +    warn_on_soon_prohibited_symbols(
> +	plugin => 'PVE::Storage::SomePlugin',
> +	plugin_parent => 'PVE::Storage::Plugin',
> +	prohibited_symbols => $prohibited_symbols,
> +    );
> +
> +=cut
> +
> +sub warn_on_soon_prohibited_symbols : prototype(%) {
> +    my (%params) = @_;
> +
> +    my $plugin = $params{plugin}
> +	or confess "'plugin' key not provided";
> +    my $plugin_parent = $params{plugin_parent}
> +	or confess "'plugin_parent' key not provided";
> +    my $prohibited_symbols = $params{prohibited_symbols}
> +	or confess "'prohibited_symbols' key not provided";
> +
> +    # How many versions to warn beforehand
> +    my $warn_version_offset = 1;
> +
> +    my @overridden_symbols = find_overridden_prohibited_symbols(
> +	plugin => $plugin,
> +	plugin_parent => $plugin_parent,
> +	prohibited_symbols => $prohibited_symbols,
> +    );
> +
> +    for my $symbol (@overridden_symbols) {
> +	# sanity check
> +	confess "expected overridden symbol to be prohibited"
> +	    if !defined($prohibited_symbols->{$symbol});
> +
> +	my $symbol_hash = $prohibited_symbols->{$symbol};
> +
> +	my $version = $symbol_hash->{version};
> +
> +	next if !defined($version);
> +
> +	my $is_version_old = $version >= $MIN_VERSION
> +	    && $version <= $MIN_VERSION + $warn_version_offset;
> +
> +	next if !$is_version_old;
> +
> +	# warning is formed in a way that uses as much information as possible,
> +	# since keys of $symbol_hash aren't strictly required to be defined
> +
> +	my $warning = "\"$plugin\" defines symbol \"$symbol\"";
> +
> +	my ($message, $filename, $reftype, $line) = (
> +	    $symbol_hash->{message},
> +	    $symbol_hash->{filename},
> +	    $symbol_hash->{reftype},
> +	    $symbol_hash->{line},
> +	);
> +
> +	if (defined($version)) {
> +	    $warning .= " which may not be used anymore from API version $version onward"
> +		. " (API version: @{[APIVER]}, minimum supported API version: $MIN_VERSION)";
> +	}
> +
> +	$warning .= ": $message" if defined($message);
> +
> +	if (defined($filename)) {
> +	    $warning .= "\nOriginally defined in $filename";
> +
> +	    $warning .= " as reftype '$reftype'" if defined($reftype);
> +	    $warning .= " at line $line" if defined($line);
> +	}
> +
> +	warn $warning, "\n\n";
> +    }
> +
> +    return;
> +}


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 3/6] introduce compile-time checks for prohibited sub overrides

[Wolfgang Bumiller]

On Wed, Feb 05, 2025 at 12:41:28PM +0100, Wolfgang Bumiller wrote:
> I'd say this patch is the main thing to aid in API compatibility and
> should be possible to do without all the refactoring before it?
> 
> So whether we want to do this (one way or another) should probably be
> decided separately from the rest of the series.
> 
> The other deprecation attribute introduced in this series may be better
> off as not-storage-specific and not bound to the storage API versions
> IMO.

Something Thomas just noted:

The deprecation warnings here mostly hit the *users*, so we need to be
careful with how much log-spam we want to produce.

Particularly since things which are just deprecated still need to be
kept around to support *older* installations.

Otherwise we punish plugin authors by forcing them to release new
versions which are *only* there to remove log-spam while making them
explicitly incompatible with older pve versions for no reason.

It might be better if the warnings are guarded by an env var so that
plugin authors can run a check explicitly as a development-time helper:

    PVE_STORAGE_API_CHECK=1 perl -e 'use PVE::Storage;'

> 
> On Thu, Jan 30, 2025 at 03:51:21PM +0100, Max Carrara wrote:
> > Add a way to prohibit overriding certain subroutines ("symbols") at
> > "compile time" -- during Perl's BEGIN phase [1].
> > 
> > This is done by extending the previously introduced plugin loading
> > machinery with an additional check.
> > 
> > In essence, the new `prohibitOverrideAt` attribute handler can be
> > added to subroutines within `PVE::Storage::Plugin` together with a
> > version and a message. Each "tagged" sub's name is stored inside a
> > hash during the BEGIN phase [1], which the import machinery then uses
> > to check whether the subroutine is overridden in any of the plugins or
> > not.
> > 
> > This check will only fail if the specified API version was reached
> > (hence the name `prohibitOverrideAt`). A warning is emitted one
> > version prior. The APIAGE is also taken into account.
> > 
> > The reason for adding this is to make more drastic changes to the
> > storage plugin API visible and noticeable instead of relying on
> > comments that have to be checked manually by a developer. This not
> > only forces third-party plugin authors to adhere more strictly to our
> > API, but also makes it easier to keep our own plugins in check.
> > 
> > For example, when a method is expected to be removed, the attribute
> > can simply be added to it with the API version of its expected
> > removal. Once that API version is actually reached (while taking
> > APIAGE into account!) the import machinery's check will throw an
> > exception for each plugin that still overrides this method.
> > 
> > To keep the check available, the body of the method can be removed
> > instead of removing the method as a whole.
> > 
> > [1]: https://perldoc.perl.org/perlmod#BEGIN%2C-UNITCHECK%2C-CHECK%2C-INIT-and-END
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > Suggested-by: Wolfang Bumiller <w.bumiller@proxmox.com>
> > ---
> >  src/PVE/Storage.pm         |  19 +++
> >  src/PVE/Storage/Plugin.pm  | 133 +++++++++++++++++++
> >  src/PVE/Storage/Version.pm | 253 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 405 insertions(+)
> > 
> > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> > index 0c8ba64..a9568b1 100755
> > --- a/src/PVE/Storage.pm
> > +++ b/src/PVE/Storage.pm
> > @@ -302,6 +302,8 @@ plugin is a custom plugin, its API conformance is also verified:
> >  
> >  =item * Its API version must not be older than the storage API age allows
> >  
> > +=item * No deprecated or prohibited methods may be overridden
> > +
> >  =back
> >  
> >  =cut
> > @@ -343,9 +345,26 @@ my sub do_pre_register_check_plugin : prototype(%) {
> >  	    if $version < $min_version;
> >      };
> >  
> > +    my $check_symbols = sub {
> > +	my $prohibited_symbols = PVE::Storage::Plugin::get_prohibited_symbols();
> > +
> > +	PVE::Storage::Version::raise_on_prohibited_symbols(
> > +	    plugin => $plugin_name,
> > +	    plugin_parent => 'PVE::Storage::Plugin',
> > +	    prohibited_symbols => $prohibited_symbols,
> > +	);
> > +
> > +	PVE::Storage::Version::warn_on_soon_prohibited_symbols(
> 
> ^ weird name
> 
> > +	    plugin => $plugin_name,
> > +	    plugin_parent => 'PVE::Storage::Plugin',
> > +	    prohibited_symbols => $prohibited_symbols,
> > +	);
> > +    };
> > +
> >      eval {
> >  	$check_derives_base->();
> >  	$check_plugin_api->() if $is_custom;
> > +	$check_symbols->();
> >      };
> >  
> >      croak "Pre-registration check failed for plugin '$plugin_name' - $@" if $@;
> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> > index 65cf43f..bcc4eea 100644
> > --- a/src/PVE/Storage/Plugin.pm
> > +++ b/src/PVE/Storage/Plugin.pm
> > @@ -3,6 +3,8 @@ package PVE::Storage::Plugin;
> >  use strict;
> >  use warnings;
> >  
> > +use Attribute::Handlers;
> > +use Carp;
> >  use Cwd qw(abs_path);
> >  use Encode qw(decode);
> >  use Fcntl ':mode';
> > @@ -10,6 +12,7 @@ use File::chdir;
> >  use File::Path;
> >  use File::Basename;
> >  use File::stat qw();
> > +use Scalar::Util qw(reftype);
> >  
> >  use PVE::Tools qw(run_command);
> >  use PVE::JSONSchema qw(get_standard_option register_standard_option);
> > @@ -27,6 +30,136 @@ use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
> >  use constant LOG_EXT => ".log";
> >  use constant NOTES_EXT => ".notes";
> >  
> > +=head1 ATTRIBUTES
> > +
> > +=cut
> > +
> > +my $override_prohibited_methods;
> > +
> > +# necessary so that the hash becomes available as early as possible
> > +BEGIN {
> > +    $override_prohibited_methods = {};
> > +}
> > +
> > +=head3 prohibitOverrideAt(...)
> > +
> > +An attribute handler used to I<prohibit> a subroutine ("symbol") from being
> > +overridden from a particular API C<version> onwards. The handler also requires
> > +a C<message> that explains why the subroutine may not be overridden.
> > +
> > +    sub some_subroutine : prohibitOverrideAt(
> > +	version => 42,
> > +	message => "Overriding this subroutine in child plugins leads to unexpected behaviour.",
> > +    ) {
> > +	# ...
> > +    }
> > +
> > +This handler stores all prohibited symbols. You may return a nested hash of all
> > +symbols using C<L<< get_prohibited_symbols()|/"get_prohibited_symbols()" >>>.
> > +
> > +=cut
> > +
> > +sub prohibitOverrideAt : ATTR(CODE,BEGIN) {
> 
> ^ snake_case, and `confess` style below
> 
> > +    my ($package, $symbol, $referent, $attr_name, $attr_data, $phase, $filename, $line) = @_;
> > +
> > +    confess "'$attr_name' attribute may only be used inside the PVE::Storage::Plugin module"
> > +	if $package ne 'PVE::Storage::Plugin';
> > +
> > +    my $data = {};
> > +    $data = { $attr_data->@* } if defined($attr_data);
> > +
> > +    my ($version, $message) = ($data->{version}, $data->{message});
> > +    my $reftype = reftype($referent);
> > +
> > +    confess "Referent is not a reference"
> > +	. " -- '$attr_name' attribute may only be used for subroutines"
> > +	if !defined($reftype);
> > +
> > +    confess "Unexpected reftype '$reftype'"
> > +	. " -- '$attr_name' attribute may only be used for subroutines"
> > +	if !($reftype eq 'CODE' || $reftype eq 'ANON');
> > +
> > +    confess "Either version or message not defined"
> > +	. " -- usage: $attr_name(version => 42, message => \"...\")"
> > +	if !defined($version) || !defined($message);
> > +
> > +    my $symbol_name = *{$symbol}{"NAME"};
> > +
> > +    confess "Symbol name is undef in $filename line $line\n"
> > +	if !defined($symbol_name);
> > +
> > +    confess "Symbol name already exists in tracked symbols in $filename line $line\n"
> > +	if exists($override_prohibited_methods->{$symbol_name});
> > +
> > +    $override_prohibited_methods->{$symbol_name} = {
> > +	version => $version,
> > +	message => $message,
> > +	package => $package,
> > +	symbol => $symbol,
> > +	referent => $referent,
> > +	reftype => $reftype,
> > +	attr_name => $attr_name,
> > +	attr_data => $attr_data,
> > +	phase => $phase,
> > +	filename => $filename,
> > +	line => $line,
> > +    };
> > +
> > +    return;
> > +}
> > +
> > +=head1 HELPERS
> > +
> > +=head3 get_prohibited_symbols()
> > +
> > +Returns a shallow copy of the nested hash that contains all of the symbols
> > +that were marked as I<prohibited> by the C<L<< prohibitOverrideAt|/prohibitOverrideAt(...) >>> attribute.
> > +
> > +The C<version> and C<message> keys that were passed to the attribute handler
> > +are passed along to the nested hash:
> > +
> > +    {
> > +	symbol_name => {
> > +	    version => 10,
> > +	    message => '...',
> > +	    # ...
> > +	},
> > +	# ...
> > +    }
> > +
> > +The remaining keys are taken from the attribute handler's parameters and are
> > +passed along for e.g. better error messaging:
> > +
> > +    {
> > +	symbol_name => {
> > +	    version => 10,
> > +	    message => '...',
> > +	    package => '...',         # the name of the package in which the attribute was invoked
> > +	    symbol => $symbol,        # reference to the symbol table entry (typeglob)
> > +	    referent => $referent,    # reference to the actual symbol (subroutine)
> > +	    reftype => $reftype,      # the type of the 'referent', usually 'CODE'
> > +	    attr_name => '...',       # the name of the attribute that created this hash, usually 'prohibitOverrideAt'
> > +	    attr_data => $attr_data,  # the data that was passed to the attribute
> > +	    phase => 'BEGIN',         # in which phase the attribute was invoked in, usually 'BEGIN' or 'CHECK'
> > +	    filename => '...',        # the filename in which the attribute was invoked
> > +	    line => $line,            # the line on which the attribute was invoked
> > +	},
> > +	# ...
> > +    }
> > +
> > +=cut
> > +
> > +sub get_prohibited_symbols : prohibitOverrideAt(
> > +    version => 0,
> > +    message => "This subroutine is for internal use only."
> > +) {
> 
>     return { %$override_prohibited_methods };
> 
> > +    # shallow copy
> > +    return {
> > +	map { $_, $override_prohibited_methods->{$_} }
> > +	keys $override_prohibited_methods->%*
> > +    };
> > +}
> > +
> >  our @COMMON_TAR_FLAGS = qw(
> >      --one-file-system
> >      -p --sparse --numeric-owner --acls
> > diff --git a/src/PVE/Storage/Version.pm b/src/PVE/Storage/Version.pm
> > index a9216c9..5efb185 100644
> > --- a/src/PVE/Storage/Version.pm
> > +++ b/src/PVE/Storage/Version.pm
> > @@ -176,5 +176,258 @@ sub UNIVERSAL::pveStorageDeprecateAt : ATTR(CODE,CHECK) {
> >      return;
> >  }
> >  
> > +=head1 FUNCTIONS
> > +
> > +=head2 Dealing With Prohibited Symbols
> > +
> > +When a symbol of a storage plugin is considered I<prohibited>, it means that no
> > +I<child plugin> may override or extend the symbol.
> > +
> > +In other words, it prevents plugins from overriding methods and helpers they
> > +shouldn't override to begin with.
> > +
> > +Currently, the base storage plugin C<L<PVE::Storage::Plugin>> is the only such
> > +plugin that defines prohibited symbols. These are returned as a nested hash via
> > +C<L<< get_prohibited_symbols()|PVE::Storage::Plugin/get_prohibited_symbols() >>>,
> > +which is used by functions like C<L<< find_overridden_prohibited_symbols()|/"find_overridden_prohibited_symbols(%params)" >>>
> > +and C<L<< raise_on_prohibited_symbols()|/"raise_on_prohibited_symbols(%params)" >>>.
> > +
> > +For details on how this hash is constructed, see: C<L<< prohibitOverrideAt()|PVE::Storage::Plugin/prohibitOverrideAt(...) >>>.
> > +
> > +=cut
> > +
> > +my sub get_plugin_symbols : prototype($$) {
> > +    my ($plugin, $plugin_parent) = @_;
> > +
> > +    my $is_derived_from_parent = eval {
> > +	no strict 'refs'; ## no critic
> > +	$plugin->isa($plugin_parent);
> > +    };
> > +
> > +    confess $@ if $@;
> > +    confess "'$plugin' does not derive from '$plugin_parent'" if !$is_derived_from_parent;
> > +
> > +    my $plugin_symbols = eval {
> > +	my $mod_name = $plugin . '::';
> > +
> > +	no strict 'refs'; ## no critic
> > +
> > +	# shallow copy
> > +	return { map { $_, $mod_name->{$_} } keys $mod_name->%* };
> > +    };
> > +
> > +    confess $@ if $@;
> > +
> > +    return $plugin_symbols;
> > +}
> > +
> > +=head3 find_overridden_prohibited_symbols(%params)
> > +
> > +Returns an array or a reference to an array of the names of symbols that were
> > +overridden by the given plugin despite being prohibited.
> > +
> > +    my @symbols = find_overridden_prohibited_symbols(
> > +	plugin => 'PVE::Storage::SomePlugin',
> > +	plugin_parent => 'PVE::Storage::Plugin',
> > +	prohibited_symbols => $prohibited_symbols,
> > +    );
> > +
> > +=cut
> > +
> > +sub find_overridden_prohibited_symbols : prototype(%) {
> > +    my (%params) = @_;
> > +
> > +    my $plugin = $params{plugin}
> > +	or confess "'plugin' key not provided";
> > +    my $plugin_parent = $params{plugin_parent}
> > +	or confess "'plugin_parent' key not provided";
> > +    my $prohibited_symbols = $params{prohibited_symbols}
> > +	or confess "'prohibited_symbols' key not provided";
> > +
> > +    my $plugin_symbols = get_plugin_symbols($plugin, $plugin_parent);
> > +
> > +    my @found_symbols = ();
> > +
> > +    for my $symbol (keys $plugin_symbols->%*) {
> > +	next if !defined($prohibited_symbols->{$symbol});
> > +
> > +	my $refers_to_same_as_parent = eval {
> > +	    no strict 'refs'; ## no critic
> > +
> > +	    my $can_plugin = $plugin->UNIVERSAL::can($symbol);
> > +	    my $can_parent = $plugin_parent->UNIVERSAL::can($symbol);
> > +
> > +	    defined($can_plugin) && defined($can_parent) && ($can_plugin == $can_parent)
> > +	};
> > +
> > +	push(@found_symbols, $symbol) if !$refers_to_same_as_parent;
> > +    }
> > +
> > +    return @found_symbols if wantarray;
> > +    return \@found_symbols;
> > +}
> > +
> > +=head3 raise_on_prohibited_symbols(%params)
> > +
> > +Raises an exception if the given C<plugin> overrides or extends any of the
> > +C<prohibited_symbols> of the C<plugin_parent>.
> > +
> > +    raise_on_prohibited_symbols(
> > +	plugin => 'PVE::Storage::SomePlugin',
> > +	plugin_parent => 'PVE::Storage::Plugin',
> > +	prohibited_symbols => $prohibited_symbols,
> > +    );
> > +
> > +If multiple prohibited symbols are overridden, the exception will consist of
> > +multiple messages instead of one.
> > +
> > +=cut
> > +
> > +sub raise_on_prohibited_symbols : prototype(%) {
> > +    my (%params) = @_;
> > +
> > +    my $plugin = $params{plugin}
> > +	or confess "'plugin' key not provided";
> > +    my $plugin_parent = $params{plugin_parent}
> > +	or confess "'plugin_parent' key not provided";
> > +    my $prohibited_symbols = $params{prohibited_symbols}
> > +	or confess "'prohibited_symbols' key not provided";
> > +
> > +    my @overridden_symbols = find_overridden_prohibited_symbols(
> > +	plugin => $plugin,
> > +	plugin_parent => $plugin_parent,
> > +	prohibited_symbols => $prohibited_symbols,
> > +    );
> > +
> > +    my @errs = ();
> > +
> > +    for my $symbol (@overridden_symbols) {
> > +	# sanity check
> > +	confess "expected overridden symbol to be prohibited"
> > +	    if !defined($prohibited_symbols->{$symbol});
> > +
> > +	my $symbol_hash = $prohibited_symbols->{$symbol};
> > +
> > +	my $version = $symbol_hash->{version};
> > +
> > +	my $is_version_too_old = ($version // -1) <= $MIN_VERSION;
> > +
> > +	next if !$is_version_too_old;
> > +
> > +	# error message is formed in a way that uses as much information as
> > +	# possible, since keys of $symbol_hash aren't strictly required to be
> > +	# defined
> > +
> > +	my $err_msg = "$plugin defines prohibited symbol \"$symbol\"";
> > +
> > +	my ($message, $filename, $reftype, $line) = (
> > +	    $symbol_hash->{message},
> > +	    $symbol_hash->{filename},
> > +	    $symbol_hash->{reftype},
> > +	    $symbol_hash->{line},
> > +	);
> > +
> > +	if (defined($version)) {
> > +	    $err_msg .= " - not allowed since version $version"
> > +		. " (API version: @{[APIVER]}, minimum supported API version: $MIN_VERSION)";
> > +	}
> > +
> > +	$err_msg .= ": $message" if defined($message);
> > +
> > +	if (defined($filename)) {
> > +	    $err_msg .= "\nOriginally defined in $filename";
> > +
> > +	    $err_msg .= " as reftype '$reftype'" if defined($reftype);
> > +	    $err_msg .= " at line $line" if defined($line);
> > +	}
> > +
> > +	push(@errs, $err_msg);
> > +    }
> > +
> > +    croak(join("\n", @errs), "\n") if scalar(@errs);
> > +
> > +    return;
> > +}
> > +
> > +=head3 warn_on_soon_prohibited_symbols(%params)
> > +
> > +Emits a warning for each of the C<prohibited_symbols> of the C<plugin_parent>
> > +that the given C<plugin> overrides, if the symbol will soon be prohibited
> > +(e.g. when APIAGE is reset and/or APIVER is bumped).
> > +
> > +    warn_on_soon_prohibited_symbols(
> > +	plugin => 'PVE::Storage::SomePlugin',
> > +	plugin_parent => 'PVE::Storage::Plugin',
> > +	prohibited_symbols => $prohibited_symbols,
> > +    );
> > +
> > +=cut
> > +
> > +sub warn_on_soon_prohibited_symbols : prototype(%) {
> > +    my (%params) = @_;
> > +
> > +    my $plugin = $params{plugin}
> > +	or confess "'plugin' key not provided";
> > +    my $plugin_parent = $params{plugin_parent}
> > +	or confess "'plugin_parent' key not provided";
> > +    my $prohibited_symbols = $params{prohibited_symbols}
> > +	or confess "'prohibited_symbols' key not provided";
> > +
> > +    # How many versions to warn beforehand
> > +    my $warn_version_offset = 1;
> > +
> > +    my @overridden_symbols = find_overridden_prohibited_symbols(
> > +	plugin => $plugin,
> > +	plugin_parent => $plugin_parent,
> > +	prohibited_symbols => $prohibited_symbols,
> > +    );
> > +
> > +    for my $symbol (@overridden_symbols) {
> > +	# sanity check
> > +	confess "expected overridden symbol to be prohibited"
> > +	    if !defined($prohibited_symbols->{$symbol});
> > +
> > +	my $symbol_hash = $prohibited_symbols->{$symbol};
> > +
> > +	my $version = $symbol_hash->{version};
> > +
> > +	next if !defined($version);
> > +
> > +	my $is_version_old = $version >= $MIN_VERSION
> > +	    && $version <= $MIN_VERSION + $warn_version_offset;
> > +
> > +	next if !$is_version_old;
> > +
> > +	# warning is formed in a way that uses as much information as possible,
> > +	# since keys of $symbol_hash aren't strictly required to be defined
> > +
> > +	my $warning = "\"$plugin\" defines symbol \"$symbol\"";
> > +
> > +	my ($message, $filename, $reftype, $line) = (
> > +	    $symbol_hash->{message},
> > +	    $symbol_hash->{filename},
> > +	    $symbol_hash->{reftype},
> > +	    $symbol_hash->{line},
> > +	);
> > +
> > +	if (defined($version)) {
> > +	    $warning .= " which may not be used anymore from API version $version onward"
> > +		. " (API version: @{[APIVER]}, minimum supported API version: $MIN_VERSION)";
> > +	}
> > +
> > +	$warning .= ": $message" if defined($message);
> > +
> > +	if (defined($filename)) {
> > +	    $warning .= "\nOriginally defined in $filename";
> > +
> > +	    $warning .= " as reftype '$reftype'" if defined($reftype);
> > +	    $warning .= " at line $line" if defined($line);
> > +	}
> > +
> > +	warn $warning, "\n\n";
> > +    }
> > +
> > +    return;
> > +}


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Max Carrara]

On Wed Feb 5, 2025 at 12:17 PM CET, Wolfgang Bumiller wrote:
> Overall thoughts - this is a bit much at once...
>
> 1) It doesn't build. The loader fails, claiming that `DirPlugin` is not
> derived from `PVE::Storage::Plugin`. Presumably because you only
> `require` it which does not set up the `@ISA`...
> (Followed by a bunch of "Plugin 'foo' is already loaded" errors)

Huh, really? That's interesting. I must've missed something then. Sorry
for the inconvenience!

>
> 2) I don't think we really need/want to rework the loading this much.
> We don't really gain much.

I perfectly understand; that's why I had sent this out as an RFC first
to gather some feedback -- had this lying around for a bit now and
didn't want to overcook.

>
> Using a plugin *list* instead of manually having a `use AllPlugins;`
> and `AllPlugins->register()` blocks might be worth it, but this is too
> much.
>
> But we definitely don't need a `wantarray`-using `sub` returning a list
> of standard plugins.
> Just use `my/our @PLUGINS = qw(The List);` please.
>
> 3) Apparently our style guide needs updating. While some things aren't
> explicit in there, they should be:
> Eg. you have a *lot* of postfix-`if` on multi-line expressions which
> should rather be regular prefix-if blocks.
>
> Also: `pveStorageDeprecateAt` - our style guide says `snake_case` for
> everything other than package names.

Ah, gotcha! Wasn't sure what to use here since it was an attr handler,
but thanks for the clarification.

>
> 4) You POD-document private `my sub`s. This does not fit our current
> code and I'm not convinced it is really worth it, but then there's a
> lot of code there I think could just be dropped (eg. the
> `get_standard_plugin_names()` sub is documented with POD while it
> should just be a list, while right above it there's a
> `$plugin_register_state` with not even a comment explaining its
> contents.
>
> 5) Generally: Please stop using `wantarray` where it has no actual
> benefit. A private `my sub` used exactly once really does not need
> this.
>

Regarding `wantarray`: Yeah we talked about this off-list wrt. the
PVE::Path series IIRC; the patches here are a bit older, so I hadn't
gotten rid of that pattern here yet.

>
> On Thu, Jan 30, 2025 at 03:51:18PM +0100, Max Carrara wrote:
> > RFC: Tighter API Control for Storage Plugins - v1
> > =================================================
> > 
> > Since this has been cooking for a while I've decided to send this in as
> > an RFC in order to get some early feedback in.
> > 
> > Note that this is quite experimental and also a little more complex;
> > I'll try my best to explain my reasoning for the changes in this RFC
> > below.
> > 
> > Any feedback on this is greatly appreciated!
> > 
> > Introduction
> > ------------
> > 
> > While working on a refactor of the Storage Plugin API, I've often hit
> > cases where I needed to move a subroutine around, but couldn't
> > confidently do so due to the code being rather old and the nature of
> > Perl as a language.
> > 
> > I had instances where things would spontaneously break here and there
> > after moving an inocuous-looking helper subroutine, due to it actually
> > being used in a different module / plugin, or worse, in certain cases in
> > a completely different package, because a plugin's helper sub ended
> > up being spontaneously used there.
> > 
> > To remedy this, I had originally added a helper subroutine for emitting
> > deprecation warnings. The plan back then was to leave the subroutines at
> > their original locations and have them call their replacement under the
> > hood while emitting a warning. Kind of like so:
> > 
> >   # PVE::Storage::SomePlugin
> > 
> >   sub some_helper_sub {
> >       my ($arg) = @_;
> > 
> >       # Emit deprecation warning here to hopefully catch any unexpected
> >       # callsites
> >       warn get_deprecation_warning(
> >           "PVE::Storage::Common::some_helper_sub"
> >       );
> > 
> >       # Call relocated helper here
> >       return PVE::Storage::Common::some_helper_sub($arg);
> >   }
> > 
> > This approach was decent-ish, but didn't *actually* guard against any
> > mishaps, unfortunately. With this approach, there is no guarantee that
> > the callsite will actually be caught, especially if e.g. a third-party
> > storage plugin was also using that subroutine and the plugin author
> > didn't bother checking or reading their CI logs.
> > 
> > What I instead wanted to have was some "strong guarantee" that a
> > subroutine absolutely cannot be used anymore after a certain point, e.g.
> > after a certain API version or similar.
>
> I don't think accidentally-public private helpers should be considered
> part of the API. We can just deprecate them immediately, remove them
> "soon". They aren't part of the `PVE::Storage`<->`Plugin` surface after
> all.

Hmm, fair. I wasn't sure what our stance on that exactly is, so I
dediced to be conservative here; as in: "If it's being used by someone
else, then it's already part of an API", if that makes sense.

Though, since we're fine with removing them, I'll just migrate them soon
and provide wrappers that emit a warning (or something) in case any
third-party modules are still using them. Once we do a major / minor
bump of PVE, we can remove the wrappers while not touching the storage
API{VER,AGE} (at least not for those helpers specifically).

>
> They may be using a private sub in `PVE::QemuServer` too - an attribute
> to warn on that (eg. just check `caller` against `__PACKAGE__`) should
> be "good enough" - a compile time solution would be way too much code.
> We don't need to write our own linters here.

Perhaps not our own linters, I agree, but IMO we should have *something*
that actually allows us to be a little bit more confident about
refactoring / migrating / tearing out Perl code.

I like what you mentioned in your second response to patch 3 [resp]
(and I also agree with your and Thomas's points there). I think what we
could have is an environment variable that perhaps doesn't (just) emit
warnings for deprecated things, but also logs them *in some other place*
so that we may continuously check whether deprecated things are still in
use somewhere.

For example, when running our services with an env var like
PVE_DEBUG_CHECK_DEPRECATIONS=1 (or whatever), we could log any usages of
deprecated stuff into some file like e.g. /var/log/pve/debug/deprecations.log.
Once we have better automated testing in place (e2e-testing etc.), we
could periodically probe that file and let it tell us where we're still
using deprecated subs etc.

Ideally, we could log those things as some kind of parsable format,
almost like some kind of runtime metric collection.

I know this sounds complicated and perhaps like it's "too much", but the
reason why I'm im advocating for *some* kind of mechanism like that is
because grepping through all of our repositories is sometimes just not
enough; it even bit me once when I was trying to remove an
inocuous-looking subroutine from our code [sub-removal], which didn't
show up anywhere when I had originally written that patch series.

Maybe to elaborate some more here: As you know, refactoring something in
Rust is quite easy (even fun) since the compiler and clippy just yell at
you in 99.999% of cases if you mess up. I tried to get Perl to do the
same thing here for me, but I agree with you that it's just too much
work and code for that to actually be effective (and tbf, I see now that
I should probably not try to make Perl act like Rust any further,
because it just won't ever do that equivalently anyways). Hence, do
those things at runtime instead and let the system tell us when we're
running debug builds.

It otherwise becomes very, very hard to make any changes *confidently*
if *somewhere* *something* could magically break because of some spooky
action at a distance.

So, I'll see if I can cook something up and toss you a couple more ideas
here and there off-list if you'd like; I agree with the points you made
and I think we should follow through on the idea of having a general
deprecation mechanism.

One last thing: I like the env var idea regarding plugin API checks [resp]
as well; I think I might work on that as some kind of standalone
feature.

Also, thanks for the thorough feedback! :) I know this is a lot, but
it's appreciated.

[resp]: https://lore.proxmox.com/pve-devel/cjiyfn6mrfhmxoxqoosyyh2csczf6jrc4uvuvyqmx75x56dus3@k6mvylmwd5pc/
[sub-removal]: https://git.proxmox.com/?p=pve-common.git;a=commit;h=d22ff1b644cc2ceb6d7ab98f232eab453a708ddf



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Fiona Ebner]

Am 05.02.25 um 16:20 schrieb Max Carrara:
> On Wed Feb 5, 2025 at 12:17 PM CET, Wolfgang Bumiller wrote:
>> I don't think accidentally-public private helpers should be considered
>> part of the API. We can just deprecate them immediately, remove them
>> "soon". They aren't part of the `PVE::Storage`<->`Plugin` surface after
>> all.
> 
> Hmm, fair. I wasn't sure what our stance on that exactly is, so I
> dediced to be conservative here; as in: "If it's being used by someone
> else, then it's already part of an API", if that makes sense.
> 
> Though, since we're fine with removing them, I'll just migrate them soon
> and provide wrappers that emit a warning (or something) in case any
> third-party modules are still using them. Once we do a major / minor
> bump of PVE, we can remove the wrappers while not touching the storage
> API{VER,AGE} (at least not for those helpers specifically).

I'd also err on the side of caution here. We never explicitly documented
what is and isn't part of the plugin API, so chances are that some
external plugins do make use of some such helpers. Removing them during
a minor release or without APIAGE reset will not be nice to plugin
authors. We'll likely do an APIAGE reset for PVE 9 in any case, so we
could just do the breaking change for such helpers then too.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Thomas Lamprecht]

Am 06.02.25 um 15:05 schrieb Fiona Ebner:
> Am 05.02.25 um 16:20 schrieb Max Carrara:
>> On Wed Feb 5, 2025 at 12:17 PM CET, Wolfgang Bumiller wrote:
>>> I don't think accidentally-public private helpers should be considered
>>> part of the API. We can just deprecate them immediately, remove them
>>> "soon". They aren't part of the `PVE::Storage`<->`Plugin` surface after
>>> all.
>> Hmm, fair. I wasn't sure what our stance on that exactly is, so I
>> dediced to be conservative here; as in: "If it's being used by someone
>> else, then it's already part of an API", if that makes sense.
>>
>> Though, since we're fine with removing them, I'll just migrate them soon
>> and provide wrappers that emit a warning (or something) in case any
>> third-party modules are still using them. Once we do a major / minor
>> bump of PVE, we can remove the wrappers while not touching the storage
>> API{VER,AGE} (at least not for those helpers specifically).
>
> I'd also err on the side of caution here. We never explicitly documented
> what is and isn't part of the plugin API, so chances are that some
> external plugins do make use of some such helpers. Removing them during
> a minor release or without APIAGE reset will not be nice to plugin
> authors. We'll likely do an APIAGE reset for PVE 9 in any case, so we
> could just do the breaking change for such helpers then too.

Do we really need to reset the Age? I.e. do we have strong reasons like
some security ones or big bug (data eating) potential?

As in general I'd try very hard to avoid resetting that, as it causes
a huge amount of fallout for our users. Refactoring and having a handful
of methods (which had close to zero maintenance cost) removed is hardly
a good reason.

And I'd also try hard to reduce, not increases warnings for regular users,
which cannot really do anything about them anyway but get annoying and
scary warnings spammed in their log. Instead, I'd rather make (warnings
for) those checks opt-in and document how they can be checked for in our
respective wiki [0], that way the storage plugin vendors that do care
can actually fix them by moving to the more modern API and potential
newer features and roll those fixes out without the user seeing any
impact.

[0]: https://pve.proxmox.com/wiki/Storage_Plugin_Development

Tangentially related more/stricter checks at runtime are really bogus,
we can only lose. If one is really affected by such "magical" perl
behavior then more test that get run at package assembly (build) time
and integration tests to cover interaction with downstream packages
is much more helpful. Avoiding refactoring just for the sake of it might
also help (yes, rust allows doing that fearlessly, no, that does _not_
mean it's a sufficient argument for doing so there too).


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Fiona Ebner]

Am 06.02.25 um 15:39 schrieb Thomas Lamprecht:
> Am 06.02.25 um 15:05 schrieb Fiona Ebner:
>> Am 05.02.25 um 16:20 schrieb Max Carrara:
>>> On Wed Feb 5, 2025 at 12:17 PM CET, Wolfgang Bumiller wrote:
>>>> I don't think accidentally-public private helpers should be considered
>>>> part of the API. We can just deprecate them immediately, remove them
>>>> "soon". They aren't part of the `PVE::Storage`<->`Plugin` surface after
>>>> all.
>>> Hmm, fair. I wasn't sure what our stance on that exactly is, so I
>>> dediced to be conservative here; as in: "If it's being used by someone
>>> else, then it's already part of an API", if that makes sense.
>>>
>>> Though, since we're fine with removing them, I'll just migrate them soon
>>> and provide wrappers that emit a warning (or something) in case any
>>> third-party modules are still using them. Once we do a major / minor
>>> bump of PVE, we can remove the wrappers while not touching the storage
>>> API{VER,AGE} (at least not for those helpers specifically).
>>
>> I'd also err on the side of caution here. We never explicitly documented
>> what is and isn't part of the plugin API, so chances are that some
>> external plugins do make use of some such helpers. Removing them during
>> a minor release or without APIAGE reset will not be nice to plugin
>> authors. We'll likely do an APIAGE reset for PVE 9 in any case, so we
>> could just do the breaking change for such helpers then too.
> 
> Do we really need to reset the Age? I.e. do we have strong reasons like
> some security ones or big bug (data eating) potential?

There are no such strong reasons, but we didn't have such strong reasons
last time either (IIRC changing snapshot parameter for export for btrfs
or something like that). I thought we need to do that on any breaking
change? We do have a few queued up already for the next reset. Hence my
suggestion to do it for PVE 9 so plugin authors can adapt together with
adapting for the new Debian release.

> 
> As in general I'd try very hard to avoid resetting that, as it causes
> a huge amount of fallout for our users. Refactoring and having a handful
> of methods (which had close to zero maintenance cost) removed is hardly
> a good reason.
> 
> And I'd also try hard to reduce, not increases warnings for regular users,
> which cannot really do anything about them anyway but get annoying and
> scary warnings spammed in their log. Instead, I'd rather make (warnings
> for) those checks opt-in and document how they can be checked for in our
> respective wiki [0], that way the storage plugin vendors that do care
> can actually fix them by moving to the more modern API and potential
> newer features and roll those fixes out without the user seeing any
> impact.

Sure, that's why I'm against doing it during a minor release or without
an APIAGE reset, because that will implicitly break plugins currently
using the methods rather than give the plugin authors an explicit heads-up.

> 
> [0]: https://pve.proxmox.com/wiki/Storage_Plugin_Development
> 
> Tangentially related more/stricter checks at runtime are really bogus,
> we can only lose. If one is really affected by such "magical" perl
> behavior then more test that get run at package assembly (build) time
> and integration tests to cover interaction with downstream packages
> is much more helpful. Avoiding refactoring just for the sake of it might
> also help (yes, rust allows doing that fearlessly, no, that does _not_
> mean it's a sufficient argument for doing so there too).



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Thomas Lamprecht]

Am 06.02.25 um 15:56 schrieb Fiona Ebner:
> There are no such strong reasons, but we didn't have such strong reasons
> last time either (IIRC changing snapshot parameter for export for btrfs
> or something like that). I thought we need to do that on any breaking
> change? We do have a few queued up already for the next reset. Hence my
> suggestion to do it for PVE 9 so plugin authors can adapt together with
> adapting for the new Debian release.

Yeah, last time caused some more fall-out than I expected, I have reduced
motivation to repeat that, especially as we grew quite a few more users
and also some new external storage plugins.

If nothing big comes up I'm fine with just keep increasing both VERSION
and AGE in lock-step as needed. If that becomes painful we should rework
our approach to be less sudden – we want short betas for $reasons, thus
plugin author do not get a long heads-up there at all – like e.g. using
a moving window to more gradually drop support for older versions, unlike
a full reset, ideally defining which will be removed already earlier (most
lenient would be a full major release, less might be fine, needs to be
thought out). We could use those two also to decide for what to warn and
for what to either output nothing or only very rarely some notice level log

We might also need to get a bit more strict with how we handle API breaks,
but as this is easier to adapt too as end-user are less likely to be
affected, as in, our UI still works, breakage happens normally at an
endpoint level instead of all or nothing like here with storage plugins,
which means likely breaking PVE interfacing with such storages completely,
which will not allow guests on those storages to run or backups to go
through, ...


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Fiona Ebner]

Am 07.02.25 um 08:17 schrieb Thomas Lamprecht:
> Am 06.02.25 um 15:56 schrieb Fiona Ebner:
>> There are no such strong reasons, but we didn't have such strong reasons
>> last time either (IIRC changing snapshot parameter for export for btrfs
>> or something like that). I thought we need to do that on any breaking
>> change? We do have a few queued up already for the next reset. Hence my
>> suggestion to do it for PVE 9 so plugin authors can adapt together with
>> adapting for the new Debian release.
> 
> Yeah, last time caused some more fall-out than I expected, I have reduced
> motivation to repeat that, especially as we grew quite a few more users
> and also some new external storage plugins.
> 
> If nothing big comes up I'm fine with just keep increasing both VERSION
> and AGE in lock-step as needed. If that becomes painful we should rework
> our approach to be less sudden – we want short betas for $reasons, thus
> plugin author do not get a long heads-up there at all – like e.g. using
> a moving window to more gradually drop support for older versions, unlike
> a full reset, ideally defining which will be removed already earlier (most
> lenient would be a full major release, less might be fine, needs to be
> thought out). We could use those two also to decide for what to warn and
> for what to either output nothing or only very rarely some notice level log
> 
> We might also need to get a bit more strict with how we handle API breaks,
> but as this is easier to adapt too as end-user are less likely to be
> affected, as in, our UI still works, breakage happens normally at an
> endpoint level instead of all or nothing like here with storage plugins,
> which means likely breaking PVE interfacing with such storages completely,
> which will not allow guests on those storages to run or backups to go
> through, ...

I just thought that in the context of a major release, many plugins will
need to adapt to the new underlying Debian environment and thus provide
new versions in any case. Doing breaking changes outside of a major
release bears bigger risk that users might miss new plugin versions
(e.g. installed the plugin once without configuring/checking for updates
later). With "full major release" which of the following do you mean:
1. deprecated in PVE N.x, dropped in PVE (N+1).x for the same x
2. deprecated in PVE N.x, still supported throughout PVE (N+1), dropped
with PVE (N+2).0

We also lose a little flexibility about how we can do breaking changes:
e.g. changing parameter order would require adding a new method,
changing the existing one to be a wrapper and then dropping the wrapper
at some point - not so bad I guess, but forces us to come up with new
names for those methods.

But I can see your point why a full APIAGE reset is bad and not doing
any is fine by me. The opt-in env-var or similar for deprecation
warnings seems like a good approach IMHO. Just need to communicate this
properly to plugin authors if we add it. The deadline for a breaking
change could then also be included in the warning message.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Thomas Lamprecht]

Am 07.02.25 um 10:59 schrieb Fiona Ebner:
> I just thought that in the context of a major release, many plugins will
> need to adapt to the new underlying Debian environment and thus provide

Not sure how much one needs to adapt here, our plugins IIRC never
required any adaption just due to doing a major Debian upgrade, being
written in a quite long-term stable interpreted language as perl after all,
as most external plugins are too.

Both external plugin examples linked in [0] actually just return the
$apiversion defined by pve-storage (if smaller than some max version
though), which is quite definitively to avoid annoying warnings as those
plugins can be used for more than one major release at the same time.
While these might not even be affected by the reset, it's IMO another
sign that the current mechanism is not ideal and a reset might be even
more dangerous in practice (sure, plugin authors fault to do some
questionable things, but I cannot really blame them tbh).

> new versions in any case. Doing breaking changes outside of a major
> release bears bigger risk that users might miss new plugin versions

I nowhere proposed doing such breaking changes outside of a major
release, just that they should not reset AGE to zero as we did last
time.

> (e.g. installed the plugin once without configuring/checking for updates
> later). With "full major release" which of the following do you mean:
> 1. deprecated in PVE N.x, dropped in PVE (N+1).x for the same x
> 2. deprecated in PVE N.x, still supported throughout PVE (N+1), dropped
> with PVE (N+2).0

3. no explicit "future" deprecation period, but on a new major version
reduce the API age such (or some other operation resulting in basically
the same thing, depending on what versioning we then use) that all API
versions that happened before PVE ($N - 1) become unsupported and any
compat code can be dropped. Then we can add a check to the pve($N-1)to$N
checker script that tests if any installed storage plugin would become
unsupported.

This would be a relatively simple deprecation process modification and
can be adapted quite quickly before cutting any new major release, if
really needed that is. By default, it would avoid churn from unconditional
upfront deprecation, as one can decide for each major release if it's
actually worth it.

btw. Printing some notice for plugin with outdated version in the XtoY
checker scripts might be a good idea in any way, that's not as intrusive
as warning on every module load and makes it more likely that users check
if their plugin got a new release they might want to update.

> We also lose a little flexibility about how we can do breaking changes:
> e.g. changing parameter order would require adding a new method,
> changing the existing one to be a wrapper and then dropping the wrapper
> at some point - not so bad I guess, but forces us to come up with new
> names for those methods.


Yeah sure, albeit I really have a hard time in seeing the (slight) loss
in flexibility as actual problem, causing churn for all plugin users and
devs certainly is one though.
And as said, when this actually becomes a problem and maintenance burden
we can come up with solutions, if the pain is big enough and a lot of stuff
accumulated we can also to a (partial) AGE reset, having some more cruft
added during that major release cycle is not really a problem that we
can avoid either way.

And I know it was an example, but just reordering parameters certainly
does not qualify for that. And if the old name really was perfect then
there's always the good ol'reliable addition of a 2, or N+1, at the end
of the name, that's honest and telling as it directly conveys that there
is/was an N-1 method.

> But I can see your point why a full APIAGE reset is bad and not doing
> any is fine by me. The opt-in env-var or similar for deprecation
> warnings seems like a good approach IMHO. Just need to communicate this
> properly to plugin authors if we add it. The deadline for a breaking
> change could then also be included in the warning message.

The wiki [0] should for now become the central place for docs, I made
the initial draft in a 

[0]: https://pve.proxmox.com/wiki/Storage_Plugin_Development


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control for Storage Plugins

[Fiona Ebner]

Am 07.02.25 um 12:57 schrieb Thomas Lamprecht:
> Am 07.02.25 um 10:59 schrieb Fiona Ebner:
>> I just thought that in the context of a major release, many plugins will
>> need to adapt to the new underlying Debian environment and thus provide
> 
> Not sure how much one needs to adapt here, our plugins IIRC never
> required any adaption just due to doing a major Debian upgrade, being
> written in a quite long-term stable interpreted language as perl after all,
> as most external plugins are too.

True for the Perl part of the plugins. I was thinking about CLI
tools/binaries/libraries that are used by the plugins.

> Both external plugin examples linked in [0] actually just return the
> $apiversion defined by pve-storage (if smaller than some max version
> though), which is quite definitively to avoid annoying warnings as those
> plugins can be used for more than one major release at the same time.
> While these might not even be affected by the reset, it's IMO another
> sign that the current mechanism is not ideal and a reset might be even
> more dangerous in practice (sure, plugin authors fault to do some
> questionable things, but I cannot really blame them tbh).
> 
>> new versions in any case. Doing breaking changes outside of a major
>> release bears bigger risk that users might miss new plugin versions
> 
> I nowhere proposed doing such breaking changes outside of a major
> release, just that they should not reset AGE to zero as we did last
> time.

Sorry, I didn't mean to imply you did. I was still thinking about
Wolfgang's "soon" from

> I don't think accidentally-public private helpers should be considered
> part of the API. We can just deprecate them immediately, remove them
> "soon". They aren't part of the `PVE::Storage`<->`Plugin` surface after
> all.

although that also doesn't explicitly say "outside of a major release"
of course ;)

> 
>> (e.g. installed the plugin once without configuring/checking for updates
>> later). With "full major release" which of the following do you mean:
>> 1. deprecated in PVE N.x, dropped in PVE (N+1).x for the same x
>> 2. deprecated in PVE N.x, still supported throughout PVE (N+1), dropped
>> with PVE (N+2).0
> 
> 3. no explicit "future" deprecation period, but on a new major version
> reduce the API age such (or some other operation resulting in basically
> the same thing, depending on what versioning we then use) that all API
> versions that happened before PVE ($N - 1) become unsupported and any
> compat code can be dropped. Then we can add a check to the pve($N-1)to$N
> checker script that tests if any installed storage plugin would become
> unsupported.
> 
> This would be a relatively simple deprecation process modification and
> can be adapted quite quickly before cutting any new major release, if
> really needed that is. By default, it would avoid churn from unconditional
> upfront deprecation, as one can decide for each major release if it's
> actually worth it.
> 
> btw. Printing some notice for plugin with outdated version in the XtoY
> checker scripts might be a good idea in any way, that's not as intrusive
> as warning on every module load and makes it more likely that users check
> if their plugin got a new release they might want to update.

Totally fine by me, as is everything below :)

>> We also lose a little flexibility about how we can do breaking changes:
>> e.g. changing parameter order would require adding a new method,
>> changing the existing one to be a wrapper and then dropping the wrapper
>> at some point - not so bad I guess, but forces us to come up with new
>> names for those methods.
> 
> 
> Yeah sure, albeit I really have a hard time in seeing the (slight) loss
> in flexibility as actual problem, causing churn for all plugin users and
> devs certainly is one though.
> And as said, when this actually becomes a problem and maintenance burden
> we can come up with solutions, if the pain is big enough and a lot of stuff
> accumulated we can also to a (partial) AGE reset, having some more cruft
> added during that major release cycle is not really a problem that we
> can avoid either way.
> 
> And I know it was an example, but just reordering parameters certainly
> does not qualify for that. And if the old name really was perfect then
> there's always the good ol'reliable addition of a 2, or N+1, at the end
> of the name, that's honest and telling as it directly conveys that there
> is/was an N-1 method.
> 
>> But I can see your point why a full APIAGE reset is bad and not doing
>> any is fine by me. The opt-in env-var or similar for deprecation
>> warnings seems like a good approach IMHO. Just need to communicate this
>> properly to plugin authors if we add it. The deadline for a breaking
>> change could then also be included in the warning message.
> 
> The wiki [0] should for now become the central place for docs, I made
> the initial draft in a 
> 
> [0]: https://pve.proxmox.com/wiki/Storage_Plugin_Development



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel