From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id DC3421FF15C for ; Wed, 5 Feb 2025 12:20:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C68A78F66; Wed, 5 Feb 2025 12:20:10 +0100 (CET) Date: Wed, 5 Feb 2025 12:20:04 +0100 From: Wolfgang Bumiller To: Max Carrara Message-ID: References: <20250130145124.317745-1-m.carrara@proxmox.com> <20250130145124.317745-2-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20250130145124.317745-2-m.carrara@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.084 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [plugin.pm, common.pm, gnu.org, version.pm, lvmplugin.pm, dirplugin.pm, storage.pm] Subject: Re: [pve-devel] [RFC v1 pve-storage 1/6] version: introduce PVE::Storage::Version X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion 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" On Thu, Jan 30, 2025 at 03:51:19PM +0100, Max Carrara wrote: > The purpose of this module is to separate the handling of the Storage > API's version from the remaining code while also providing additional > mechanisms to handle changes to the Storage API more gracefully. > > The first such mechanism is the `pveStorageDeprecateAt` attribute, > which, when added to a subroutine, marks the subroutine as deprecated > at a specific version. Should the lowest supported API version > increase beyond the version passed to the attribute, the module will > fail to compile (during Perl's CHECK phase). > > This provides a more robust mechanism instead of having to rely on > "FIXME" comments or similar. > > Unfortunately attributes can't conveniently be exported using the > `Exporter` module or similar mechanisms, and they can neither be > referenced via their fully qualified path. The attribute's name is > therefore intentionally made long in order to not conflict with any > other possible attributes in the UNIVERSAL namespace. > > Signed-off-by: Max Carrara > --- > src/PVE/Storage.pm | 10 +-- > src/PVE/Storage/Makefile | 1 + > src/PVE/Storage/Version.pm | 180 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 185 insertions(+), 6 deletions(-) > create mode 100644 src/PVE/Storage/Version.pm > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index 3b4f041..df4d62f 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -24,6 +24,8 @@ use PVE::RPCEnvironment; > use PVE::SSHInfo; > use PVE::RESTEnvironment qw(log_warn); > > +use PVE::Storage::Version; > + > use PVE::Storage::Plugin; > use PVE::Storage::DirPlugin; > use PVE::Storage::LVMPlugin; > @@ -41,12 +43,8 @@ use PVE::Storage::PBSPlugin; > use PVE::Storage::BTRFSPlugin; > use PVE::Storage::ESXiPlugin; > > -# Storage API version. Increment it on changes in storage API interface. > -use constant APIVER => 10; > -# Age is the number of versions we're backward compatible with. > -# This is like having 'current=APIVER' and age='APIAGE' in libtool, > -# see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html > -use constant APIAGE => 1; > +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']; > > diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile > index ce3fd68..5315f69 100644 > --- a/src/PVE/Storage/Makefile > +++ b/src/PVE/Storage/Makefile > @@ -1,5 +1,6 @@ > SOURCES= \ > Common.pm \ > + Version.pm \ > Plugin.pm \ > DirPlugin.pm \ > LVMPlugin.pm \ > diff --git a/src/PVE/Storage/Version.pm b/src/PVE/Storage/Version.pm > new file mode 100644 > index 0000000..a9216c9 > --- /dev/null > +++ b/src/PVE/Storage/Version.pm > @@ -0,0 +1,180 @@ > +=head1 NAME > + > +C - Storage API Version Management > + > +=head1 DESCRIPTION > + > +This module is concerned with tracking and managing the version of the Storage > +API interface C>. Furthermore, this module also provides > +a mechanism to I subroutines that are part of the Storage API via a > +custom I called C>>. > + > +For more information on this mechanism, see L below. > + > +B Importing this module will define the C>> > +attribute inside the C (global) namespace. Ensure that no other > +symbols have the same name as this attribute in order to avoid conflicts. > + > +=cut > + > +package PVE::Storage::Version; > + > +use strict; > +use warnings; > + > +use v5.36; ^ activates signatures, use them. > + > +use Attribute::Handlers; > +use Carp; > +use Scalar::Util qw(reftype); > + > +# NOTE: This module must not import PVE::Storage or any of its submodules, > +# either directly or transitively. > +# > +# Otherwise there's a risk of introducing nasty cyclical imports and/or causing > +# obscure errors during compile time, which may be very hard to debug. > + > +=head1 CONSTANTS > + > +=head3 APIVER > + > +The version of the Storage API. > + > +This version must be incremented if changes to the C> > +API interface are made. > + > +=head3 APIAGE > + > +The API age is the number of versions we're backward compatible with. > + > +This is similar to having C and C in C. > +For more information, see L. > + > +Generally, you'll want to increment this when incrementing C>>. > +Resetting C to C<0> means that backward compatibility is broken and should > +only be done when appropriate (e.g. during a release of a new major version of PVE). > + > +=cut > + > +use constant APIVER => 10; > +use constant APIAGE => 1; > + > +my $MIN_VERSION = (APIVER - APIAGE); > + > + > +=head1 DEPRECATING SUBROUTINES > + > +Upon importing this module, the C>> > +attribute becomes available. This attribute ought to be used similar to how > +prototypes are used in Perl and requires a C and a C key to > +be passed: > + > + sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") { > + # [...] > + } > + > +Most likely you'll need more space for the C, in which case the > +attribute may simply span multiple lines: > + > + sub some_sub : pveStorageDeprecateAt( > + version => 42, > + message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit." > + . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium" > + . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl" > + . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel" > + . " suscipit lorem varius. Mauris et erat posuere, gravida dui" > + . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet," > + . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id" > + . " diam venenatis, nec placerat felis viverra.", > + ) { > + # [...] > + } One example with "..." as message should be good enough. > + > +As soon as the passed C is below the I > +(C>> minus C>>), the attribute raises > +an exception during the C phase of the Perl interpreter and also displays > +the passed C. This happens before any runtime code is executed. > + > +Should the passed C be equal to the lowest supported version, a warning > +containing the passed C is emitted whenever the subroutine is called > +instead. This ensures that implementors of custom plugins are made aware of > +subroutines being deprecated, should there be any in use. > + > +The C>> attribute > +may only be used inside C> or its submodules and throws an error > +if used elsewhere. > + > +This mechanism's sole purpose is to enforce deprecation of subroutines during > +"compile time", forcing the subroutine to be removed once the API version is > +incremented too far or the API age is reset. > + > +=head1 ATTRIBUTES > + > +=head3 UNIVERSAL::pveStorageDeprecateAt > + > +This attribute marks subroutines as deprecated at a specific C and will > +emit an error with the given C if the lowest supported Storage API > +version exceeds the C passed to the attribute. > + > + sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") { > + # [...] > + } > + > + sub some_sub : pveStorageDeprecateAt( > + version => 42, > + message => "Lorem ipsum dolor sit amet, consectetur adipiscing elit." > + . " Vestibulum ac placerat arcu. Nulla eget risus sed eros pretium" > + . " tempus in sed purus. Mauris iaculis ligula justo, in facilisis nisl" > + . " maximus at. Aliquam bibendum sapien ut turpis scelerisque, vel" > + . " suscipit lorem varius. Mauris et erat posuere, gravida dui" > + . " porttitor, faucibus mauris. Phasellus ultricies ante aliquet," > + . " luctus lectus pretium, lobortis arcu. Aliquam fringilla libero id" > + . " diam venenatis, nec placerat felis viverra.", > + ) { > + # [...] > + } > + > +Should the passed C be equal to the currently lowest supported API > +version, a warning containing the passed C is emitted instead. > + > +=cut > + > +sub UNIVERSAL::pveStorageDeprecateAt : ATTR(CODE,CHECK) { > + my ($package, $symbol, $referent, $attr_name, $attr_data, $phase, $filename, $line) = @_; > + > + confess "'$attr_name' attribute may only be used inside PVE::Storage and its submodules" > + if $package !~ m/^PVE::Storage/; (Could end in `($|:)`, otherwise this allows `PVE::StorageNotReally`) > + > + my $data = { $attr_data->@* }; > + > + my ($version, $message) = ($data->{version}, $data->{message}); > + my $reftype = reftype($referent); > + > + confess "Referent is not a reference" > + . " -- '$attr_name' attribute may only be used for subroutines" > + if !defined($reftype); ^ Style issues mentioned in my cover letter reply. > + > + confess "Unexpected reftype '$reftype'" > + . " -- '$attr_name' attribute may only be used for subroutines" > + if !($reftype eq 'CODE' || $reftype eq 'ANON'); > + > + confess "Either version or message not defined" > + . " -- usage: $attr_name(version => 42, message => \"...\")" > + if !defined($version) || !defined($message); > + > + confess "Storage API version '$version' not supported: $message" > + if $version < $MIN_VERSION; > + > + no warnings 'redefine'; > + *$symbol = sub { > + my $symbol_name = *{$symbol}{"NAME"}; > + carp "Warning: subroutine '$symbol_name' is deprecated - $message" > + if $version == $MIN_VERSION; > + $referent->(@_); > + }; > + > + return; > +} > + > + > +1; > -- > 2.39.5 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel