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.carr...@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
