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 AFBF81FF2C8 for ; Wed, 17 Jul 2024 11:41:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B583538507; Wed, 17 Jul 2024 11:42:01 +0200 (CEST) From: Max Carrara To: pve-devel@lists.proxmox.com Date: Wed, 17 Jul 2024 11:40:14 +0200 Message-Id: <20240717094034.124857-17-m.carrara@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240717094034.124857-1-m.carrara@proxmox.com> References: <20240717094034.124857-1-m.carrara@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.030 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 Subject: [pve-devel] [RFC pve-storage 16/36] plugin: dir: factor storage methods into separate common subs 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" This creates new helper subroutines in the `Common` module in order to remove other plugins' dependency on `DirPlugin` in the future. The methods are changed to call these helpers instead. Simultaneously, emit a `warn`ing if deprecated methods / functions are being used instead of just relying on a comment in the source code. The new helper functions are also fully documented and use prototyes. Signed-off-by: Max Carrara --- src/PVE/Storage/Common.pm | 201 +++++++++++++++++++++++++++++++++++ src/PVE/Storage/DirPlugin.pm | 104 +++++------------- 2 files changed, 227 insertions(+), 78 deletions(-) diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm index 0ca0db1..3cc3c37 100644 --- a/src/PVE/Storage/Common.pm +++ b/src/PVE/Storage/Common.pm @@ -3,13 +3,21 @@ package PVE::Storage::Common; use strict; use warnings; +use Encode qw(decode encode); +use POSIX; + use PVE::JSONSchema; +use PVE::Tools; use parent qw(Exporter); our @EXPORT_OK = qw( get_deprecation_warning storage_parse_is_mountpoint + storage_dir_get_volume_notes + storage_dir_update_volume_notes + storage_dir_get_volume_attribute + storage_dir_update_volume_attribute ); =pod @@ -91,4 +99,197 @@ sub storage_parse_is_mountpoint : prototype($) { return $is_mp; # contains a path } +# FIXME move into 'storage_dir_get_volume_attribute' when removing +# 'storage_dir_get_volume_notes' +my $get_volume_notes_impl = sub { + my ($class, $scfg, $storeid, $volname, $timeout) = @_; + + my ($vtype) = $class->parse_volname($volname); + return if $vtype ne 'backup'; + + my $path = $class->filesystem_path($scfg, $volname); + $path .= $class->SUPER::NOTES_EXT; + + if (-f $path) { + my $data = PVE::Tools::file_get_contents($path); + return eval { decode('UTF-8', $data, 1) } // $data; + } + + return ''; +}; + +=pod + +=head3 storage_dir_get_volume_notes + + $notes = storage_dir_get_volume_notes($class, $scfg, $storeid, $volname, $timeout) + +B This helper is deprecated in favour of +C> and will be removed in the future. + +This is a general implementation of C> +that may be used by storage plugins with a directory-based storage. It is therefore +useful for something like L, which +(network stuff aside) also behaves like a directory. + +Returns the notes from a volume named C<$volname>. The volume is expected to +belong to the storage with ID C<$storeid> that has the configuration C<$scfg>. +The storage must in turn stem from the storage plugin C<$class>. + +The C<$timeout> parameter has no effect in this case. + +=cut + +sub storage_dir_get_volume_notes : prototype($$$$$) { + warn get_deprecation_warning( + __PACKAGE__ . "::storage_dir_get_volume_attribute" + ); + + my ($class, $scfg, $storeid, $volname, $timeout) = @_; + + return $get_volume_notes_impl->($class, $scfg, $storeid, $volname, $timeout); +} + +# FIXME move into 'storage_dir_update_volume_attribute' when removing +# 'storage_dir_update_volume_notes' +my $update_volume_notes_impl = sub { + my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; + + my ($vtype) = $class->parse_volname($volname); + die "only backups can have notes\n" if $vtype ne 'backup'; + + my $path = $class->filesystem_path($scfg, $volname); + $path .= $class->SUPER::NOTES_EXT; + + if (defined($notes) && $notes ne '') { + my $encoded = encode('UTF-8', $notes); + PVE::Tools::file_set_contents($path, $encoded); + } else { + unlink $path or $! == ENOENT or die "could not delete notes - $!\n"; + } + return; +}; + + +=pod + +=head3 storage_dir_update_volume_notes + + $notes = storage_dir_update_volume_notes($class, $scfg, $storeid, $volname, $notes, $timeout) + +B This helper is deprecated in favour of +C> and will be removed in the future. + +This is a general implementation of C> +that may be used by storage plugins with a directory-based storage. It is therefore +useful for something like L, which +(network stuff aside) also behaves like a directory. + +Sets the notes of a volume named C<$volname> to C<$notes>. The volume is +expected to belong to the storage with ID C<$storeid> that has the configuration +C<$scfg>. The storage must in turn stem from the storage plugin C<$class>. + +The C<$timeout> parameter has no effect in this case. + +=cut + +sub storage_dir_update_volume_notes : prototype($$$$$$) { + warn get_deprecation_warning( + __PACKAGE__ . "storage_dir_update_volume_attribute" + ); + + my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; + + return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $notes, $timeout); +} + +=pod + +=head3 storage_dir_get_volume_attribute + + $attr_value = storage_dir_get_volume_attribute($class, $scfg, $storeid, $volname, $attribute) + +This is a general implementation of C> +that may be used by storage plugins with a directory-based storage. It is therefore +useful for something like L, which +(network stuff aside) also behaves like a directory. + +Returns the value of the attribute named C<$attribute> from a volume named +C<$volname>. The volume is expected to belong to the storage with ID C<$storeid> +that has the configuration C<$scfg>. The storage must in turn stem from the +storage plugin C<$class>. + +Returns C if the attribute is not supported by the plugin. + +Dies if it encounters an error while getting the attribute. + +=cut + +sub storage_dir_get_volume_attribute : prototype($$$$$) { + my ($class, $scfg, $storeid, $volname, $attribute) = @_; + + if ($attribute eq 'notes') { + return $get_volume_notes_impl->($class, $scfg, $storeid, $volname); + } + + my ($vtype) = $class->parse_volname($volname); + return if $vtype ne 'backup'; + + if ($attribute eq 'protected') { + my $path = $class->filesystem_path($scfg, $volname); + return -e PVE::Storage::protection_file_path($path) ? 1 : 0; + } + + return; +} + +=pod + +=head3 storage_dir_update_volume_attribute + + storage_dir_update_volume_attribute($class, $scfg, $storeid, $volname, $attribute, $value) + +This is a general implementation of C> +that may be used by storage plugins with a directory-based storage. It is therefore +useful for something like L, which +(network stuff aside) also behaves like a directory. + +Sets the value of the attribute named C<$attribute> from a volume named +C<$volname> to C<$value>. The volume is expected to belong to the storage with +ID C<$storeid> that has the configuration C<$scfg>. The storage must in turn +stem from the storage plugin C<$class>. + +=cut + +sub storage_dir_update_volume_attribute : prototype($$$$$$) { + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_; + + if ($attribute eq 'notes') { + return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $value); + } + + my ($vtype) = $class->parse_volname($volname); + die "only backups support attribute '$attribute'\n" if $vtype ne 'backup'; + + if ($attribute eq 'protected') { + my $path = $class->filesystem_path($scfg, $volname); + my $protection_path = PVE::Storage::protection_file_path($path); + + return if !((-e $protection_path) xor $value); # protection status already correct + + if ($value) { + my $fh = IO::File->new($protection_path, O_CREAT, 0644) + or die "unable to create protection file '$protection_path' - $!\n"; + close($fh); + } else { + unlink $protection_path or $! == ENOENT + or die "could not delete protection file '$protection_path' - $!\n"; + } + + return; + } + + die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n"; +} + 1; diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm index ac0d365..f6e1d73 100644 --- a/src/PVE/Storage/DirPlugin.pm +++ b/src/PVE/Storage/DirPlugin.pm @@ -11,6 +11,10 @@ use POSIX; use PVE::Storage::Common qw( get_deprecation_warning storage_parse_is_mountpoint + storage_dir_get_volume_notes + storage_dir_update_volume_notes + storage_dir_get_volume_attribute + storage_dir_update_volume_attribute ); use PVE::Storage::Common::Path; use PVE::Storage::Plugin; @@ -106,104 +110,48 @@ sub parse_is_mountpoint { return storage_parse_is_mountpoint(@_); } -# FIXME move into 'get_volume_attribute' when removing 'get_volume_notes' -my $get_volume_notes_impl = sub { - my ($class, $scfg, $storeid, $volname, $timeout) = @_; - - my ($vtype) = $class->parse_volname($volname); - return if $vtype ne 'backup'; - - my $path = $class->filesystem_path($scfg, $volname); - $path .= $class->SUPER::NOTES_EXT; - - if (-f $path) { - my $data = PVE::Tools::file_get_contents($path); - return eval { decode('UTF-8', $data, 1) } // $data; - } - - return ''; -}; - # FIXME remove on the next APIAGE reset. # Deprecated, use get_volume_attribute instead. sub get_volume_notes { - my ($class, $scfg, $storeid, $volname, $timeout) = @_; - return $get_volume_notes_impl->($class, $scfg, $storeid, $volname, $timeout); -} - -# FIXME move into 'update_volume_attribute' when removing 'update_volume_notes' -my $update_volume_notes_impl = sub { - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; - - my ($vtype) = $class->parse_volname($volname); - die "only backups can have notes\n" if $vtype ne 'backup'; + warn get_deprecation_warning( + "PVE::Storage::Common::storage_dir_get_volume_notes" + ); - my $path = $class->filesystem_path($scfg, $volname); - $path .= $class->SUPER::NOTES_EXT; + my ($class, $scfg, $storeid, $volname, $timeout) = @_; - if (defined($notes) && $notes ne '') { - my $encoded = encode('UTF-8', $notes); - PVE::Tools::file_set_contents($path, $encoded); - } else { - unlink $path or $! == ENOENT or die "could not delete notes - $!\n"; - } - return; -}; + return storage_dir_get_volume_notes( + $class, $scfg, $storeid, $volname, $timeout + ); +} # FIXME remove on the next APIAGE reset. # Deprecated, use update_volume_attribute instead. sub update_volume_notes { + warn get_deprecation_warning( + "PVE::Storage::Common::storage_dir_update_volume_notes" + ); + my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; - return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $notes, $timeout); + + return storage_dir_update_volume_notes( + $class, $scfg, $storeid, $volname, $notes, $timeout + ); } sub get_volume_attribute { my ($class, $scfg, $storeid, $volname, $attribute) = @_; - if ($attribute eq 'notes') { - return $get_volume_notes_impl->($class, $scfg, $storeid, $volname); - } - - my ($vtype) = $class->parse_volname($volname); - return if $vtype ne 'backup'; - - if ($attribute eq 'protected') { - my $path = $class->filesystem_path($scfg, $volname); - return -e PVE::Storage::protection_file_path($path) ? 1 : 0; - } - - return; + return storage_dir_get_volume_attribute( + $class, $scfg, $storeid, $volname, $attribute + ); } sub update_volume_attribute { my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_; - if ($attribute eq 'notes') { - return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $value); - } - - my ($vtype) = $class->parse_volname($volname); - die "only backups support attribute '$attribute'\n" if $vtype ne 'backup'; - - if ($attribute eq 'protected') { - my $path = $class->filesystem_path($scfg, $volname); - my $protection_path = PVE::Storage::protection_file_path($path); - - return if !((-e $protection_path) xor $value); # protection status already correct - - if ($value) { - my $fh = IO::File->new($protection_path, O_CREAT, 0644) - or die "unable to create protection file '$protection_path' - $!\n"; - close($fh); - } else { - unlink $protection_path or $! == ENOENT - or die "could not delete protection file '$protection_path' - $!\n"; - } - - return; - } - - die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n"; + return storage_dir_update_volume_attribute( + $class, $scfg, $storeid, $volname, $attribute, $value + ); } sub status { -- 2.39.2 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel