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 <m.carr...@proxmox.com> > --- > 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<PVE::Storage::Version> - Storage API Version Management > + > +=head1 DESCRIPTION > + > +This module is concerned with tracking and managing the version of the > Storage > +API interface C<L<PVE::Storage::Plugin>>. Furthermore, this module also > provides > +a mechanism to I<deprecate> subroutines that are part of the Storage API via > a > +custom I<attribute> called C<L<< > pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>>. > + > +For more information on this mechanism, see L</"DEPRECATING SUBROUTINES"> > below. > + > +B<NOTE:> Importing this module will define the C<L<< > pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt() >>> > +attribute inside the C<UNIVERSAL> (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<L<PVE::Storage::Plugin>> > +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<current='APIVER'> and C<age='APIAGE'> in > C<libtool>. > +For more information, see > L<here|https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html>. > + > +Generally, you'll want to increment this when incrementing C<L<< > APIVER|/APIVER >>>. > +Resetting C<APIAGE> 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<L<< > pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>> > +attribute becomes available. This attribute ought to be used similar to how > +prototypes are used in Perl and requires a C<version> and a C<message> key to > +be passed: > + > + sub some_sub : pveStorageDeprecateAt(version => 42, message => "...") { > + # [...] > + } > + > +Most likely you'll need more space for the C<message>, 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<version> is below the I<lowest supported version> > +(C<L<< APIVER|/APIVER >>> minus C<L<< APIAGE|/APIAGE >>>), the attribute > raises > +an exception during the C<CHECK> phase of the Perl interpreter and also > displays > +the passed C<message>. This happens before any runtime code is executed. > + > +Should the passed C<version> be equal to the lowest supported version, a > warning > +containing the passed C<message> 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<L<< pveStorageDeprecateAt|/UNIVERSAL::pveStorageDeprecateAt >>> > attribute > +may only be used inside C<L<PVE::Storage>> 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<version> and > will > +emit an error with the given C<message> if the lowest supported Storage API > +version exceeds the C<version> 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<version> be equal to the currently lowest supported API > +version, a warning containing the passed C<message> 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
