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