From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 57A6B1FF15C
for <inbox@lore.proxmox.com>; Wed, 5 Feb 2025 12:17:29 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 50A5B8F1F;
Wed, 5 Feb 2025 12:17:26 +0100 (CET)
Date: Wed, 5 Feb 2025 12:17:23 +0100
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Max Carrara <m.carrara@proxmox.com>
Message-ID: <7pmkq3by7xrcxr6wku6wvrpoa52xi3q7k5br2ztkwmmvhggyxz@h3vx353ekvky>
References: <20250130145124.317745-1-m.carrara@proxmox.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <20250130145124.317745-1-m.carrara@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.083 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [RFC v1 pve-storage 0/6] RFC: Tighter API Control
for Storage Plugins
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: pve-devel@lists.proxmox.com
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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