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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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