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 DC3421FF15C
for <inbox@lore.proxmox.com>; 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 <w.bumiller@proxmox.com>
To: Max Carrara <m.carrara@proxmox.com>
Message-ID: <zzytq4x5e3hr37dbzxoragwtm2zv6aqmdrfnb3iynk5ogew3ua@jo5nodbuumjq>
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 <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>
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.carrara@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