From f1de828166a95d938828ea8539e6de666c9644bc Mon Sep 17 00:00:00 2001 From: Fabian Ebner Date: Thu, 30 Sep 2021 13:42:06 +0200 Subject: [PATCH] add generalized functions to manage volume attributes replacing the ones for handling notes. To ensure backwards compatibility with external plugins, all plugins that do not just call another implementation need to call $class->{get, update}_volume_notes when the attribute is 'notes' to catch any derived implementations. This is mainly done to avoid the need to add new methods every time a new attribute is added. Not adding a timeout parameter like the notes functions have, because it was not used and can still be added if it ever is needed in the future. For get_volume_attribute, undef will indicate that the attribute is not supported. This makes it possible to distinguish "not supported" from "error getting the attribute", which is useful when the attribute is important for an operation. For example, free_image checking for protection (introduced in a later patch) can abort if getting the 'protected' attribute fails. Suggested-by: Thomas Lamprecht Signed-off-by: Fabian Ebner --- PVE/API2/Storage/Content.pm | 7 ++++--- PVE/Storage.pm | 12 ++++++------ PVE/Storage/BTRFSPlugin.pm | 4 ++-- PVE/Storage/CIFSPlugin.pm | 13 +++++++++++++ PVE/Storage/CephFSPlugin.pm | 12 ++++++++++++ PVE/Storage/DirPlugin.pm | 24 ++++++++++++++++++++++++ PVE/Storage/NFSPlugin.pm | 13 +++++++++++++ PVE/Storage/PBSPlugin.pm | 24 ++++++++++++++++++++++++ PVE/Storage/Plugin.pm | 36 ++++++++++++++++++++++++++++++++++++ 9 files changed, 134 insertions(+), 11 deletions(-) diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm index 4d0ceb6..b3dc593 100644 --- a/PVE/API2/Storage/Content.pm +++ b/PVE/API2/Storage/Content.pm @@ -321,11 +321,12 @@ __PACKAGE__->register_method ({ format => $format, }; - # not all storages/types support notes, so ignore errors here + # keep going if fetching an optional attribute fails eval { - my $notes = PVE::Storage::get_volume_notes($cfg, $volid); + my $notes = PVE::Storage::get_volume_attribute($cfg, $volid, 'notes'); $entry->{notes} = $notes if defined($notes); }; + warn $@ if $@; return $entry; }}); @@ -371,7 +372,7 @@ __PACKAGE__->register_method ({ PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $volid); if (exists $param->{notes}) { - PVE::Storage::update_volume_notes($cfg, $volid, $param->{notes}); + PVE::Storage::update_volume_attribute($cfg, $volid, 'notes', $param->{notes}); } return undef; diff --git a/PVE/Storage.pm b/PVE/Storage.pm index bc477c1..6d0aa25 100755 --- a/PVE/Storage.pm +++ b/PVE/Storage.pm @@ -215,24 +215,24 @@ sub file_size_info { return PVE::Storage::Plugin::file_size_info($filename, $timeout); } -sub get_volume_notes { - my ($cfg, $volid, $timeout) = @_; +sub get_volume_attribute { + my ($cfg, $volid, $attribute) = @_; my ($storeid, $volname) = parse_volume_id($volid); my $scfg = storage_config($cfg, $storeid); my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); - return $plugin->get_volume_notes($scfg, $storeid, $volname, $timeout); + return $plugin->get_volume_attribute($scfg, $storeid, $volname, $attribute); } -sub update_volume_notes { - my ($cfg, $volid, $notes, $timeout) = @_; +sub update_volume_attribute { + my ($cfg, $volid, $attribute, $value) = @_; my ($storeid, $volname) = parse_volume_id($volid); my $scfg = storage_config($cfg, $storeid); my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); - $plugin->update_volume_notes($scfg, $storeid, $volname, $notes, $timeout); + return $plugin->update_volume_attribute($scfg, $storeid, $volname, $attribute, $value); } sub volume_size_info { diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm index 04b42f1..c8caa41 100644 --- a/PVE/Storage/BTRFSPlugin.pm +++ b/PVE/Storage/BTRFSPlugin.pm @@ -137,9 +137,9 @@ sub status { return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache); } -# TODO: sub get_volume_notes {} +# TODO: sub get_volume_attribute {} -# TODO: sub update_volume_notes {} +# TODO: sub update_volume_attribute {} # croak would not include the caller from within this module sub __error { diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm index 3a7e638..a3f9ebe 100644 --- a/PVE/Storage/CIFSPlugin.pm +++ b/PVE/Storage/CIFSPlugin.pm @@ -287,13 +287,26 @@ sub check_connection { return 1; } +# FIXME remove on the next APIAGE reset. +# Deprecated, use get_volume_attribute instead. sub get_volume_notes { my $class = shift; PVE::Storage::DirPlugin::get_volume_notes($class, @_); } + +# FIXME remove on the next APIAGE reset. +# Deprecated, use update_volume_attribute instead. sub update_volume_notes { my $class = shift; PVE::Storage::DirPlugin::update_volume_notes($class, @_); } +sub get_volume_attribute { + return PVE::Storage::DirPlugin::get_volume_attribute(@_); +} + +sub update_volume_attribute { + return PVE::Storage::DirPlugin::update_volume_attribute(@_); +} + 1; diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm index f587db7..f75c1b8 100644 --- a/PVE/Storage/CephFSPlugin.pm +++ b/PVE/Storage/CephFSPlugin.pm @@ -240,14 +240,26 @@ sub deactivate_storage { } } +# FIXME remove on the next APIAGE reset. +# Deprecated, use get_volume_attribute instead. sub get_volume_notes { my $class = shift; PVE::Storage::DirPlugin::get_volume_notes($class, @_); } +# FIXME remove on the next APIAGE reset. +# Deprecated, use update_volume_attribute instead. sub update_volume_notes { my $class = shift; PVE::Storage::DirPlugin::update_volume_notes($class, @_); } +sub get_volume_attribute { + return PVE::Storage::DirPlugin::get_volume_attribute(@_); +} + +sub update_volume_attribute { + return PVE::Storage::DirPlugin::update_volume_attribute(@_); +} + 1; diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm index 2aacb35..2cb55b8 100644 --- a/PVE/Storage/DirPlugin.pm +++ b/PVE/Storage/DirPlugin.pm @@ -91,6 +91,8 @@ sub parse_is_mountpoint { return $is_mp; # contains a path } +# FIXME remove on the next APIAGE reset. +# Deprecated, use get_volume_attribute instead. sub get_volume_notes { my ($class, $scfg, $storeid, $volname, $timeout) = @_; @@ -105,6 +107,8 @@ sub get_volume_notes { return ''; } +# FIXME remove on the next APIAGE reset. +# Deprecated, use update_volume_attribute instead. sub update_volume_notes { my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; @@ -122,6 +126,26 @@ sub update_volume_notes { return; } +sub get_volume_attribute { + my ($class, $scfg, $storeid, $volname, $attribute) = @_; + + if ($attribute eq 'notes') { + return $class->get_volume_notes($scfg, $storeid, $volname); + } + + return; +} + +sub update_volume_attribute { + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_; + + if ($attribute eq 'notes') { + return $class->update_volume_notes($scfg, $storeid, $volname, $value); + } + + die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n"; +} + sub status { my ($class, $storeid, $scfg, $cache) = @_; diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm index 21b288a..0400c93 100644 --- a/PVE/Storage/NFSPlugin.pm +++ b/PVE/Storage/NFSPlugin.pm @@ -189,13 +189,26 @@ sub check_connection { return 1; } +# FIXME remove on the next APIAGE reset. +# Deprecated, use get_volume_attribute instead. sub get_volume_notes { my $class = shift; PVE::Storage::DirPlugin::get_volume_notes($class, @_); } + +# FIXME remove on the next APIAGE reset. +# Deprecated, use update_volume_attribute instead. sub update_volume_notes { my $class = shift; PVE::Storage::DirPlugin::update_volume_notes($class, @_); } +sub get_volume_attribute { + return PVE::Storage::DirPlugin::get_volume_attribute(@_); +} + +sub update_volume_attribute { + return PVE::Storage::DirPlugin::update_volume_attribute(@_); +} + 1; diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm index bb1c382..76699e6 100644 --- a/PVE/Storage/PBSPlugin.pm +++ b/PVE/Storage/PBSPlugin.pm @@ -782,6 +782,8 @@ sub deactivate_volume { return 1; } +# FIXME remove on the next APIAGE reset. +# Deprecated, use get_volume_attribute instead. sub get_volume_notes { my ($class, $scfg, $storeid, $volname, $timeout) = @_; @@ -792,6 +794,8 @@ sub get_volume_notes { return $data->{notes}; } +# FIXME remove on the next APIAGE reset. +# Deprecated, use update_volume_attribute instead. sub update_volume_notes { my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; @@ -802,6 +806,26 @@ sub update_volume_notes { return undef; } +sub get_volume_attribute { + my ($class, $scfg, $storeid, $volname, $attribute) = @_; + + if ($attribute eq 'notes') { + return $class->get_volume_notes($scfg, $storeid, $volname); + } + + return; +} + +sub update_volume_attribute { + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_; + + if ($attribute eq 'notes') { + return $class->update_volume_notes($scfg, $storeid, $volname, $value); + } + + die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n"; +} + sub volume_size_info { my ($class, $scfg, $storeid, $volname, $timeout) = @_; diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm index 42eabdf..84de785 100644 --- a/PVE/Storage/Plugin.pm +++ b/PVE/Storage/Plugin.pm @@ -897,18 +897,52 @@ sub file_size_info { return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size; } +# FIXME remove on the next APIAGE reset. +# Deprecated, use get_volume_attribute instead. sub get_volume_notes { my ($class, $scfg, $storeid, $volname, $timeout) = @_; die "volume notes are not supported for $class"; } +# FIXME remove on the next APIAGE reset. +# Deprecated, use update_volume_attribute instead. sub update_volume_notes { my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; die "volume notes are not supported for $class"; } +# Returns undef if the attribute is not supported for the volume. +# Should die if there is an error fetching the attribute. +# Possible attributes: +# notes - user-provided comments/notes. +sub get_volume_attribute { + my ($class, $scfg, $storeid, $volname, $attribute) = @_; + + if ($attribute eq 'notes') { + my $notes = eval { $class->get_volume_notes($scfg, $storeid, $volname); }; + if (my $err = $@) { + return if $err =~ m/^volume notes are not supported/; + die $err; + } + return $notes; + } + + return; +} + +# Dies if the attribute is not supported for the volume. +sub update_volume_attribute { + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_; + + if ($attribute eq 'notes') { + $class->update_volume_notes($scfg, $storeid, $volname, $value); + } + + die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n"; +} + sub volume_size_info { my ($class, $scfg, $storeid, $volname, $timeout) = @_; my $path = $class->filesystem_path($scfg, $volname); @@ -1147,6 +1181,8 @@ my $get_subdir_files = sub { return $res; }; +# If attributes are set on a volume, they should be included in the result. +# See get_volume_attribute for a list of possible attributes. sub list_volumes { my ($class, $storeid, $scfg, $vmid, $content_types) = @_;