[pve-devel] [RFC v1 pve-storage 3/6] introduce compile-time checks for prohibited sub overrides
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Feb 5 12:55:16 CET 2025
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 at proxmox.com>
> > Suggested-by: Wolfang Bumiller <w.bumiller at 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;
> > +}
More information about the pve-devel
mailing list