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
