[pve-devel] [RFC pve-storage 00/36] Refactor / Cleanup of Storage Plugins

[pve-devel] [RFC pve-storage 00/36] Refactor / Cleanup of Storage Plugins

[Max Carrara]

RFC: Refactor / Cleanup of Storage Plugins
==========================================

I know this looks like a lot of patches and a lot to read up front, but
the changes I want to discuss here are not that intricate. I will
elaborate more in the paragraphs below, which should make it all much
more digestible.

Preface
-------

These patches represent the "current state of affairs" regarding the
refactor and are by no means complete, which is why this is obviously an
RFC instead of an actual patch series.

A lot of commits could probably be merged or split, or perhaps even be
split into multiple smaller series in the future.

The point of this RFC is to get some early feedback in before diving too
deep into the refactor itself or prematurely deciding which solution(s)
to use.

The overall long term goal is to refactor and document the entirety of
the storage API so that third parties can develop their own plugins much
more easily. Once the API has been polished and documented, a separate
plugin is set to be developed and put into its own Debian package that
ought to serve as an example (or perhaps starting point) for plugin
authors.

This RFC is basically the first step towards this goal.

Goals of This Refactor
----------------------

There are three main goals of this refactor / cleanup:

  1. Ensure storage plugins only do "plugin stuff" and are completely
     independent of one another - no plugin should use features,
     helpers, methods, etc. of another plugin. The only exempt of this
     is SectionConfig-related functionality, like using properties that
     another plugin provides.

The point of this is to ensure that our plugins themselves can be
changed or adapted at will without possibly affecting other plugins. In
other words, plugins should have *high cohesion and low coupling*.

This also means that plugins that inherit from another plugin should be
changed to inherit from the base plugin. The only such case currently is
the ZFSPlugin, which uses ZFSPoolPlugin as base.

  2. Expose and document existing helper subroutines and methods so that
     they may be reused where desired, so that no code has to depend on
     a specific plugin anymore. This applies to the more "useful" / less
     niche helpers.

There are a couple instances where other code relies directly on certain
plugins' helpers, thus making those helpers implicitly and
unintentionally part of a public interface.

Exposing and documenting those helpers has the benefit that our code
doesn't need to rely on the existence of a specific plugin anymore,
while at the same time making it easier for ourselves to develop further
plugins with similar functionalities. On top of that, these helpers can
then also be used by third party plugin authors for their own purposes.

  3. Make other "less important" helpers, helper methods, etc.
     private, so that they do not unintentionally become part of an
     implicit public interface.

This is mainly done to further prevent plugin-specific code from
accidentally being used in other places.

Goal 2 and 3 can be summed up as: "What's inside a plugin stays inside
the plugin, but if something is used elsewhere or potentially useful for
others, put it in a separate location and expose it explicitly."

Current Approaches & Reasonings
-------------------------------

The goals above are reflected rather obviously in the series, with a lot
of helpers either being made private or factored into a separate module.

One example of a plugin's functionalities being used by other plugins is
PVE::Storage::DirPlugin, whose methods are also refactored into helper
subroutines. The new helpers are given proper documentation as well as
subroutine prototypes and are substituted into places where the
DirPlugin's methods were being called.

The original helpers still remain in the plugin and instead only wrap
the new ones while also emitting a deprecation warning.

Similar refactorings are done for LVM- and ISCSI-related plugins, as
some of their code is used outside of the plugin it belongs to (or in
the case of LVM, some helpers are even used in `pve-manager`, for
example).

The ZFS-related plugins are only partially refactored at the moment,
mainly because refactoring them specifically is a lot more involved, and
as stated in the preface, I deemed it better to get some feedback in
first before "going all in" on a solution.

Nevertheless, the patches 34, 35, 36 demonstrate how some of the ZFS
helpers could be factored out into a separate module.

The only plugin currently not touched at all is the PBSPlugin - as
mentioned off list, some of the helpers there are also present in
`pve-common` and should go into a separate package in the future.

All helpers that are factored out are put into a new module / namespace:
PVE::Storage::Common. Helpers that can be grouped more specifically by
their functionalities are put into separate sub-packages. In total, the
following new modules are introduced so far:

  * PVE::Storage::Common
  * PVE::Storage::Common::ISCSI
  * PVE::Storage::Common::LVM
  * PVE::Storage::Common::Path
  * PVE::Storage::Common::ZFS

Each module comes with documentation written in Perl's POD format, which
can be rendered via the `perldoc` CLI or be viewed via an LSP.

Open Questions
--------------

This is a non-exhaustive list of questions that arose during
development:

  1. Instead of making some helpers private and some explicitly public
     in a new module, should all helpers be made public instead? There
     could be third-party plugins out there using some of the helpers
     that were made private that we do not know of.

  2. Because the helpers of the PBSPlugin (which are also in
     `pve-common`) are expected to be put into a separate package in the
     future, should the helpers that are factored out into
     PVE::Storage::Common and its sub-packages also be put into a
     separate package or moved to an existing one?

  3. What would be the best way to keep track of deprecated helpers so
     that we know when to remove them?

  4. Because the refactored helpers (those in PVE::Storage::Common etc.)
     are public, should they fall under the same API guarantees as
     PVE::Storage? Should there be a different mechanism instead?

  5. What would be the best way to split these changes up into multiple
     smaller series (so that e.g. reviewing becomes easier)?

     My current idea would be to put the following changes into separate
     series:
       * Making other plugins independent of the DirPlugin
       * Factoring out LVM-related helpers
       * Factoring out ISCSI-related helpers
       * Factoring out ZFS-related helpers

  6. Are there any alternatives to the current approach(es) that might
     be better than what's currently in this RFC?

Closing Thoughts
----------------

Please don't hesitate to let me know what you think - and thank you very
much for reading this far :) Even if it's just a minor thing, any
feedback is greatly appreciated.

Summary of Changes
------------------

Max Carrara (36):
  plugin: base: remove old fixme comments
  plugin: btrfs: make plugin-specific helpers private
  plugin: cephfs: make plugin-specific helpers private
  api: remove unused import of CIFS storage plugin
  plugin: cifs: make plugin-specific helpers private
  api: remove unused import of LVM storage plugin
  common: introduce common module
  plugin: dir: move helper subs of directory plugin to common modules
  plugin: lvm: move LVM helper subroutines into separate common module
  api: replace usages of deprecated LVM helper subs with new ones
  plugin: lvmthin: replace usages of deprecated LVM helpers with new
    ones
  plugin: lvmthin: move helper that lists thinpools to common LVM module
  common: lvm: update code style
  api: replace usages of deprecated LVM thin pool helper sub
  plugin: btrfs: replace deprecated helpers from directory plugin
  plugin: dir: factor storage methods into separate common subs
  plugin: dir: factor path validity check into helper methods
  plugin: btrfs: remove dependency on directory plugin
  plugin: cifs: remove dependency on directory plugin
  plugin: cephfs: remove dependency on directory plugin
  plugin: nfs: remove dependency on directory plugin
  plugin: btrfs: make helper methods private
  plugin: esxi: make helper subroutines private
  plugin: esxi: remove unused helper subroutine `query_vmdk_size`
  plugin: esxi: make helper methods private
  plugin: gluster: make helper subroutines private
  plugin: iscsi-direct: make helper subroutine `iscsi_ls` private
  plugin: iscsi: factor helper functions into common module
  plugin: iscsi: make helper subroutine `iscsi_session` private
  plugin: lvm: update definition of subroutine `check_tags`
  plugin: lvmthin: update definition of subroutine `activate_lv`
  plugin: nfs: make helper subroutines private
  plugin: rbd: update private sub signatures and make helpers private
  common: zfs: introduce module for common ZFS helpers
  plugin: zfspool: move helper `zfs_parse_zvol_list` to common module
  plugin: zfspool: refactor method `zfs_request` into helper subroutine

 src/PVE/API2/Disks/LVM.pm            |  12 +-
 src/PVE/API2/Disks/LVMThin.pm        |  14 +-
 src/PVE/API2/Storage/Config.pm       |   2 -
 src/PVE/API2/Storage/Scan.pm         |   6 +-
 src/PVE/Storage.pm                   |   4 +-
 src/PVE/Storage/BTRFSPlugin.pm       | 105 +++---
 src/PVE/Storage/CIFSPlugin.pm        |  53 ++-
 src/PVE/Storage/CephFSPlugin.pm      |  47 ++-
 src/PVE/Storage/Common.pm            | 303 ++++++++++++++++
 src/PVE/Storage/Common/ISCSI.pm      | 475 ++++++++++++++++++++++++
 src/PVE/Storage/Common/LVM.pm        | 515 +++++++++++++++++++++++++++
 src/PVE/Storage/Common/Makefile      |  10 +
 src/PVE/Storage/Common/Path.pm       | 124 +++++++
 src/PVE/Storage/Common/ZFS.pm        | 196 ++++++++++
 src/PVE/Storage/DirPlugin.pm         | 146 +++-----
 src/PVE/Storage/ESXiPlugin.pm        |  46 +--
 src/PVE/Storage/GlusterfsPlugin.pm   |  16 +-
 src/PVE/Storage/ISCSIDirectPlugin.pm |   2 +-
 src/PVE/Storage/ISCSIPlugin.pm       | 257 +------------
 src/PVE/Storage/LVMPlugin.pm         | 258 ++++----------
 src/PVE/Storage/LvmThinPlugin.pm     |  51 ++-
 src/PVE/Storage/Makefile             |   1 +
 src/PVE/Storage/NFSPlugin.pm         |  45 ++-
 src/PVE/Storage/Plugin.pm            |  19 -
 src/PVE/Storage/RBDPlugin.pm         |  84 ++---
 src/PVE/Storage/ZFSPoolPlugin.pm     | 157 ++++----
 26 files changed, 2107 insertions(+), 841 deletions(-)
 create mode 100644 src/PVE/Storage/Common.pm
 create mode 100644 src/PVE/Storage/Common/ISCSI.pm
 create mode 100644 src/PVE/Storage/Common/LVM.pm
 create mode 100644 src/PVE/Storage/Common/Makefile
 create mode 100644 src/PVE/Storage/Common/Path.pm
 create mode 100644 src/PVE/Storage/Common/ZFS.pm

-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments

[Max Carrara]

These have been around since 2012 - suffice to say they're not needed
anymore.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 6444390..3219f1f 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -299,19 +299,6 @@ sub parse_lvm_name {
     return $name;
 }
 
-# fixme: do we need this
-#PVE::JSONSchema::register_format('pve-storage-portal', \&verify_portal);
-#sub verify_portal {
-#    my ($portal, $noerr) = @_;
-#
-#    # IP with optional port
-#    if ($portal !~ m/^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}(:\d+)?$/) {
-#	return undef if $noerr;
-#	die "value does not look like a valid portal address\n";
-#    }
-#    return $portal;
-#}
-
 PVE::JSONSchema::register_format('pve-storage-portal-dns', \&verify_portal_dns);
 sub verify_portal_dns {
     my ($portal, $noerr) = @_;
@@ -457,12 +444,6 @@ sub decode_value {
 	    }
 	}
 
-	# fixme:
-	# no node restrictions for local storage
-	#if ($storeid && $storeid eq 'local' && scalar(keys(%$res))) {
-	#    die "storage '$storeid' does not allow node restrictions\n";
-	#}
-
 	return $res;
     } elsif ($key eq 'content-dirs') {
 	my $valid_content = $def->{content}->[0];
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 02/36] plugin: btrfs: make plugin-specific helpers private

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/BTRFSPlugin.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 42815cb..a503404 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -156,7 +156,7 @@ sub update_volume_attribute {
 }
 
 # croak would not include the caller from within this module
-sub __error {
+my sub __error {
     my ($msg) = @_;
     my (undef, $f, $n) = caller(1);
     die "$msg at $f: $n\n";
@@ -164,7 +164,7 @@ sub __error {
 
 # Given a name (eg. `vm-VMID-disk-ID.raw`), take the part up to the format suffix as the name of
 # the subdirectory (subvolume).
-sub raw_name_to_dir($) {
+my sub raw_name_to_dir($) {
     my ($raw) = @_;
 
     # For the subvolume directory Strip the `.<format>` suffix:
@@ -175,7 +175,7 @@ sub raw_name_to_dir($) {
     __error "internal error: bad disk name: $raw";
 }
 
-sub raw_file_to_subvol($) {
+my sub raw_file_to_subvol($) {
     my ($file) = @_;
 
     if ($file =~ m|^(.*)/disk\.raw$|) {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 03/36] plugin: cephfs: make plugin-specific helpers private

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/CephFSPlugin.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/CephFSPlugin.pm b/src/PVE/Storage/CephFSPlugin.pm
index 8aad518..98d1ba6 100644
--- a/src/PVE/Storage/CephFSPlugin.pm
+++ b/src/PVE/Storage/CephFSPlugin.pm
@@ -16,7 +16,7 @@ use PVE::Tools qw(run_command file_set_contents);
 
 use base qw(PVE::Storage::Plugin);
 
-sub cephfs_is_mounted {
+my sub cephfs_is_mounted {
     my ($scfg, $storeid, $mountdata) = @_;
 
     my $cmd_option = PVE::CephConfig::ceph_connect_option($scfg, $storeid);
@@ -39,7 +39,7 @@ sub cephfs_is_mounted {
 }
 
 # FIXME: remove once it's possible to specify _netdev for fuse.ceph mounts
-sub systemd_netmount {
+my sub systemd_netmount {
     my ($where, $type, $what, $opts) = @_;
 
 # don't do default deps, systemd v241 generator produces ordering deps on both
@@ -76,7 +76,7 @@ EOF
 
 }
 
-sub cephfs_mount {
+my sub cephfs_mount {
     my ($scfg, $storeid) = @_;
 
     my $mountpoint = $scfg->{path};
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 04/36] api: remove unused import of CIFS storage plugin

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/API2/Storage/Config.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/PVE/API2/Storage/Config.pm b/src/PVE/API2/Storage/Config.pm
index e04b6ab..56124fe 100755
--- a/src/PVE/API2/Storage/Config.pm
+++ b/src/PVE/API2/Storage/Config.pm
@@ -9,7 +9,6 @@ use PVE::Cluster qw(cfs_read_file cfs_write_file);
 use PVE::Storage;
 use PVE::Storage::Plugin;
 use PVE::Storage::LVMPlugin;
-use PVE::Storage::CIFSPlugin;
 use HTTP::Status qw(:constants);
 use Storable qw(dclone);
 use PVE::JSONSchema qw(get_standard_option);
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 05/36] plugin: cifs: make plugin-specific helpers private

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/CIFSPlugin.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/CIFSPlugin.pm b/src/PVE/Storage/CIFSPlugin.pm
index 2184471..18dc017 100644
--- a/src/PVE/Storage/CIFSPlugin.pm
+++ b/src/PVE/Storage/CIFSPlugin.pm
@@ -13,7 +13,7 @@ use base qw(PVE::Storage::Plugin);
 
 # CIFS helper functions
 
-sub cifs_is_mounted : prototype($$) {
+my sub cifs_is_mounted : prototype($$) {
     my ($scfg, $mountdata) = @_;
 
     my ($mountpoint, $server, $share) = $scfg->@{'path', 'server', 'share'};
@@ -31,12 +31,12 @@ sub cifs_is_mounted : prototype($$) {
     return undef;
 }
 
-sub cifs_cred_file_name {
+my sub cifs_cred_file_name {
     my ($storeid) = @_;
     return "/etc/pve/priv/storage/${storeid}.pw";
 }
 
-sub cifs_delete_credentials {
+my sub cifs_delete_credentials {
     my ($storeid) = @_;
 
     if (my $cred_file = get_cred_file($storeid)) {
@@ -44,7 +44,7 @@ sub cifs_delete_credentials {
     }
 }
 
-sub cifs_set_credentials {
+my sub cifs_set_credentials {
     my ($password, $storeid) = @_;
 
     my $cred_file = cifs_cred_file_name($storeid);
@@ -55,7 +55,7 @@ sub cifs_set_credentials {
     return $cred_file;
 }
 
-sub get_cred_file {
+my sub get_cred_file {
     my ($storeid) = @_;
 
     my $cred_file = cifs_cred_file_name($storeid);
@@ -66,7 +66,7 @@ sub get_cred_file {
     return undef;
 }
 
-sub cifs_mount : prototype($$$$$) {
+my sub cifs_mount : prototype($$$$$) {
     my ($scfg, $storeid, $smbver, $user, $domain) = @_;
 
     my ($mountpoint, $server, $share, $options) = $scfg->@{'path', 'server', 'share', 'options'};
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 06/36] api: remove unused import of LVM storage plugin

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/API2/Storage/Config.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/PVE/API2/Storage/Config.pm b/src/PVE/API2/Storage/Config.pm
index 56124fe..648b0f9 100755
--- a/src/PVE/API2/Storage/Config.pm
+++ b/src/PVE/API2/Storage/Config.pm
@@ -8,7 +8,6 @@ use PVE::Tools qw(extract_param extract_sensitive_params);
 use PVE::Cluster qw(cfs_read_file cfs_write_file);
 use PVE::Storage;
 use PVE::Storage::Plugin;
-use PVE::Storage::LVMPlugin;
 use HTTP::Status qw(:constants);
 use Storable qw(dclone);
 use PVE::JSONSchema qw(get_standard_option);
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 07/36] common: introduce common module

[Max Carrara]

This module's purpose is to provide shared functions, constants, etc.
for storage plugins and storage-related operations.

It also contains the `get_deprecation_warning` subroutine that makes
it easier to warn developers and/or plugin authors that a subroutine
will be removed in the future.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common.pm       | 59 +++++++++++++++++++++++++++++++++
 src/PVE/Storage/Common/Makefile |  6 ++++
 src/PVE/Storage/Makefile        |  1 +
 3 files changed, 66 insertions(+)
 create mode 100644 src/PVE/Storage/Common.pm
 create mode 100644 src/PVE/Storage/Common/Makefile

diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
new file mode 100644
index 0000000..f828c8c
--- /dev/null
+++ b/src/PVE/Storage/Common.pm
@@ -0,0 +1,59 @@
+package PVE::Storage::Common;
+
+use strict;
+use warnings;
+
+use parent qw(Exporter);
+
+our @EXPORT_OK = qw(
+    get_deprecation_warning
+);
+
+=pod
+
+=head1 NAME
+
+PVE::Storage::Common - Shared functions and utilities for storage plugins and storage operations
+
+=head1 DESCRIPTION
+
+This module contains common subroutines that are mainly to be used by storage
+plugins. This module's submodules contain subroutines that are tailored towards
+a more specific or related purpose.
+
+Functions concerned with storage-related C<PVE::SectionConfig> things, helpers
+for the C<PVE::Storage> API can be found in this module. Functions that can't
+be grouped in a submodule can also be found here.
+
+=head1 SUBMODULES
+
+=over
+
+=back
+
+=head1 FUNCTIONS
+
+=cut
+
+=pod
+
+=head3 get_deprecation_warning
+
+    $warning = get_deprecation_warning($new_sub_name)
+
+Returns a string that warns that the subroutine that called C<get_deprecation_warning>
+will be removed in the future and notes that C<$new_sub_name> should be used
+instead.
+
+=cut
+
+sub get_deprecation_warning : prototype($) {
+    my ($new_sub_name) = @_;
+
+    my $calling_sub = (caller(1))[3];
+
+    return "The subroutine '$calling_sub' is deprecated and will be removed in "
+	. "the future. Please use '$new_sub_name' instead.";
+}
+
+1;
diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
new file mode 100644
index 0000000..0c4bba5
--- /dev/null
+++ b/src/PVE/Storage/Common/Makefile
@@ -0,0 +1,6 @@
+SOURCES = \
+
+
+.PHONY: install
+install:
+	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/Common/$$i; done
diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
index d5cc942..2627062 100644
--- a/src/PVE/Storage/Makefile
+++ b/src/PVE/Storage/Makefile
@@ -18,5 +18,6 @@ SOURCES= \
 
 .PHONY: install
 install:
+	make -C Common install
 	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/$$i; done
 	make -C LunCmd install
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 08/36] plugin: dir: move helper subs of directory plugin to common modules

[Max Carrara]

Because the directory plugin's subroutines are frequently used by
other plugins, it's best to factor these helpers into the common
module. This is a first step in making other plugins not depend on the
directory plugin anymore.

Simultaneously this commit also introduces the `Common::Path` module,
which ought to provide shared subroutines for operations on paths.

The changes of this commit are backwards-compatible; the old subs act
as mere wrappers and will emit a warning when used.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common.pm       | 31 ++++++++++++++++++++
 src/PVE/Storage/Common/Makefile |  1 +
 src/PVE/Storage/Common/Path.pm  | 51 +++++++++++++++++++++++++++++++++
 src/PVE/Storage/DirPlugin.pm    | 38 ++++++++++++------------
 4 files changed, 101 insertions(+), 20 deletions(-)
 create mode 100644 src/PVE/Storage/Common/Path.pm

diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
index f828c8c..a2ae979 100644
--- a/src/PVE/Storage/Common.pm
+++ b/src/PVE/Storage/Common.pm
@@ -3,10 +3,13 @@ package PVE::Storage::Common;
 use strict;
 use warnings;
 
+use PVE::JSONSchema;
+
 use parent qw(Exporter);
 
 our @EXPORT_OK = qw(
     get_deprecation_warning
+    storage_parse_is_mountpoint
 );
 
 =pod
@@ -29,10 +32,19 @@ be grouped in a submodule can also be found here.
 
 =over
 
+=item C<PVE::Storage::Common::Path>
+
+Utilities concerned with working with paths.
+
 =back
 
 =head1 FUNCTIONS
 
+B<Note:> Functions prefixed with C<storage_> are related to the C<PVE::Storage>
+API and usually expect an C<$scfg> ("storage config") hash. This hash is
+expected to contain the configuration for I<one> storage, which is (usually)
+acquired via C<PVE::Storage::storage_config>.
+
 =cut
 
 =pod
@@ -56,4 +68,23 @@ sub get_deprecation_warning : prototype($) {
 	. "the future. Please use '$new_sub_name' instead.";
 }
 
+=head3 storage_parse_is_mountpoint
+
+    $path = storage_parse_is_mountpoint($scfg)
+
+Helper that tries to return a path if the given I<storage config> hash C<$scfg>
+contains an C<is_mountpoint> property. Returns C<undef> if it can't.
+
+=cut
+
+sub storage_parse_is_mountpoint : prototype($) {
+    my ($scfg) = @_;
+    my $is_mp = $scfg->{is_mountpoint};
+    return undef if !defined $is_mp;
+    if (defined(my $bool = PVE::JSONSchema::parse_boolean($is_mp))) {
+	return $bool ? $scfg->{path} : undef;
+    }
+    return $is_mp; # contains a path
+}
+
 1;
diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
index 0c4bba5..9455b81 100644
--- a/src/PVE/Storage/Common/Makefile
+++ b/src/PVE/Storage/Common/Makefile
@@ -1,4 +1,5 @@
 SOURCES = \
+	  Path.pm \
 
 
 .PHONY: install
diff --git a/src/PVE/Storage/Common/Path.pm b/src/PVE/Storage/Common/Path.pm
new file mode 100644
index 0000000..7535dda
--- /dev/null
+++ b/src/PVE/Storage/Common/Path.pm
@@ -0,0 +1,51 @@
+package PVE::Storage::Common::Path;
+
+use strict;
+use warnings;
+
+use Cwd;
+
+use PVE::ProcFSTools;
+
+use parent qw(Exporter);
+
+our @EXPORT_OK = qw(
+    path_is_mounted
+);
+
+=pod
+
+=head1 NAME
+
+PVE::Storage::Common::Path - Shared functions and utilities for manipulating paths
+
+=head1 FUNCTIONS
+
+=cut
+
+=pod
+
+=head3 path_is_mounted
+
+    $is_mounted = path_is_mounted($mountpoint)
+    $is_mounted = path_is_mounted($mountpoint, $mountdata)
+
+Checks if the given path in C<$mountpoint> is actually mounted. Optionally takes
+a C<$mountdata> hash returned by C<PVE::ProcFSTools::parse_proc_mounts> in
+order to avoid repeatedly reading and parsing C</proc/mounts>.
+
+=cut
+
+# NOTE: should ProcFSTools::is_mounted accept an optional cache like this?
+sub path_is_mounted {
+    my ($mountpoint, $mountdata) = @_;
+
+    $mountpoint = Cwd::realpath($mountpoint); # symlinks
+    return 0 if !defined($mountpoint); # path does not exist
+
+    $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata;
+    return 1 if grep { $_->[1] eq $mountpoint } @$mountdata;
+    return undef;
+}
+
+1;
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index 2efa8d5..ac0d365 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -3,12 +3,16 @@ package PVE::Storage::DirPlugin;
 use strict;
 use warnings;
 
-use Cwd;
 use Encode qw(decode encode);
 use File::Path;
 use IO::File;
 use POSIX;
 
+use PVE::Storage::Common qw(
+    get_deprecation_warning
+    storage_parse_is_mountpoint
+);
+use PVE::Storage::Common::Path;
 use PVE::Storage::Plugin;
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -86,26 +90,20 @@ sub options {
 # Storage implementation
 #
 
-# NOTE: should ProcFSTools::is_mounted accept an optional cache like this?
 sub path_is_mounted {
-    my ($mountpoint, $mountdata) = @_;
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::Path::path_is_mounted"
+    );
 
-    $mountpoint = Cwd::realpath($mountpoint); # symlinks
-    return 0 if !defined($mountpoint); # path does not exist
-
-    $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata;
-    return 1 if grep { $_->[1] eq $mountpoint } @$mountdata;
-    return undef;
+    return PVE::Storage::Common::Path::path_is_mounted(@_);
 }
 
 sub parse_is_mountpoint {
-    my ($scfg) = @_;
-    my $is_mp = $scfg->{is_mountpoint};
-    return undef if !defined $is_mp;
-    if (defined(my $bool = PVE::JSONSchema::parse_boolean($is_mp))) {
-	return $bool ? $scfg->{path} : undef;
-    }
-    return $is_mp; # contains a path
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::storage_parse_is_mountpoint"
+    );
+
+    return storage_parse_is_mountpoint(@_);
 }
 
 # FIXME move into 'get_volume_attribute' when removing 'get_volume_notes'
@@ -211,11 +209,11 @@ sub update_volume_attribute {
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    if (defined(my $mp = parse_is_mountpoint($scfg))) {
+    if (defined(my $mp = PVE::Storage::Common::storage_parse_is_mountpoint($scfg))) {
 	$cache->{mountdata} = PVE::ProcFSTools::parse_proc_mounts()
 	    if !$cache->{mountdata};
 
-	return undef if !path_is_mounted($mp, $cache->{mountdata});
+	return undef if !PVE::Storage::Common::Path::path_is_mounted($mp, $cache->{mountdata});
     }
 
     return $class->SUPER::status($storeid, $scfg, $cache);
@@ -227,8 +225,8 @@ sub activate_storage {
 
     my $path = $scfg->{path};
 
-    my $mp = parse_is_mountpoint($scfg);
-    if (defined($mp) && !path_is_mounted($mp, $cache->{mountdata})) {
+    my $mp = storage_parse_is_mountpoint($scfg);
+    if (defined($mp) && !PVE::Storage::Common::Path::path_is_mounted($mp, $cache->{mountdata})) {
 	die "unable to activate storage '$storeid' - " .
 	    "directory is expected to be a mount point but is not mounted: '$mp'\n";
     }
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 09/36] plugin: lvm: move LVM helper subroutines into separate common module

[Max Carrara]

A plugin should only do "plugin stuff" and not provide helper subs
that other modules also use, so move those helpers into a separate
module and document them.

This new `Common::LVM` module is a submodule of `Common` (as its name
implies) as that logically groups LVM-related subroutines together.

The changes of this commit are backwards-compatible; the old subs act
as mere wrappers and will emit a warning when used.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common.pm       |   4 +
 src/PVE/Storage/Common/LVM.pm   | 432 ++++++++++++++++++++++++++++++++
 src/PVE/Storage/Common/Makefile |   1 +
 src/PVE/Storage/LVMPlugin.pm    | 254 +++++--------------
 4 files changed, 497 insertions(+), 194 deletions(-)
 create mode 100644 src/PVE/Storage/Common/LVM.pm

diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
index a2ae979..0ca0db1 100644
--- a/src/PVE/Storage/Common.pm
+++ b/src/PVE/Storage/Common.pm
@@ -32,6 +32,10 @@ be grouped in a submodule can also be found here.
 
 =over
 
+=item C<PVE::Storage::Common::LVM>
+
+Utilities concerned with LVM, such as manipulating logical volumes.
+
 =item C<PVE::Storage::Common::Path>
 
 Utilities concerned with working with paths.
diff --git a/src/PVE/Storage/Common/LVM.pm b/src/PVE/Storage/Common/LVM.pm
new file mode 100644
index 0000000..e0e3263
--- /dev/null
+++ b/src/PVE/Storage/Common/LVM.pm
@@ -0,0 +1,432 @@
+package PVE::Storage::Common::LVM;
+
+use strict;
+use warnings;
+
+use IO::File;
+
+use PVE::Tools qw(run_command trim);
+
+use parent qw(Exporter);
+
+our @EXPORT_OK = qw(
+    lvm_pv_info
+    lvm_clear_first_sector
+    lvm_create_volume_group
+    lvm_destroy_volume_group
+    lvm_vgs
+    lvm_list_volumes
+    lvm_lvcreate
+    lvm_lvrename
+);
+
+=pod
+
+=head1 NAME
+
+Common::LVM - Provides helper subroutines that wrap commonly used LVM commands
+
+=head1 FUNCTIONS
+
+=cut
+
+my $ignore_no_medium_warnings = sub {
+    my $line = shift;
+    # ignore those, most of the time they're from (virtual) IPMI/iKVM devices
+    # and just spam the log..
+    if ($line !~ /open failed: No medium found/) {
+	print STDERR "$line\n";
+    }
+};
+
+=pod
+
+=head3 lvm_pv_info
+
+    $pvinfo = lvm_pv_info($device)
+
+Returns a hash containing information for a I<physical volume> specified
+by C<$device>, which must be a valid device path under C</dev>.
+
+The returned hash has the following structure:
+
+    {
+	pvname => "some-pv-name",
+	size => 15728640,  # size in kibibytes!
+	vgname => "some-vg-name",
+	uuid => "1ba3cb42-5407-4cd5-9754-7060dc36ce6d",
+    }
+
+This function will die if no C<$device> is specified of if multiple PV entries
+exist for C<$device>.
+
+Should the C<$device> not have an C<LVM2> label, this function will return
+C<undef> instead.
+
+=cut
+
+sub lvm_pv_info : prototype($) {
+    my ($device) = @_;
+
+    die "no device specified" if !$device;
+
+    my $has_label = 0;
+
+    my $cmd = ['/usr/bin/file', '-L', '-s', $device];
+    run_command($cmd, outfunc => sub {
+	my $line = shift;
+	$has_label = 1 if $line =~ m/LVM2/;
+    });
+
+    return undef if !$has_label;
+
+    $cmd = ['/sbin/pvs', '--separator', ':', '--noheadings', '--units', 'k',
+	    '--unbuffered', '--nosuffix', '--options',
+	    'pv_name,pv_size,vg_name,pv_uuid', $device];
+
+    my $pvinfo;
+    run_command($cmd, outfunc => sub {
+	my $line = shift;
+
+	$line = trim($line);
+
+	my ($pvname, $size, $vgname, $uuid) = split(':', $line);
+
+	die "found multiple pvs entries for device '$device'\n"
+	    if $pvinfo;
+
+	$pvinfo = {
+	    pvname => $pvname,
+	    size => int($size),
+	    vgname => $vgname,
+	    uuid => $uuid,
+	};
+    });
+
+    return $pvinfo;
+}
+
+=pod
+
+=head3 lvm_clear_first_sector
+
+    lvm_clear_first_sector($device)
+
+Clears the first sector (first 512 bits) of the given block device C<$device>.
+
+B<WARNING:> Use with caution. This function does not actually check whether
+a valid device is passed or not.
+
+=cut
+
+sub lvm_clear_first_sector : prototype($) {
+    my ($dev) = shift;
+
+    if (my $fh = IO::File->new($dev, "w")) {
+	my $buf = 0 x 512;
+	syswrite $fh, $buf;
+	$fh->close();
+    }
+}
+
+=pod
+
+=head3 lvm_create_volume_group
+
+    lvm_create_volume_group($device, $vgname, $shared)
+
+Creates a I<volume group> for the block device C<$device> with the name
+C<$vgname>. The C<$shared> parameter is currently unused.
+
+Dies if C<$device> is already part of a volume group.
+
+If C<$device> is already part of a volume group with the exact same name as in
+C<$vgname>, nothing will be done and the function returns early.
+
+=cut
+
+sub lvm_create_volume_group : prototype($$$) {
+    my ($device, $vgname, $shared) = @_;
+
+    my $res = lvm_pv_info($device);
+
+    if ($res->{vgname}) {
+	return if $res->{vgname} eq $vgname; # already created
+	die "device '$device' is already used by volume group '$res->{vgname}'\n";
+    }
+
+    lvm_clear_first_sector($device); # else pvcreate fails
+
+    # we use --metadatasize 250k, which reseults in "pe_start = 512"
+    # so pe_start is aligned on a 128k boundary (advantage for SSDs)
+    my $cmd = ['/sbin/pvcreate', '--metadatasize', '250k', $device];
+
+    run_command($cmd, errmsg => "pvcreate '$device' error");
+
+    $cmd = ['/sbin/vgcreate', $vgname, $device];
+    # push @$cmd, '-c', 'y' if $shared; # we do not use this yet
+
+    run_command($cmd, errmsg => "vgcreate $vgname $device error", errfunc => $ignore_no_medium_warnings, outfunc => $ignore_no_medium_warnings);
+}
+
+=pod
+
+=head3 lvm_destroy_volume_group
+
+    lvm_destroy_volume_group($vgname)
+
+Destroys the I<volume group> with the name C<$vgname>.
+
+=cut
+
+sub lvm_destroy_volume_group : prototype($) {
+    my ($vgname) = @_;
+
+    run_command(
+	['vgremove', '-y', $vgname],
+	errmsg => "unable to remove volume group $vgname",
+	errfunc => $ignore_no_medium_warnings,
+	outfunc => $ignore_no_medium_warnings,
+    );
+}
+
+=pod
+
+=head3 lvm_vgs
+
+    $vgs = lvm_vgs()
+    $vgs = lvm_vgs($includepvs)
+    $vgs = lvm_vgs(1)
+
+Returns a hash containing information of the host's I<volume groups>. If the
+optional C<$includepvs> parameter is truthy or C<1>, the hash will also contain
+extra data for the I<physical volumes> in each volume group.
+
+The returned hash has the following structure:
+
+    {
+	'vg-name-00' => {
+	    size => 16106127360,  # sizes in bytes!
+	    free => 10737418240,
+	    lvcount => 2,
+	    pvs => [
+		{
+		    name => 'vg-name-00--pv-00',
+		    size => ...,
+		    free => ...,
+		},
+		{
+		    name => 'vg-name-00--pv-01',
+		    size => ...,
+		    free => ...,
+		},
+	    ]
+	},
+	'vg-name-01' => {
+	    size => 16106127360,
+	    free => 10737418240,
+	    lvcount => 1,
+	    pvs => [
+		{
+		    name => 'vg-name-01--pv-00',
+		    size => ...,
+		    free => ...,
+		},
+	    ]
+	},
+    }
+
+=cut
+
+sub lvm_vgs : prototype(;$) {
+    my ($includepvs) = @_;
+
+    my $cmd = ['/sbin/vgs', '--separator', ':', '--noheadings', '--units', 'b',
+	       '--unbuffered', '--nosuffix', '--options'];
+
+    my $cols = [qw(vg_name vg_size vg_free lv_count)];
+
+    if ($includepvs) {
+	push @$cols, qw(pv_name pv_size pv_free);
+    }
+
+    push @$cmd, join(',', @$cols);
+
+    my $vgs = {};
+    eval {
+	run_command($cmd, outfunc => sub {
+	    my $line = shift;
+	    $line = trim($line);
+
+	    my ($name, $size, $free, $lvcount, $pvname, $pvsize, $pvfree) = split (':', $line);
+
+	    $vgs->{$name} //= {
+		size => int ($size),
+		free => int ($free),
+		lvcount => int($lvcount)
+	    };
+
+	    if (defined($pvname) && defined($pvsize) && defined($pvfree)) {
+		push @{$vgs->{$name}->{pvs}}, {
+		    name => $pvname,
+		    size => int($pvsize),
+		    free => int($pvfree),
+		};
+	    }
+	},
+	errfunc => $ignore_no_medium_warnings,
+	);
+    };
+    my $err = $@;
+
+    # just warn (vgs return error code 5 if clvmd does not run)
+    # but output is still OK (list without clustered VGs)
+    warn $err if $err;
+
+    return $vgs;
+}
+
+=pod
+
+=head3 lvm_list_volumes
+
+    $lvs = lvm_list_volumes()
+    $lvs = lvm_list_volumes($vgname)
+
+Returns a hash with information of all I<logical volumes> on the host. May
+optionally be limited to a single I<volume group> by providing its name
+C<$vgname>.
+
+The returned hash has the following structure:
+
+    {
+	'vg-name-00' => {
+	    'lv-name-00' => {
+		lv_size => 10737418240,  # sizes in bytes!
+		lv_state => '...',
+		lv_type => '...',
+		pool_lv => '...',   # optional
+		tags => '...',	    # optional
+		ctime => '...',
+	    },
+	    'lv-name-01' => {
+		lv_size => 10737418240,
+		lv_state => '...',
+		lv_type => 't',
+		pool_lv => '...',   # optional
+		tags => '...',	    # optional
+		ctime => '...',
+		# thinpool specific data
+		metadata_size => 29381237,  # sizes in bytes!
+		metadata_used => 12917,
+		used => 3543348019,
+	    },
+	},
+	'vg-name-00' => {
+	    ...
+	}
+    }
+
+=cut
+
+sub lvm_list_volumes : prototype(;$) {
+    my ($vgname) = @_;
+
+    my $option_list = 'vg_name,lv_name,lv_size,lv_attr,pool_lv,data_percent,metadata_percent,snap_percent,uuid,tags,metadata_size,time';
+
+    my $cmd = [
+	'/sbin/lvs', '--separator', ':', '--noheadings', '--units', 'b',
+	'--unbuffered', '--nosuffix',
+	'--config', 'report/time_format="%s"',
+	'--options', $option_list,
+    ];
+
+    push @$cmd, $vgname if $vgname;
+
+    my $lvs = {};
+    run_command($cmd, outfunc => sub {
+	my $line = shift;
+
+	$line = trim($line);
+
+	my ($vg_name, $lv_name, $lv_size, $lv_attr, $pool_lv, $data_percent, $meta_percent, $snap_percent, $uuid, $tags, $meta_size, $ctime) = split(':', $line);
+	return if !$vg_name;
+	return if !$lv_name;
+
+	my $lv_type = substr($lv_attr, 0, 1);
+
+	my $d = {
+	    lv_size => int($lv_size),
+	    lv_state => substr($lv_attr, 4, 1),
+	    lv_type => $lv_type,
+	};
+	$d->{pool_lv} = $pool_lv if $pool_lv;
+	$d->{tags} = $tags if $tags;
+	$d->{ctime} = $ctime;
+
+	if ($lv_type eq 't') {
+	    $data_percent ||= 0;
+	    $meta_percent ||= 0;
+	    $snap_percent ||= 0;
+	    $d->{metadata_size} = int($meta_size);
+	    $d->{metadata_used} = int(($meta_percent * $meta_size)/100);
+	    $d->{used} = int(($data_percent * $lv_size)/100);
+	}
+	$lvs->{$vg_name}->{$lv_name} = $d;
+    },
+    errfunc => $ignore_no_medium_warnings,
+    );
+
+    return $lvs;
+}
+
+=head3 lvm_lvcreate
+
+    lvm_lvcreate($vgname, $name, $size, $tags)
+
+Creates a new I<logical volume> named C<$name> for the I<volume group>
+C<$vgname>, with a size of C<$size> B<kibibytes> per default. Optionally,
+a list of tags for the new LV may be provided via C<$tags>.
+
+Alternatively, C<$size> may optionally also be expressed with a unit, e.g.
+C<"50g"> (50 gibibytes), C<"1T"> (1 terabyte), etc.
+
+=cut
+
+sub lvm_lvcreate : prototype($$$;$) {
+    my ($vg, $name, $size, $tags) = @_;
+
+    if ($size =~ m/\d$/) { # no unit is given
+	$size .= "k"; # default to kilobytes
+    }
+
+    my $cmd = ['/sbin/lvcreate', '-aly', '-Wy', '--yes', '--size', $size, '--name', $name];
+    for my $tag (@$tags) {
+	push @$cmd, '--addtag', $tag;
+    }
+    push @$cmd, $vg;
+
+    run_command($cmd, errmsg => "lvcreate '$vg/$name' error");
+}
+
+=pod
+
+=head3 lvm_lvrename
+
+    lvm_lvrename($vgname, $oldname, $newname)
+
+Renames a I<logical volume> of a I<volume group> C<$vgname> from C<$oldname>
+to C<$newname>.
+
+=cut
+
+sub lvm_lvrename : prototype($$$) {
+    my ($vg, $oldname, $newname) = @_;
+
+    run_command(
+	['/sbin/lvrename', $vg, $oldname, $newname],
+	errmsg => "lvrename '${vg}/${oldname}' to '${newname}' error",
+    );
+}
+
+1;
diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
index 9455b81..863f7c7 100644
--- a/src/PVE/Storage/Common/Makefile
+++ b/src/PVE/Storage/Common/Makefile
@@ -1,4 +1,5 @@
 SOURCES = \
+	  LVM.pm \
 	  Path.pm \
 
 
diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 4b951e7..a9bc178 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -6,207 +6,98 @@ use warnings;
 use IO::File;
 
 use PVE::Tools qw(run_command trim);
+use PVE::Storage::Common qw(get_deprecation_warning);
 use PVE::Storage::Plugin;
+use PVE::Storage::Common::LVM;
 use PVE::JSONSchema qw(get_standard_option);
 
 use base qw(PVE::Storage::Plugin);
 
 # lvm helper functions
 
-my $ignore_no_medium_warnings = sub {
-    my $line = shift;
-    # ignore those, most of the time they're from (virtual) IPMI/iKVM devices
-    # and just spam the log..
-    if ($line !~ /open failed: No medium found/) {
-	print STDERR "$line\n";
-    }
-};
-
 sub lvm_pv_info {
-    my ($device) = @_;
-
-    die "no device specified" if !$device;
-
-    my $has_label = 0;
-
-    my $cmd = ['/usr/bin/file', '-L', '-s', $device];
-    run_command($cmd, outfunc => sub {
-	my $line = shift;
-	$has_label = 1 if $line =~ m/LVM2/;
-    });
-
-    return undef if !$has_label;
-
-    $cmd = ['/sbin/pvs', '--separator', ':', '--noheadings', '--units', 'k',
-	    '--unbuffered', '--nosuffix', '--options',
-	    'pv_name,pv_size,vg_name,pv_uuid', $device];
-
-    my $pvinfo;
-    run_command($cmd, outfunc => sub {
-	my $line = shift;
-
-	$line = trim($line);
-
-	my ($pvname, $size, $vgname, $uuid) = split(':', $line);
-
-	die "found multiple pvs entries for device '$device'\n"
-	    if $pvinfo;
+    warn get_deprecation_warning(
+	'PVE::Storage::Common::LVM::lvm_pv_info'
+    );
 
-	$pvinfo = {
-	    pvname => $pvname,
-	    size => int($size),
-	    vgname => $vgname,
-	    uuid => $uuid,
-	};
-    });
+    my ($device) = @_;
 
-    return $pvinfo;
+    return PVE::Storage::Common::LVM::lvm_pv_info($device);
 }
 
 sub clear_first_sector {
-    my ($dev) = shift;
+    warn get_depreciation_warning(
+	'PVE::Storage::Common::LVM::lvm_clear_first_sector'
+    );
 
-    if (my $fh = IO::File->new($dev, "w")) {
-	my $buf = 0 x 512;
-	syswrite $fh, $buf;
-	$fh->close();
-    }
+    my ($dev) = @_;
+
+    PVE::Storage::Common::LVM::lvm_clear_first_sector($dev);
 }
 
 sub lvm_create_volume_group {
-    my ($device, $vgname, $shared) = @_;
-
-    my $res = lvm_pv_info($device);
-
-    if ($res->{vgname}) {
-	return if $res->{vgname} eq $vgname; # already created
-	die "device '$device' is already used by volume group '$res->{vgname}'\n";
-    }
-
-    clear_first_sector($device); # else pvcreate fails
-
-    # we use --metadatasize 250k, which reseults in "pe_start = 512"
-    # so pe_start is aligned on a 128k boundary (advantage for SSDs)
-    my $cmd = ['/sbin/pvcreate', '--metadatasize', '250k', $device];
-
-    run_command($cmd, errmsg => "pvcreate '$device' error");
+    warn get_depreciation_warning(
+	'PVE::Storage::Common::LVM::lvm_create_volume_group'
+    );
 
-    $cmd = ['/sbin/vgcreate', $vgname, $device];
-    # push @$cmd, '-c', 'y' if $shared; # we do not use this yet
+    my ($device, $vgname, $shared) = @_;
 
-    run_command($cmd, errmsg => "vgcreate $vgname $device error", errfunc => $ignore_no_medium_warnings, outfunc => $ignore_no_medium_warnings);
+    PVE::Storage::Common::LVM::lvm_create_volume_group(
+	$device, $vgname, $shared
+    );
 }
 
 sub lvm_destroy_volume_group {
+    warn get_depreciation_warning(
+	'PVE::Storage::Common::LVM::lvm_destroy_volume_group'
+    );
+
     my ($vgname) = @_;
 
-    run_command(
-	['vgremove', '-y', $vgname],
-	errmsg => "unable to remove volume group $vgname",
-	errfunc => $ignore_no_medium_warnings,
-	outfunc => $ignore_no_medium_warnings,
-    );
+    PVE::Storage::Common::LVM::lvm_destroy_volume_group($vgname);
 }
 
 sub lvm_vgs {
-    my ($includepvs) = @_;
-
-    my $cmd = ['/sbin/vgs', '--separator', ':', '--noheadings', '--units', 'b',
-	       '--unbuffered', '--nosuffix', '--options'];
-
-    my $cols = [qw(vg_name vg_size vg_free lv_count)];
+    warn get_depreciation_warning(
+	'PVE::Storage::Common::LVM::lvm_vgs'
+    );
 
-    if ($includepvs) {
-	push @$cols, qw(pv_name pv_size pv_free);
-    }
+    my ($includepvs) = @_;
 
-    push @$cmd, join(',', @$cols);
+    return PVE::Storage::Common::LVM::lvm_vgs($includepvs);
+}
 
-    my $vgs = {};
-    eval {
-	run_command($cmd, outfunc => sub {
-	    my $line = shift;
-	    $line = trim($line);
+sub lvm_list_volumes {
+    warn get_depreciation_warning(
+	'PVE::Storage::Common::LVM::lvm_list_volumes'
+    );
 
-	    my ($name, $size, $free, $lvcount, $pvname, $pvsize, $pvfree) = split (':', $line);
+    my ($vgname) = @_;
 
-	    $vgs->{$name} //= {
-		size => int ($size),
-		free => int ($free),
-		lvcount => int($lvcount)
-	    };
+    return PVE::Storage::Common::LVM::lvm_list_volumes($vgname);
+}
 
-	    if (defined($pvname) && defined($pvsize) && defined($pvfree)) {
-		push @{$vgs->{$name}->{pvs}}, {
-		    name => $pvname,
-		    size => int($pvsize),
-		    free => int($pvfree),
-		};
-	    }
-	},
-	errfunc => $ignore_no_medium_warnings,
-	);
-    };
-    my $err = $@;
+sub lvcreate {
+    warn get_deprecation_warning(
+	'PVE::Storage::Common::LVM::lvm_lvcreate'
+    );
 
-    # just warn (vgs return error code 5 if clvmd does not run)
-    # but output is still OK (list without clustered VGs)
-    warn $err if $err;
+    my ($vgname, $name, $size, $tags) = @_;
 
-    return $vgs;
+    PVE::Storage::Common::LVM::lvm_lvcreate($vgname, $name, $size, $tags);
 }
 
-sub lvm_list_volumes {
-    my ($vgname) = @_;
-
-    my $option_list = 'vg_name,lv_name,lv_size,lv_attr,pool_lv,data_percent,metadata_percent,snap_percent,uuid,tags,metadata_size,time';
-
-    my $cmd = [
-	'/sbin/lvs', '--separator', ':', '--noheadings', '--units', 'b',
-	'--unbuffered', '--nosuffix',
-	'--config', 'report/time_format="%s"',
-	'--options', $option_list,
-    ];
-
-    push @$cmd, $vgname if $vgname;
-
-    my $lvs = {};
-    run_command($cmd, outfunc => sub {
-	my $line = shift;
-
-	$line = trim($line);
-
-	my ($vg_name, $lv_name, $lv_size, $lv_attr, $pool_lv, $data_percent, $meta_percent, $snap_percent, $uuid, $tags, $meta_size, $ctime) = split(':', $line);
-	return if !$vg_name;
-	return if !$lv_name;
-
-	my $lv_type = substr($lv_attr, 0, 1);
-
-	my $d = {
-	    lv_size => int($lv_size),
-	    lv_state => substr($lv_attr, 4, 1),
-	    lv_type => $lv_type,
-	};
-	$d->{pool_lv} = $pool_lv if $pool_lv;
-	$d->{tags} = $tags if $tags;
-	$d->{ctime} = $ctime;
-
-	if ($lv_type eq 't') {
-	    $data_percent ||= 0;
-	    $meta_percent ||= 0;
-	    $snap_percent ||= 0;
-	    $d->{metadata_size} = int($meta_size);
-	    $d->{metadata_used} = int(($meta_percent * $meta_size)/100);
-	    $d->{used} = int(($data_percent * $lv_size)/100);
-	}
-	$lvs->{$vg_name}->{$lv_name} = $d;
-    },
-    errfunc => $ignore_no_medium_warnings,
+sub lvrename {
+    warn get_deprecation_warning(
+	'PVE::Storage::Common::LVM::lvm_lvrename'
     );
 
-    return $lvs;
+    my ($vgname, $oldname, $newname) = @_;
+
+    PVE::Storage::Common::LVM::lvm_lvrename($vgname, $oldname, $newname);
 }
 
+
 # Configuration
 
 sub type {
@@ -279,7 +170,7 @@ sub on_add_hook {
 
 	PVE::Storage::activate_storage($cfg, $baseid);
 
-	lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared});
+	PVE::Storage::Common::LVM::lvm_create_volume_group($path, $scfg->{vgname}, $scfg->{shared});
     }
 
     return;
@@ -328,38 +219,13 @@ sub find_free_diskname {
 
     my $vg = $scfg->{vgname};
 
-    my $lvs = lvm_list_volumes($vg);
+    my $lvs = PVE::Storage::Common::LVM::lvm_list_volumes($vg);
 
     my $disk_list = [ keys %{$lvs->{$vg}} ];
 
     return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
 }
 
-sub lvcreate {
-    my ($vg, $name, $size, $tags) = @_;
-
-    if ($size =~ m/\d$/) { # no unit is given
-	$size .= "k"; # default to kilobytes
-    }
-
-    my $cmd = ['/sbin/lvcreate', '-aly', '-Wy', '--yes', '--size', $size, '--name', $name];
-    for my $tag (@$tags) {
-	push @$cmd, '--addtag', $tag;
-    }
-    push @$cmd, $vg;
-
-    run_command($cmd, errmsg => "lvcreate '$vg/$name' error");
-}
-
-sub lvrename {
-    my ($vg, $oldname, $newname) = @_;
-
-    run_command(
-	['/sbin/lvrename', $vg, $oldname, $newname],
-	errmsg => "lvrename '${vg}/${oldname}' to '${newname}' error",
-    );
-}
-
 sub alloc_image {
     my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
 
@@ -368,7 +234,7 @@ sub alloc_image {
     die "illegal name '$name' - should be 'vm-$vmid-*'\n"
 	if  $name && $name !~ m/^vm-$vmid-/;
 
-    my $vgs = lvm_vgs();
+    my $vgs = PVE::Storage::Common::LVM::lvm_vgs();
 
     my $vg = $scfg->{vgname};
 
@@ -381,7 +247,7 @@ sub alloc_image {
     $name = $class->find_free_diskname($storeid, $scfg, $vmid)
 	if !$name;
 
-    lvcreate($vg, $name, $size, ["pve-vm-$vmid"]);
+    PVE::Storage::Common::LVM::lvcreate($vg, $name, $size, ["pve-vm-$vmid"]);
 
     return $name;
 }
@@ -452,7 +318,7 @@ sub list_images {
 
     my $vgname = $scfg->{vgname};
 
-    $cache->{lvs} = lvm_list_volumes() if !$cache->{lvs};
+    $cache->{lvs} = PVE::Storage::Common::LVM::lvm_list_volumes() if !$cache->{lvs};
 
     my $res = [];
 
@@ -492,7 +358,7 @@ sub list_images {
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    $cache->{vgs} = lvm_vgs() if !$cache->{vgs};
+    $cache->{vgs} = PVE::Storage::Common::LVM::lvm_vgs() if !$cache->{vgs};
 
     my $vgname = $scfg->{vgname};
 
@@ -506,7 +372,7 @@ sub status {
 sub activate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    $cache->{vgs} = lvm_vgs() if !$cache->{vgs};
+    $cache->{vgs} = PVE::Storage::Common::LVM::lvm_vgs() if !$cache->{vgs};
 
     # In LVM2, vgscans take place automatically;
     # this is just to be sure
@@ -669,7 +535,7 @@ sub volume_import {
 	if $file_format ne 'raw';
 
     my $vg = $scfg->{vgname};
-    my $lvs = lvm_list_volumes($vg);
+    my $lvs = PVE::Storage::Common::LVM::lvm_list_volumes($vg);
     if ($lvs->{$vg}->{$volname}) {
 	die "volume $vg/$volname already exists\n" if !$allow_rename;
 	warn "volume $vg/$volname already exists - importing with a different name\n";
@@ -730,7 +596,7 @@ sub rename_volume {
 	if !$target_volname;
 
     my $vg = $scfg->{vgname};
-    my $lvs = lvm_list_volumes($vg);
+    my $lvs = PVE::Storage::Common::LVM::lvm_list_volumes($vg);
     die "target volume '${target_volname}' already exists\n"
 	if ($lvs->{$vg}->{$target_volname});
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 10/36] api: replace usages of deprecated LVM helper subs with new ones

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/API2/Disks/LVM.pm     | 12 ++++++------
 src/PVE/API2/Disks/LVMThin.pm | 10 +++++-----
 src/PVE/API2/Storage/Scan.pm  |  4 ++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/PVE/API2/Disks/LVM.pm b/src/PVE/API2/Disks/LVM.pm
index 3c5bdfa..d062758 100644
--- a/src/PVE/API2/Disks/LVM.pm
+++ b/src/PVE/API2/Disks/LVM.pm
@@ -3,7 +3,7 @@ package PVE::API2::Disks::LVM;
 use strict;
 use warnings;
 
-use PVE::Storage::LVMPlugin;
+use PVE::Storage::Common::LVM qw(lvm_vgs lvm_create_volume_group lvm_destroy_volume_group);
 use PVE::Diskmanage;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::API2::Storage::Config;
@@ -91,7 +91,7 @@ __PACKAGE__->register_method ({
 
 	my $result = [];
 
-	my $vgs = PVE::Storage::LVMPlugin::lvm_vgs(1);
+	my $vgs = lvm_vgs(1);
 
 	foreach my $vg_name (sort keys %$vgs) {
 	    my $vg = $vgs->{$vg_name};
@@ -174,14 +174,14 @@ __PACKAGE__->register_method ({
 	    PVE::Diskmanage::locked_disk_action(sub {
 		PVE::Diskmanage::assert_disk_unused($dev);
 		die "volume group with name '${name}' already exists on node '${node}'\n"
-		    if PVE::Storage::LVMPlugin::lvm_vgs()->{$name};
+		    if lvm_vgs()->{$name};
 
 		if (PVE::Diskmanage::is_partition($dev)) {
 		    eval { PVE::Diskmanage::change_parttype($dev, '8E00'); };
 		    warn $@ if $@;
 		}
 
-		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
+		lvm_create_volume_group($dev, $name);
 
 		PVE::Diskmanage::udevadm_trigger($dev);
 
@@ -240,10 +240,10 @@ __PACKAGE__->register_method ({
 
 	my $worker = sub {
 	    PVE::Diskmanage::locked_disk_action(sub {
-		my $vgs = PVE::Storage::LVMPlugin::lvm_vgs(1);
+		my $vgs = lvm_vgs(1);
 		die "no such volume group '$name'\n" if !$vgs->{$name};
 
-		PVE::Storage::LVMPlugin::lvm_destroy_volume_group($name);
+		lvm_destroy_volume_group($name);
 
 		my $config_err;
 		if ($param->{'cleanup-config'}) {
diff --git a/src/PVE/API2/Disks/LVMThin.pm b/src/PVE/API2/Disks/LVMThin.pm
index f1c3957..bd46478 100644
--- a/src/PVE/API2/Disks/LVMThin.pm
+++ b/src/PVE/API2/Disks/LVMThin.pm
@@ -3,7 +3,7 @@ package PVE::API2::Disks::LVMThin;
 use strict;
 use warnings;
 
-use PVE::Storage::LvmThinPlugin;
+use PVE::Storage::Common::LVM qw(lvm_vgs lvm_create_volume_group lvm_pv_info);
 use PVE::Diskmanage;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::API2::Storage::Config;
@@ -133,15 +133,15 @@ __PACKAGE__->register_method ({
 		PVE::Diskmanage::assert_disk_unused($dev);
 
 		die "volume group with name '${name}' already exists on node '${node}'\n"
-		    if PVE::Storage::LVMPlugin::lvm_vgs()->{$name};
+		    if lvm_vgs()->{$name};
 
 		if (PVE::Diskmanage::is_partition($dev)) {
 		    eval { PVE::Diskmanage::change_parttype($dev, '8E00'); };
 		    warn $@ if $@;
 		}
 
-		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
-		my $pv = PVE::Storage::LVMPlugin::lvm_pv_info($dev);
+		lvm_create_volume_group($dev, $name);
+		my $pv = lvm_pv_info($dev);
 		# keep some free space just in case
 		my $datasize = $pv->{size} - 128*1024;
 		# default to 1% for metadata
@@ -241,7 +241,7 @@ __PACKAGE__->register_method ({
 		}
 
 		if ($param->{'cleanup-disks'}) {
-		    my $vgs = PVE::Storage::LVMPlugin::lvm_vgs(1);
+		    my $vgs = lvm_vgs(1);
 
 		    die "no such volume group '$vg'\n" if !$vgs->{$vg};
 		    die "volume group '$vg' still in use\n" if $vgs->{$vg}->{lvcount} > 0;
diff --git a/src/PVE/API2/Storage/Scan.pm b/src/PVE/API2/Storage/Scan.pm
index d7a8743..bad280d 100644
--- a/src/PVE/API2/Storage/Scan.pm
+++ b/src/PVE/API2/Storage/Scan.pm
@@ -8,7 +8,7 @@ use warnings;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTHandler;
 use PVE::SafeSyslog;
-use PVE::Storage::LVMPlugin;
+use PVE::Storage::Common::LVM qw(lvm_vgs);
 use PVE::Storage;
 use PVE::SysFSTools;
 
@@ -369,7 +369,7 @@ __PACKAGE__->register_method({
     code => sub {
 	my ($param) = @_;
 
-	my $res = PVE::Storage::LVMPlugin::lvm_vgs();
+	my $res = lvm_vgs();
 	return PVE::RESTHandler::hash_to_array($res, 'vg');
     }});
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 11/36] plugin: lvmthin: replace usages of deprecated LVM helpers with new ones

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LvmThinPlugin.pm | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 4b23623..480cc78 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -6,6 +6,7 @@ use warnings;
 use IO::File;
 
 use PVE::Tools qw(run_command trim);
+use PVE::Storage::Common::LVM qw(lvm_vgs lvm_list_volumes);
 use PVE::Storage::Plugin;
 use PVE::Storage::LVMPlugin;
 use PVE::JSONSchema qw(get_standard_option);
@@ -89,7 +90,7 @@ sub alloc_image {
     die "illegal name '$name' - should be 'vm-$vmid-*'\n"
 	if  $name && $name !~ m/^vm-$vmid-/;
 
-    my $vgs = PVE::Storage::LVMPlugin::lvm_vgs();
+    my $vgs = lvm_vgs();
 
     my $vg = $scfg->{vgname};
 
@@ -111,7 +112,7 @@ sub free_image {
 
     my $vg = $scfg->{vgname};
 
-    my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
+    my $lvs = lvm_list_volumes($vg);
 
     if (my $dat = $lvs->{$scfg->{vgname}}) {
 
@@ -137,7 +138,7 @@ sub list_images {
 
     my $vgname = $scfg->{vgname};
 
-    $cache->{lvs} = PVE::Storage::LVMPlugin::lvm_list_volumes() if !$cache->{lvs};
+    $cache->{lvs} = lvm_list_volumes() if !$cache->{lvs};
 
     my $res = [];
 
@@ -176,7 +177,7 @@ sub list_images {
 sub list_thinpools {
     my ($vg) = @_;
 
-    my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
+    my $lvs = lvm_list_volumes($vg);
     my $thinpools = [];
 
     foreach my $vg (keys %$lvs) {
@@ -195,7 +196,7 @@ sub list_thinpools {
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    my $lvs = $cache->{lvs} ||= PVE::Storage::LVMPlugin::lvm_list_volumes();
+    my $lvs = $cache->{lvs} ||= lvm_list_volumes();
 
     return if !$lvs->{$scfg->{vgname}};
 
@@ -214,7 +215,7 @@ sub status {
 my $activate_lv = sub {
     my ($vg, $lv, $cache) = @_;
 
-    my $lvs = $cache->{lvs} ||= PVE::Storage::LVMPlugin::lvm_list_volumes();
+    my $lvs = $cache->{lvs} ||= lvm_list_volumes();
 
     die "no such logical volume $vg/$lv\n" if !$lvs->{$vg} || !$lvs->{$vg}->{$lv};
 
@@ -295,7 +296,7 @@ sub create_base {
     die "create_base not possible with base image\n" if $isBase;
 
     my $vg = $scfg->{vgname};
-    my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
+    my $lvs = lvm_list_volumes($vg);
 
     if (my $dat = $lvs->{$vg}) {
 	# to avoid confusion, reject if we find volume snapshots
@@ -404,7 +405,7 @@ sub volume_import {
     } else {
 	my $tempname;
 	my $vg = $scfg->{vgname};
-	my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
+	my $lvs = lvm_list_volumes($vg);
 	if ($lvs->{$vg}->{$volname}) {
 	    die "volume $vg/$volname already exists\n" if !$allow_rename;
 	    warn "volume $vg/$volname already exists - importing with a different name\n";
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 12/36] plugin: lvmthin: move helper that lists thinpools to common LVM module

[Max Carrara]

and deprecate the original helper, emitting a warning if it's used.
Also document the moved subroutine.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common/LVM.pm    | 49 ++++++++++++++++++++++++++++++++
 src/PVE/Storage/LvmThinPlugin.pm | 30 +++++++------------
 2 files changed, 60 insertions(+), 19 deletions(-)

diff --git a/src/PVE/Storage/Common/LVM.pm b/src/PVE/Storage/Common/LVM.pm
index e0e3263..d4fa95f 100644
--- a/src/PVE/Storage/Common/LVM.pm
+++ b/src/PVE/Storage/Common/LVM.pm
@@ -16,6 +16,7 @@ our @EXPORT_OK = qw(
     lvm_destroy_volume_group
     lvm_vgs
     lvm_list_volumes
+    lvm_list_thinpools
     lvm_lvcreate
     lvm_lvrename
 );
@@ -380,6 +381,54 @@ sub lvm_list_volumes : prototype(;$) {
     return $lvs;
 }
 
+=pod
+
+=head3 lvm_list_thinpools
+
+    $thinpools = lvm_list_thinpools()
+    $thinpools = lvm_list_thinpools($vgname)
+
+Returns a list of hashes containing all I<thin pools> (I<logical volumes> with
+C<lv_type> B<C<t>>). May optionally be limited to a single I<volume group> by
+providing its name C<$vgname>.
+
+The returned list has the following structure:
+
+    [
+	{
+	    lv => 'lv-name-00',
+	    vg => 'vg-name-00',
+	},
+	{
+	    lv => 'lv-name-01',
+	    vg => 'vg-name-00',
+	},
+	...
+    ]
+
+See also: L<C<lvm_list_volumes>|/lvm_list_volumes>
+
+=cut
+
+sub lvm_list_thinpools : prototype(;$) {
+    my ($vg) = @_;
+
+    my $lvs = lvm_list_volumes($vg);
+    my $thinpools = [];
+
+    foreach my $vg (keys %$lvs) {
+	foreach my $lvname (keys %{$lvs->{$vg}}) {
+	    next if $lvs->{$vg}->{$lvname}->{lv_type} ne 't';
+	    my $lv = $lvs->{$vg}->{$lvname};
+	    $lv->{lv} = $lvname;
+	    $lv->{vg} = $vg;
+	    push @$thinpools, $lv;
+	}
+    }
+
+    return $thinpools;
+}
+
 =head3 lvm_lvcreate
 
     lvm_lvcreate($vgname, $name, $size, $tags)
diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 480cc78..1fcafdd 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -6,6 +6,7 @@ use warnings;
 use IO::File;
 
 use PVE::Tools qw(run_command trim);
+use PVE::Storage::Common qw(get_deprecation_warning);
 use PVE::Storage::Common::LVM qw(lvm_vgs lvm_list_volumes);
 use PVE::Storage::Plugin;
 use PVE::Storage::LVMPlugin;
@@ -25,6 +26,16 @@ use PVE::JSONSchema qw(get_standard_option);
 
 use base qw(PVE::Storage::LVMPlugin);
 
+sub list_thinpools {
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::LVM::lvm_list_thinpools"
+    );
+
+    my ($vgname) = @_;
+
+    return PVE::Storage::Common::LVM::lvm_list_thinpools($vgname);
+}
+
 sub type {
     return 'lvmthin';
 }
@@ -174,25 +185,6 @@ sub list_images {
     return $res;
 }
 
-sub list_thinpools {
-    my ($vg) = @_;
-
-    my $lvs = lvm_list_volumes($vg);
-    my $thinpools = [];
-
-    foreach my $vg (keys %$lvs) {
-	foreach my $lvname (keys %{$lvs->{$vg}}) {
-	    next if $lvs->{$vg}->{$lvname}->{lv_type} ne 't';
-	    my $lv = $lvs->{$vg}->{$lvname};
-	    $lv->{lv} = $lvname;
-	    $lv->{vg} = $vg;
-	    push @$thinpools, $lv;
-	}
-    }
-
-    return $thinpools;
-}
-
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 13/36] common: lvm: update code style

[Max Carrara]

in order to be more in line with our style guide [0]. This also
renames some parameters for clarity.

[0]: https://pve.proxmox.com/wiki/Perl_Style_Guide

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common/LVM.pm | 80 +++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/src/PVE/Storage/Common/LVM.pm b/src/PVE/Storage/Common/LVM.pm
index d4fa95f..2e010b6 100644
--- a/src/PVE/Storage/Common/LVM.pm
+++ b/src/PVE/Storage/Common/LVM.pm
@@ -167,7 +167,12 @@ sub lvm_create_volume_group : prototype($$$) {
     $cmd = ['/sbin/vgcreate', $vgname, $device];
     # push @$cmd, '-c', 'y' if $shared; # we do not use this yet
 
-    run_command($cmd, errmsg => "vgcreate $vgname $device error", errfunc => $ignore_no_medium_warnings, outfunc => $ignore_no_medium_warnings);
+    run_command(
+	$cmd,
+	errmsg => "vgcreate $vgname $device error",
+	errfunc => $ignore_no_medium_warnings,
+	outfunc => $ignore_no_medium_warnings
+    );
 }
 
 =pod
@@ -248,10 +253,10 @@ sub lvm_vgs : prototype(;$) {
     my $cols = [qw(vg_name vg_size vg_free lv_count)];
 
     if ($includepvs) {
-	push @$cols, qw(pv_name pv_size pv_free);
+	push $cols->@*, qw(pv_name pv_size pv_free);
     }
 
-    push @$cmd, join(',', @$cols);
+    push $cmd->@*, join(',', @$cols);
 
     my $vgs = {};
     eval {
@@ -268,7 +273,7 @@ sub lvm_vgs : prototype(;$) {
 	    };
 
 	    if (defined($pvname) && defined($pvsize) && defined($pvfree)) {
-		push @{$vgs->{$name}->{pvs}}, {
+		push $vgs->{$name}->{pvs}->@*, {
 		    name => $pvname,
 		    size => int($pvsize),
 		    free => int($pvfree),
@@ -333,7 +338,20 @@ The returned hash has the following structure:
 sub lvm_list_volumes : prototype(;$) {
     my ($vgname) = @_;
 
-    my $option_list = 'vg_name,lv_name,lv_size,lv_attr,pool_lv,data_percent,metadata_percent,snap_percent,uuid,tags,metadata_size,time';
+    my $option_list = join(',', qw(
+	vg_name
+	lv_name
+	lv_size
+	lv_attr
+	pool_lv
+	data_percent
+	metadata_percent
+	snap_percent
+	uuid
+	tags
+	metadata_size
+	time
+    ));
 
     my $cmd = [
 	'/sbin/lvs', '--separator', ':', '--noheadings', '--units', 'b',
@@ -342,7 +360,7 @@ sub lvm_list_volumes : prototype(;$) {
 	'--options', $option_list,
     ];
 
-    push @$cmd, $vgname if $vgname;
+    push $cmd->@*, $vgname if $vgname;
 
     my $lvs = {};
     run_command($cmd, outfunc => sub {
@@ -350,7 +368,21 @@ sub lvm_list_volumes : prototype(;$) {
 
 	$line = trim($line);
 
-	my ($vg_name, $lv_name, $lv_size, $lv_attr, $pool_lv, $data_percent, $meta_percent, $snap_percent, $uuid, $tags, $meta_size, $ctime) = split(':', $line);
+	my (
+	    $vg_name,
+	    $lv_name,
+	    $lv_size,
+	    $lv_attr,
+	    $pool_lv,
+	    $data_percent,
+	    $meta_percent,
+	    $snap_percent,
+	    $uuid,
+	    $tags,
+	    $meta_size,
+	    $ctime
+	) = split(':', $line);
+
 	return if !$vg_name;
 	return if !$lv_name;
 
@@ -370,8 +402,8 @@ sub lvm_list_volumes : prototype(;$) {
 	    $meta_percent ||= 0;
 	    $snap_percent ||= 0;
 	    $d->{metadata_size} = int($meta_size);
-	    $d->{metadata_used} = int(($meta_percent * $meta_size)/100);
-	    $d->{used} = int(($data_percent * $lv_size)/100);
+	    $d->{metadata_used} = int(($meta_percent * $meta_size) / 100);
+	    $d->{used} = int(($data_percent * $lv_size) / 100);
 	}
 	$lvs->{$vg_name}->{$lv_name} = $d;
     },
@@ -411,18 +443,20 @@ See also: L<C<lvm_list_volumes>|/lvm_list_volumes>
 =cut
 
 sub lvm_list_thinpools : prototype(;$) {
-    my ($vg) = @_;
+    my ($vgname) = @_;
 
-    my $lvs = lvm_list_volumes($vg);
+    my $lvs = lvm_list_volumes($vgname);
     my $thinpools = [];
 
-    foreach my $vg (keys %$lvs) {
-	foreach my $lvname (keys %{$lvs->{$vg}}) {
-	    next if $lvs->{$vg}->{$lvname}->{lv_type} ne 't';
-	    my $lv = $lvs->{$vg}->{$lvname};
+    for my $vgname (keys $lvs->%*) {
+	for my $lvname (keys $lvs->{$vgname}->%*) {
+	    next if $lvs->{$vgname}->{$lvname}->{lv_type} ne 't';
+
+	    my $lv = $lvs->{$vgname}->{$lvname};
 	    $lv->{lv} = $lvname;
-	    $lv->{vg} = $vg;
-	    push @$thinpools, $lv;
+	    $lv->{vg} = $vgname;
+
+	    push $thinpools->@*, $lv;
 	}
     }
 
@@ -443,19 +477,19 @@ C<"50g"> (50 gibibytes), C<"1T"> (1 terabyte), etc.
 =cut
 
 sub lvm_lvcreate : prototype($$$;$) {
-    my ($vg, $name, $size, $tags) = @_;
+    my ($vgname, $name, $size, $tags) = @_;
 
     if ($size =~ m/\d$/) { # no unit is given
-	$size .= "k"; # default to kilobytes
+	$size .= "k"; # default to kibibytes
     }
 
     my $cmd = ['/sbin/lvcreate', '-aly', '-Wy', '--yes', '--size', $size, '--name', $name];
-    for my $tag (@$tags) {
-	push @$cmd, '--addtag', $tag;
+    for my $tag ($tags->@*) {
+	push $cmd->@*, '--addtag', $tag;
     }
-    push @$cmd, $vg;
+    push $cmd->@*, $vgname;
 
-    run_command($cmd, errmsg => "lvcreate '$vg/$name' error");
+    run_command($cmd, errmsg => "lvcreate '$vgname/$name' error");
 }
 
 =pod
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 14/36] api: replace usages of deprecated LVM thin pool helper sub

[Max Carrara]

with the "new" sub in Common::LVM.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/API2/Disks/LVMThin.pm | 6 +++---
 src/PVE/API2/Storage/Scan.pm  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/PVE/API2/Disks/LVMThin.pm b/src/PVE/API2/Disks/LVMThin.pm
index bd46478..142a062 100644
--- a/src/PVE/API2/Disks/LVMThin.pm
+++ b/src/PVE/API2/Disks/LVMThin.pm
@@ -3,7 +3,7 @@ package PVE::API2::Disks::LVMThin;
 use strict;
 use warnings;
 
-use PVE::Storage::Common::LVM qw(lvm_vgs lvm_create_volume_group lvm_pv_info);
+use PVE::Storage::Common::LVM qw(lvm_vgs lvm_create_volume_group lvm_pv_info lvm_list_thinpools);
 use PVE::Diskmanage;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::API2::Storage::Config;
@@ -65,7 +65,7 @@ __PACKAGE__->register_method ({
     },
     code => sub {
 	my ($param) = @_;
-	return PVE::Storage::LvmThinPlugin::list_thinpools(undef);
+	return lvm_list_thinpools(undef);
     }});
 
 __PACKAGE__->register_method ({
@@ -221,7 +221,7 @@ __PACKAGE__->register_method ({
 
 	my $worker = sub {
 	    PVE::Diskmanage::locked_disk_action(sub {
-		my $thinpools = PVE::Storage::LvmThinPlugin::list_thinpools();
+		my $thinpools = lvm_list_thinpools();
 
 		die "no such thin pool ${vg}/${lv}\n"
 		    if !grep { $_->{lv} eq $lv && $_->{vg} eq $vg } $thinpools->@*;
diff --git a/src/PVE/API2/Storage/Scan.pm b/src/PVE/API2/Storage/Scan.pm
index bad280d..5bb9883 100644
--- a/src/PVE/API2/Storage/Scan.pm
+++ b/src/PVE/API2/Storage/Scan.pm
@@ -8,7 +8,7 @@ use warnings;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTHandler;
 use PVE::SafeSyslog;
-use PVE::Storage::Common::LVM qw(lvm_vgs);
+use PVE::Storage::Common::LVM qw(lvm_vgs lvm_list_thinpools);
 use PVE::Storage;
 use PVE::SysFSTools;
 
@@ -409,7 +409,7 @@ __PACKAGE__->register_method({
     code => sub {
 	my ($param) = @_;
 
-	return PVE::Storage::LvmThinPlugin::list_thinpools($param->{vg});
+	return lvm_list_thinpools($param->{vg});
     }});
 
 __PACKAGE__->register_method({
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 15/36] plugin: btrfs: replace deprecated helpers from directory plugin

[Max Carrara]

with the recently moved ones from `Common` and `Common::Path`.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/BTRFSPlugin.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index a503404..10fd441 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -13,6 +13,8 @@ use POSIX qw(EEXIST);
 
 use PVE::Tools qw(run_command dir_glob_foreach);
 
+use PVE::Storage::Common qw(storage_parse_is_mountpoint);
+use PVE::Storage::Common::Path qw(path_is_mounted);
 use PVE::Storage::DirPlugin;
 
 use constant {
@@ -122,8 +124,8 @@ sub activate_storage {
     my $path = $scfg->{path};
     $class->config_aware_base_mkdir($scfg, $path);
 
-    my $mp = PVE::Storage::DirPlugin::parse_is_mountpoint($scfg);
-    if (defined($mp) && !PVE::Storage::DirPlugin::path_is_mounted($mp, $cache->{mountdata})) {
+    my $mp = storage_parse_is_mountpoint($scfg);
+    if (defined($mp) && !path_is_mounted($mp, $cache->{mountdata})) {
 	die "unable to activate storage '$storeid' - directory is expected to be a mount point but"
 	." is not mounted: '$mp'\n";
     }
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 16/36] plugin: dir: factor storage methods into separate common subs

[Max Carrara]

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 <m.carrara@proxmox.com>
---
 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<DEPRECATED:> This helper is deprecated in favour of
+C<L</storage_dir_get_volume_attribute>> and will be removed in the future.
+
+This is a general implementation of C<L<PVE::Storage::Plugin::get_volume_notes>>
+that may be used by storage plugins with a directory-based storage. It is therefore
+useful for something like L<an NFS storage|PVE::Storage::NFSPlugin>, 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<DEPRECATED:> This helper is deprecated in favour of
+C<L</storage_dir_update_volume_attribute>> and will be removed in the future.
+
+This is a general implementation of C<L<PVE::Storage::Plugin::update_volume_notes>>
+that may be used by storage plugins with a directory-based storage. It is therefore
+useful for something like L<an NFS storage|PVE::Storage::NFSPlugin>, 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<L<PVE::Storage::Plugin::get_volume_attribute>>
+that may be used by storage plugins with a directory-based storage. It is therefore
+useful for something like L<an NFS storage|PVE::Storage::NFSPlugin>, 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<undef> 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<L<PVE::Storage::Plugin::get_volume_attribute>>
+that may be used by storage plugins with a directory-based storage. It is therefore
+useful for something like L<an NFS storage|PVE::Storage::NFSPlugin>, 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

[pve-devel] [RFC pve-storage 17/36] plugin: dir: factor path validity check into helper methods

[Max Carrara]

Whether a directory-based storage's path is valid or not should not be
solely decided within a method of the directoy plugin, but should
instead be available to other plugins, possibly third-party plugins,
as well.

Therefore, factor that check into three different helper functions in
`Common::Path`, so that they may be re-used by other plugins in the
future. Document the helper functions as well.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common/Path.pm | 73 ++++++++++++++++++++++++++++++++++
 src/PVE/Storage/DirPlugin.pm   |  4 +-
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/Common/Path.pm b/src/PVE/Storage/Common/Path.pm
index 7535dda..b9072bf 100644
--- a/src/PVE/Storage/Common/Path.pm
+++ b/src/PVE/Storage/Common/Path.pm
@@ -11,6 +11,9 @@ use parent qw(Exporter);
 
 our @EXPORT_OK = qw(
     path_is_mounted
+    path_is_absolute
+    path_contains_valid_chars
+    path_is_storage_dir
 );
 
 =pod
@@ -48,4 +51,74 @@ sub path_is_mounted {
     return undef;
 }
 
+=pod
+
+=head3 path_is_absolute
+
+    $result = path_is_absolute($path)
+
+Checks whether a C<$path> is absolute.
+
+Will return C<undef> if C<$path> is C<undef>, or a boolean otherwise.
+
+=cut
+
+sub path_is_absolute : prototype($) {
+    my ($path) = @_;
+
+    return undef if !defined($path);
+
+    return ($path =~ m|^/|) + 0; # convert to number
+}
+
+=pod
+
+=head3 path_contains_valid_chars
+
+    $result = path_contains_valid_chars($path)
+
+Checks whether a C<$path> contains only valid characters.
+
+"Valid" in this context means "the characters that we allow". While Unix/Linux/POSIX
+paths L<may contain almost any sequence of bytes|https://lwn.net/Articles/71472/>,
+I<allowing> almost any sequence of bytes can lead to many unforeseen issues.
+See L<this|https://dwheeler.com/essays/fixing-unix-linux-filenames.html> for more
+information.
+
+Valid characters are the letters C<a-z> as well as their uppercase variants
+C<A-Z>, the numbers C<0-9> and the symbols C<->, C</>, C<_>, C<.> and C<@>.
+
+Will return C<undef> if C<$path> is C<undef>, or a boolean otherwise.
+
+=cut
+
+sub path_contains_valid_chars : prototype($) {
+    my ($path) = @_;
+
+    return undef if !defined($path);
+
+    return ($path =~ m|[-/a-zA-Z0-9_.@]+|) + 0; # convert to number
+}
+
+
+=pod
+
+=head3 path_is_storage_dir
+
+    $result = path_is_storage_dir($path)
+
+Shorthand for C<L</path_is_absolute>> C<&&> C<L</path_contains_valid_chars>>.
+
+Will return C<undef> if C<$path> is C<undef>, or a boolean otherwise.
+
+=cut
+
+sub path_is_storage_dir : prototype($) {
+    my ($path) = @_;
+
+    return undef if !defined($path);
+
+    return path_is_absolute($path) && path_contains_valid_chars($path);
+}
+
 1;
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index f6e1d73..4be39f9 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -187,9 +187,11 @@ sub check_config {
     my ($self, $sectionId, $config, $create, $skipSchemaCheck) = @_;
     my $opts = PVE::SectionConfig::check_config($self, $sectionId, $config, $create, $skipSchemaCheck);
     return $opts if !$create;
-    if ($opts->{path} !~ m|^/[-/a-zA-Z0-9_.@]+$|) {
+
+    if (!PVE::Storage::Common::Path::path_is_storage_dir($opts->{path})) {
 	die "illegal path for directory storage: $opts->{path}\n";
     }
+
     return $opts;
 }
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 18/36] plugin: btrfs: remove dependency on directory plugin

[Max Carrara]

This commit removes the BTRFS plugin's dependency on the directory
plugin. This is done by replacing calls to the dir plugin's methods
with the corresponding helper subroutines from the `Common` module
instead.

Furthermore, the methods `check_config` and `status` both get their
own implementations that behave essentially the same as the ones from
the dir plugin.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/BTRFSPlugin.pm | 49 +++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 10fd441..daff8a4 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -13,9 +13,15 @@ use POSIX qw(EEXIST);
 
 use PVE::Tools qw(run_command dir_glob_foreach);
 
-use PVE::Storage::Common qw(storage_parse_is_mountpoint);
-use PVE::Storage::Common::Path qw(path_is_mounted);
-use PVE::Storage::DirPlugin;
+use PVE::Storage::Common qw(
+    storage_parse_is_mountpoint
+    storage_dir_get_volume_attribute
+    storage_dir_update_volume_attribute
+);
+use PVE::Storage::Common::Path qw(
+    path_is_mounted
+    path_is_storage_dir
+);
 
 use constant {
     BTRFS_FIRST_FREE_OBJECTID => 256,
@@ -92,10 +98,16 @@ sub options {
 #   -> `images/VMID/vm-VMID-disk-ID/disk.raw`
 #   where the `vm-VMID-disk-ID/` subdirectory is a btrfs subvolume
 
-# Reuse `DirPlugin`'s `check_config`. This simply checks for invalid paths.
 sub check_config {
     my ($self, $sectionId, $config, $create, $skipSchemaCheck) = @_;
-    return PVE::Storage::DirPlugin::check_config($self, $sectionId, $config, $create, $skipSchemaCheck);
+    my $opts = PVE::SectionConfig::check_config($self, $sectionId, $config, $create, $skipSchemaCheck);
+    return $opts if !$create;
+
+    if (!path_is_storage_dir($opts->{path})) {
+	die "illegal path for directory of BTRFS storage: $opts->{path}\n";
+    }
+
+    return $opts;
 }
 
 my sub getfsmagic($) {
@@ -137,23 +149,30 @@ sub activate_storage {
 
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
-    return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
+
+    if (defined(my $mp = storage_parse_is_mountpoint($scfg))) {
+	$cache->{mountdata} = PVE::ProcFSTools::parse_proc_mounts()
+	    if !$cache->{mountdata};
+
+	return undef if !path_is_mounted($mp, $cache->{mountdata});
+    }
+
+    return $class->SUPER::status($storeid, $scfg, $cache);
 }
 
 sub get_volume_attribute {
     my ($class, $scfg, $storeid, $volname, $attribute) = @_;
-    return PVE::Storage::DirPlugin::get_volume_attribute($class, $scfg, $storeid, $volname, $attribute);
+
+    return storage_dir_get_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute
+    );
 }
 
 sub update_volume_attribute {
     my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
-    return PVE::Storage::DirPlugin::update_volume_attribute(
-	$class,
-	$scfg,
-	$storeid,
-	$volname,
-	$attribute,
-	$value,
+
+    return storage_dir_update_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute, $value
     );
 }
 
@@ -295,7 +314,7 @@ sub clone_image {
     # If we're not working with a 'raw' file, which is the only thing that's "different" for btrfs,
     # or a subvolume, we forward to the DirPlugin
     if ($format ne 'raw' && $format ne 'subvol') {
-	return PVE::Storage::DirPlugin::clone_image(@_);
+	return $class->SUPER::clone_image(@_);
     }
 
     my $imagedir = $class->get_subdir($scfg, 'images');
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 19/36] plugin: cifs: remove dependency on directory plugin

[Max Carrara]

As with the BTRFS plugin, the dependency on the directory plugin is
removed by using the helpers from the `Common` module instead of
calling the dir plugin's methods.

Deprecation warnings for the `get_volume_notes` and
`update_volume_notes` methods are also added.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/CIFSPlugin.pm | 41 ++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/CIFSPlugin.pm b/src/PVE/Storage/CIFSPlugin.pm
index 18dc017..ae51be8 100644
--- a/src/PVE/Storage/CIFSPlugin.pm
+++ b/src/PVE/Storage/CIFSPlugin.pm
@@ -6,6 +6,13 @@ use Net::IP;
 use PVE::Tools qw(run_command);
 use PVE::ProcFSTools;
 use File::Path;
+use PVE::Storage::Common qw(
+    get_deprecation_warning
+    storage_dir_get_volume_notes
+    storage_dir_update_volume_notes
+    storage_dir_get_volume_attribute
+    storage_dir_update_volume_attribute
+);
 use PVE::Storage::Plugin;
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -295,23 +302,45 @@ sub check_connection {
 # 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, @_);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::storage_dir_get_volume_notes"
+    );
+
+    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+
+    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 {
-    my $class = shift;
-    PVE::Storage::DirPlugin::update_volume_notes($class, @_);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::storage_dir_update_volume_notes"
+    );
+
+    my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
+
+    return storage_dir_update_volume_notes(
+	$class, $scfg, $storeid, $volname, $notes, $timeout
+    );
 }
 
 sub get_volume_attribute {
-    return PVE::Storage::DirPlugin::get_volume_attribute(@_);
+    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+    return storage_dir_get_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute
+    );
 }
 
 sub update_volume_attribute {
-    return PVE::Storage::DirPlugin::update_volume_attribute(@_);
+    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+    return storage_dir_update_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute, $value
+    );
 }
 
 1;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 20/36] plugin: cephfs: remove dependency on directory plugin

[Max Carrara]

As with the BTRFS plugin, the dependency on the directory plugin is
removed by using the helpers from the `Common` module instead of
calling the dir plugin's methods.

Deprecation warnings for the `get_volume_notes` and
`update_volume_notes` methods are also added.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/CephFSPlugin.pm | 41 ++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/CephFSPlugin.pm b/src/PVE/Storage/CephFSPlugin.pm
index 98d1ba6..3be7259 100644
--- a/src/PVE/Storage/CephFSPlugin.pm
+++ b/src/PVE/Storage/CephFSPlugin.pm
@@ -10,6 +10,13 @@ use File::Path;
 use PVE::CephConfig;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::ProcFSTools;
+use PVE::Storage::Common qw(
+    get_deprecation_warning
+    storage_dir_get_volume_notes
+    storage_dir_update_volume_notes
+    storage_dir_get_volume_attribute
+    storage_dir_update_volume_attribute
+);
 use PVE::Storage::Plugin;
 use PVE::Systemd;
 use PVE::Tools qw(run_command file_set_contents);
@@ -242,23 +249,45 @@ 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, @_);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::storage_dir_get_volume_notes"
+    );
+
+    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+
+    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 {
-    my $class = shift;
-    PVE::Storage::DirPlugin::update_volume_notes($class, @_);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::storage_dir_update_volume_notes"
+    );
+
+    my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
+
+    return storage_dir_update_volume_notes(
+	$class, $scfg, $storeid, $volname, $notes, $timeout
+    );
 }
 
 sub get_volume_attribute {
-    return PVE::Storage::DirPlugin::get_volume_attribute(@_);
+    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+    return storage_dir_get_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute
+    );
 }
 
 sub update_volume_attribute {
-    return PVE::Storage::DirPlugin::update_volume_attribute(@_);
+    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+    return storage_dir_update_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute, $value
+    );
 }
 
 1;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 21/36] plugin: nfs: remove dependency on directory plugin

[Max Carrara]

As with the BTRFS plugin, the dependency on the directory plugin is
removed by using the helpers from the `Common` module instead of
calling the dir plugin's methods.

Deprecation warnings for the `get_volume_notes` and
`update_volume_notes` methods are also added.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/NFSPlugin.pm | 41 ++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
index f2e4c0d..0d02b49 100644
--- a/src/PVE/Storage/NFSPlugin.pm
+++ b/src/PVE/Storage/NFSPlugin.pm
@@ -9,6 +9,13 @@ use File::Path;
 use PVE::Network;
 use PVE::Tools qw(run_command);
 use PVE::ProcFSTools;
+use PVE::Storage::Common qw(
+    get_deprecation_warning
+    storage_dir_get_volume_notes
+    storage_dir_update_volume_notes
+    storage_dir_get_volume_attribute
+    storage_dir_update_volume_attribute
+);
 use PVE::Storage::Plugin;
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -204,23 +211,45 @@ sub check_connection {
 # 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, @_);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::storage_dir_get_volume_notes"
+    );
+
+    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+
+    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 {
-    my $class = shift;
-    PVE::Storage::DirPlugin::update_volume_notes($class, @_);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::storage_dir_update_volume_notes"
+    );
+
+    my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
+
+    return storage_dir_update_volume_notes(
+	$class, $scfg, $storeid, $volname, $notes, $timeout
+    );
 }
 
 sub get_volume_attribute {
-    return PVE::Storage::DirPlugin::get_volume_attribute(@_);
+    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+    return storage_dir_get_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute
+    );
 }
 
 sub update_volume_attribute {
-    return PVE::Storage::DirPlugin::update_volume_attribute(@_);
+    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+    return storage_dir_update_volume_attribute(
+	$class, $scfg, $storeid, $volname, $attribute, $value
+    );
 }
 
 1;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 22/36] plugin: btrfs: make helper methods private

[Max Carrara]

The methods `btrfs_cmd` and `btrfs_get_subvol_id` are made private in
order to prevent them from accidentally becoming part of the plugin's
public interface in the future.

The call sites are adapted accordingly, due to the methods not being
accessible by reference via `$class` anymore. Therefore, calls like

  $class->btrfs_cmd(['some', 'command']);

are changed to

  btrfs_cmd($class, ['some', 'command']);

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/BTRFSPlugin.pm | 48 +++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index daff8a4..24694a6 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -234,7 +234,7 @@ sub filesystem_path {
     return wantarray ? ($path, $vmid, $vtype) : $path;
 }
 
-sub btrfs_cmd {
+my sub btrfs_cmd {
     my ($class, $cmd, $outfunc) = @_;
 
     my $msg = '';
@@ -252,9 +252,9 @@ sub btrfs_cmd {
     return $msg;
 }
 
-sub btrfs_get_subvol_id {
+my sub btrfs_get_subvol_id {
     my ($class, $path) = @_;
-    my $info = $class->btrfs_cmd(['subvolume', 'show', '--', $path]);
+    my $info = btrfs_cmd($class, ['subvolume', 'show', '--', $path]);
     if ($info !~ /^\s*(?:Object|Subvolume) ID:\s*(\d+)$/m) {
 	die "failed to get btrfs subvolume ID from: $info\n";
     }
@@ -299,7 +299,7 @@ sub create_base {
 
     rename($subvol, $newsubvol)
 	|| die "rename '$subvol' to '$newsubvol' failed - $!\n";
-    eval { $class->btrfs_cmd(['property', 'set', $newsubvol, 'ro', 'true']) };
+    eval { btrfs_cmd($class, ['property', 'set', $newsubvol, 'ro', 'true']) };
     warn $@ if $@;
 
     return $newvolname;
@@ -336,7 +336,7 @@ sub clone_image {
 	$newsubvol = raw_file_to_subvol($newsubvol);
     }
 
-    $class->btrfs_cmd(['subvolume', 'snapshot', '--', $subvol, $newsubvol]);
+    btrfs_cmd($class, ['subvolume', 'snapshot', '--', $subvol, $newsubvol]);
 
     return $newvolname;
 }
@@ -380,7 +380,7 @@ sub alloc_image {
 	die "btrfs quotas are currently not supported, use an unsized subvolume or a raw file\n";
     }
 
-    $class->btrfs_cmd(['subvolume', 'create', '--', $subvol]);
+    btrfs_cmd($class, ['subvolume', 'create', '--', $subvol]);
 
     eval {
 	if ($fmt eq 'subvol') {
@@ -391,9 +391,9 @@ sub alloc_image {
 	    # eval {
 	    #     # This call should happen at storage creation instead and therefore governed by a
 	    #     # configuration option!
-	    #     # $class->btrfs_cmd(['quota', 'enable', $subvol]);
-	    #     my $id = $class->btrfs_get_subvol_id($subvol);
-	    #     $class->btrfs_cmd(['qgroup', 'limit', "${size}k", "0/$id", $subvol]);
+	    #     # btrfs_cmd($class, ['quota', 'enable', $subvol]);
+	    #     my $id = btrfs_get_subvol_id($class, $subvol);
+	    #     btrfs_cmd($class, ['qgroup', 'limit', "${size}k", "0/$id", $subvol]);
 	    # };
 	} elsif ($fmt eq 'raw') {
 	    sysopen my $fh, $path, O_WRONLY | O_CREAT | O_EXCL
@@ -408,7 +408,7 @@ sub alloc_image {
     };
 
     if (my $err = $@) {
-	eval { $class->btrfs_cmd(['subvolume', 'delete', '--', $subvol]); };
+	eval { btrfs_cmd($class, ['subvolume', 'delete', '--', $subvol]); };
 	warn $@ if $@;
 	die $err;
     }
@@ -466,7 +466,7 @@ sub free_image {
 	push @snapshot_vols, "$dir/$volume";
     });
 
-    $class->btrfs_cmd(['subvolume', 'delete', '--', @snapshot_vols, $subvol]);
+    btrfs_cmd($class, ['subvolume', 'delete', '--', @snapshot_vols, $subvol]);
     # try to cleanup directory to not clutter storage with empty $vmid dirs if
     # all images from a guest got deleted
     rmdir($dir);
@@ -477,10 +477,10 @@ sub free_image {
 # Currently not used because quotas clash with send/recv.
 # my sub btrfs_subvol_quota {
 #     my ($class, $path) = @_;
-#     my $id = '0/' . $class->btrfs_get_subvol_id($path);
+#     my $id = '0/' . btrfs_get_subvol_id($class, $path);
 #     my $search = qr/^\Q$id\E\s+(\d)+\s+\d+\s+(\d+)\s*$/;
 #     my ($used, $size);
-#     $class->btrfs_cmd(['qgroup', 'show', '--raw', '-rf', '--', $path], sub {
+#     btrfs_cmd($class, ['qgroup', 'show', '--raw', '-rf', '--', $path], sub {
 # 	return if defined($size);
 # 	if ($_[0] =~ $search) {
 # 	    ($used, $size) = ($1, $2);
@@ -519,8 +519,8 @@ sub volume_resize {
     my $format = ($class->parse_volname($volname))[6];
     if ($format eq 'subvol') {
 	my $path = $class->filesystem_path($scfg, $volname);
-	my $id = '0/' . $class->btrfs_get_subvol_id($path);
-	$class->btrfs_cmd(['qgroup', 'limit', '--', "${size}k", "0/$id", $path]);
+	my $id = '0/' . btrfs_get_subvol_id($class, $path);
+	btrfs_cmd($class, ['qgroup', 'limit', '--', "${size}k", "0/$id", $path]);
 	return undef;
     }
 
@@ -546,7 +546,7 @@ sub volume_snapshot {
     my $snapshot_dir = $class->get_subdir($scfg, 'images') . "/$vmid";
     mkpath $snapshot_dir;
 
-    $class->btrfs_cmd(['subvolume', 'snapshot', '-r', '--', $path, $snap_path]);
+    btrfs_cmd($class, ['subvolume', 'snapshot', '-r', '--', $path, $snap_path]);
     return undef;
 }
 
@@ -580,11 +580,11 @@ sub volume_snapshot_rollback {
     # But for atomicity in case the rename after create-failure *also* fails, we create the new
     # subvol first, then use RENAME_EXCHANGE, 
     my $tmp_path = "$path.tmp.$$";
-    $class->btrfs_cmd(['subvolume', 'snapshot', '--', $snap_path, $tmp_path]);
+    btrfs_cmd($class, ['subvolume', 'snapshot', '--', $snap_path, $tmp_path]);
     # The paths are absolute, so pass -1 as file descriptors.
     my $ok = PVE::Tools::renameat2(-1, $tmp_path, -1, $path, &PVE::Tools::RENAME_EXCHANGE);
 
-    eval { $class->btrfs_cmd(['subvolume', 'delete', '--', $tmp_path]) };
+    eval { btrfs_cmd($class, ['subvolume', 'delete', '--', $tmp_path]) };
     warn "failed to remove '$tmp_path' subvolume: $@" if $@;
 
     if (!$ok) {
@@ -609,7 +609,7 @@ sub volume_snapshot_delete {
 	$path = raw_file_to_subvol($path);
     }
 
-    $class->btrfs_cmd(['subvolume', 'delete', '--', $path]);
+    btrfs_cmd($class, ['subvolume', 'delete', '--', $path]);
 
     return undef;
 }
@@ -888,7 +888,7 @@ sub volume_import {
 	# Rotate the disk into place, first the current state:
 	# Note that read-only subvolumes cannot be moved into different directories, but for the
 	# "current" state we also want a writable copy, so start with that:
-	$class->btrfs_cmd(['property', 'set', "$tmppath/$diskname\@$snapshot", 'ro', 'false']);
+	btrfs_cmd($class, ['property', 'set', "$tmppath/$diskname\@$snapshot", 'ro', 'false']);
 	PVE::Tools::renameat2(
 	    -1,
 	    "$tmppath/$diskname\@$snapshot",
@@ -899,7 +899,7 @@ sub volume_import {
 	    . " into place at '$destination' - $!\n";
 
 	# Now recreate the actual snapshot:
-	$class->btrfs_cmd([
+	btrfs_cmd($class, [
 	    'subvolume',
 	    'snapshot',
 	    '-r',
@@ -910,7 +910,7 @@ sub volume_import {
 
 	# Now go through the remaining snapshots (if any)
 	foreach my $snap (@snapshots) {
-	    $class->btrfs_cmd(['property', 'set', "$tmppath/$diskname\@$snap", 'ro', 'false']);
+	    btrfs_cmd($class, ['property', 'set', "$tmppath/$diskname\@$snap", 'ro', 'false']);
 	    PVE::Tools::renameat2(
 		-1,
 		"$tmppath/$diskname\@$snap",
@@ -919,7 +919,7 @@ sub volume_import {
 		&PVE::Tools::RENAME_NOREPLACE,
 	    ) or die "failed to move received snapshot '$tmppath/$diskname\@$snap'"
 		. " into place at '$destination\@$snap' - $!\n";
-	    eval { $class->btrfs_cmd(['property', 'set', "$destination\@$snap", 'ro', 'true']) };
+	    eval { btrfs_cmd($class, ['property', 'set', "$destination\@$snap", 'ro', 'true']) };
 	    warn "failed to make $destination\@$snap read-only - $!\n" if $@;
 	}
     };
@@ -932,7 +932,7 @@ sub volume_import {
 	    $dh->rewind;
 	    while (defined(my $entry = $dh->read)) {
 		next if $entry eq '.' || $entry eq '..';
-		eval { $class->btrfs_cmd(['subvolume', 'delete', '--', "$tmppath/$entry"]) };
+		eval { btrfs_cmd($class, ['subvolume', 'delete', '--', "$tmppath/$entry"]) };
 		warn $@ if $@;
 	    }
 	    $dh->close; undef $dh;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 23/36] plugin: esxi: make helper subroutines private

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/ESXiPlugin.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm
index a35693d..d4b6afc 100644
--- a/src/PVE/Storage/ESXiPlugin.pm
+++ b/src/PVE/Storage/ESXiPlugin.pm
@@ -59,12 +59,12 @@ sub options {
    };
 }
 
-sub esxi_cred_file_name {
+my sub esxi_cred_file_name {
     my ($storeid) = @_;
     return "/etc/pve/priv/storage/${storeid}.pw";
 }
 
-sub esxi_delete_credentials {
+my sub esxi_delete_credentials {
     my ($storeid) = @_;
 
     if (my $cred_file = get_cred_file($storeid)) {
@@ -72,7 +72,7 @@ sub esxi_delete_credentials {
     }
 }
 
-sub esxi_set_credentials {
+my sub esxi_set_credentials {
     my ($password, $storeid) = @_;
 
     my $cred_file = esxi_cred_file_name($storeid);
@@ -83,7 +83,7 @@ sub esxi_set_credentials {
     return $cred_file;
 }
 
-sub get_cred_file {
+my sub get_cred_file {
     my ($storeid) = @_;
 
     my $cred_file = esxi_cred_file_name($storeid);
@@ -267,7 +267,7 @@ sub esxi_unmount : prototype($$$) {
 }
 
 # Split a path into (datacenter, datastore, path)
-sub split_path : prototype($) {
+my sub split_path : prototype($) {
     my ($path) = @_;
     if ($path =~ m!^([^/]+)/([^/]+)/(.+)$!) {
 	return ($1, $2, $3);
@@ -752,7 +752,7 @@ sub vmx_path { $_[0]->{'pve.vmx.path'} }
 sub manifest { $_[0]->{'pve.manifest'} }
 
 # (Also used for the fileName config key...)
-sub is_disk_entry : prototype($) {
+my sub is_disk_entry : prototype($) {
     my ($id) = @_;
     if ($id =~ /^(scsi|ide|sata|nvme)(\d+:\d+)(:?\.fileName)?$/) {
 	return ($1, $2);
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 24/36] plugin: esxi: remove unused helper subroutine `query_vmdk_size`

[Max Carrara]

This subroutine does not appear to be used in any of our Perl code, so
remove it.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/ESXiPlugin.pm | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm
index d4b6afc..2bab7f8 100644
--- a/src/PVE/Storage/ESXiPlugin.pm
+++ b/src/PVE/Storage/ESXiPlugin.pm
@@ -299,24 +299,6 @@ sub get_import_metadata : prototype($$$$$) {
     return $vmx->get_create_args();
 }
 
-# Returns a size in bytes, this is a helper for already-mounted files.
-sub query_vmdk_size : prototype($;$) {
-    my ($filename, $timeout) = @_;
-
-    my $json = eval {
-	my $json = '';
-	run_command(['/usr/bin/qemu-img', 'info', '--output=json', $filename],
-	    timeout => $timeout,
-	    outfunc => sub { $json .= $_[0]; },
-	    errfunc => sub { warn "$_[0]\n"; }
-	);
-	from_json($json)
-    };
-    warn $@ if $@;
-
-    return int($json->{'virtual-size'});
-}
-
 #
 # Storage API implementation
 #
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 25/36] plugin: esxi: make helper methods private

[Max Carrara]

The methods `get_manifest`, `esxi_mount` and `esxi_unmount` are made
private in order to prevent them from accidentally becoming part of
the plugin's public interface in the future.

Similar to the BTRFS changes, adapt the call sites accordingly. This
means that calls like

  my $manifest = $class->get_manifest($storeid, $scfg, 0);

are changed to

  my $manifest = get_manifest($class, $storeid, $scfg, 0);

because the methods cannot be accessed via the `$class` reference
anymore.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/ESXiPlugin.pm | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm
index 2bab7f8..355a33c 100644
--- a/src/PVE/Storage/ESXiPlugin.pm
+++ b/src/PVE/Storage/ESXiPlugin.pm
@@ -120,7 +120,7 @@ my sub is_old : prototype($) {
     return !defined($mtime) || ($mtime + 30) < CORE::time();
 }
 
-sub get_manifest : prototype($$$;$) {
+my sub get_manifest : prototype($$$;$) {
     my ($class, $storeid, $scfg, $force_query) = @_;
 
     my $rundir = run_path($storeid);
@@ -177,12 +177,12 @@ my sub is_mounted : prototype($) {
     return PVE::Systemd::is_unit_active($scope_name_base . '.scope');
 }
 
-sub esxi_mount : prototype($$$;$) {
+my sub esxi_mount : prototype($$$;$) {
     my ($class, $storeid, $scfg, $force_requery) = @_;
 
     return if !$force_requery && is_mounted($storeid);
 
-    $class->get_manifest($storeid, $scfg, $force_requery);
+    get_manifest($class, $storeid, $scfg, $force_requery);
 
     my $rundir = run_path($storeid);
     my $manifest_file = "$rundir/manifest.json";
@@ -253,7 +253,7 @@ sub esxi_mount : prototype($$$;$) {
     }
 }
 
-sub esxi_unmount : prototype($$$) {
+my sub esxi_unmount : prototype($$$) {
     my ($class, $storeid, $scfg) = @_;
 
     my $scope_name_base = scope_name_base($storeid);
@@ -287,7 +287,7 @@ sub get_import_metadata : prototype($$$$$) {
 	die "storage '$storeid' is not activated\n";
     }
 
-    my $manifest = $class->get_manifest($storeid, $scfg, 0);
+    my $manifest = get_manifest($class, $storeid, $scfg, 0);
     my $contents = file_get_contents($vmx_path);
     my $vmx = PVE::Storage::ESXiPlugin::VMX->parse(
 	$storeid,
@@ -351,13 +351,13 @@ sub on_delete_hook {
 sub activate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    $class->esxi_mount($storeid, $scfg, 0);
+    esxi_mount($class, $storeid, $scfg, 0);
 }
 
 sub deactivate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    $class->esxi_unmount($storeid, $scfg);
+    esxi_unmount($class, $storeid, $scfg);
 
     my $rundir = run_path($storeid);
     remove_tree($rundir); # best-effort, ignore errors for now
@@ -418,7 +418,7 @@ sub list_volumes {
 
     return if !grep { $_ eq 'import' } @$content_types;
 
-    my $data = $class->get_manifest($storeid, $scfg, 0);
+    my $data = get_manifest($class, $storeid, $scfg, 0);
 
     my $res = [];
     for my $dc_name (keys $data->%*) {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 26/36] plugin: gluster: make helper subroutines private

[Max Carrara]

.. and also use a regular sub definition for `get_active_server` in
order to avoid the sub prefix deref syntax when calling it.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/GlusterfsPlugin.pm | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/PVE/Storage/GlusterfsPlugin.pm b/src/PVE/Storage/GlusterfsPlugin.pm
index 2b7f9e1..634a090 100644
--- a/src/PVE/Storage/GlusterfsPlugin.pm
+++ b/src/PVE/Storage/GlusterfsPlugin.pm
@@ -16,7 +16,7 @@ use base qw(PVE::Storage::Plugin);
 
 my $server_test_results = {};
 
-my $get_active_server = sub {
+my sub get_active_server {
     my ($scfg, $return_default_if_offline) = @_;
 
     my $defaultserver = $scfg->{server} ? $scfg->{server} : 'localhost';
@@ -66,7 +66,7 @@ my $get_active_server = sub {
     return undef;
 };
 
-sub glusterfs_is_mounted {
+my sub glusterfs_is_mounted {
     my ($volume, $mountpoint, $mountdata) = @_;
 
     $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata;
@@ -79,7 +79,7 @@ sub glusterfs_is_mounted {
     return undef;
 }
 
-sub glusterfs_mount {
+my sub glusterfs_mount {
     my ($server, $volume, $mountpoint) = @_;
 
     my $source = "$server:$volume";
@@ -179,7 +179,7 @@ sub path {
     my $path = undef;
     if ($vtype eq 'images') {
 
-	my $server = &$get_active_server($scfg, 1);
+	my $server = get_active_server($scfg, 1);
 	my $glustervolume = $scfg->{volume};
 	my $transport = $scfg->{transport};
 	my $protocol = "gluster";
@@ -227,7 +227,7 @@ sub clone_image {
 
     die "disk image '$path' already exists\n" if -e $path;
 
-    my $server = &$get_active_server($scfg, 1);
+    my $server = get_active_server($scfg, 1);
     my $glustervolume = $scfg->{volume};
     my $volumepath = "gluster://$server/$glustervolume/images/$vmid/$name";
 
@@ -258,7 +258,7 @@ sub alloc_image {
 
     die "disk image '$path' already exists\n" if -e $path;
 
-    my $server = &$get_active_server($scfg, 1);
+    my $server = get_active_server($scfg, 1);
     my $glustervolume = $scfg->{volume};
     my $volumepath = "gluster://$server/$glustervolume/images/$vmid/$name";
 
@@ -309,7 +309,7 @@ sub activate_storage {
 	die "unable to activate storage '$storeid' - " .
 	    "directory '$path' does not exist\n" if ! -d $path;
 
-	my $server = &$get_active_server($scfg, 1);
+	my $server = get_active_server($scfg, 1);
 
 	glusterfs_mount($server, $volume, $path);
     }
@@ -347,7 +347,7 @@ sub deactivate_volume {
 sub check_connection {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    my $server = &$get_active_server($scfg);
+    my $server = get_active_server($scfg);
 
     return defined($server) ? 1 : 0;
 }
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 27/36] plugin: iscsi-direct: make helper subroutine `iscsi_ls` private

[Max Carrara]

in order to prevent it from being used in other places or third-party
plugins.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/ISCSIDirectPlugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
index eb329d4..c804ab4 100644
--- a/src/PVE/Storage/ISCSIDirectPlugin.pm
+++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
@@ -11,7 +11,7 @@ use PVE::JSONSchema qw(get_standard_option);
 
 use base qw(PVE::Storage::Plugin);
 
-sub iscsi_ls {
+my sub iscsi_ls {
     my ($scfg, $storeid) = @_;
 
     my $portal = $scfg->{portal};
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 28/36] plugin: iscsi: factor helper functions into common module

[Max Carrara]

Because the `iscsi_discovery` subroutine is used by `PVE::Storage`
directly, move it and all other helpers independent from the plugin
into the new `PVE::Storage::Common::ISCSI` module. As the name
suggests, this new module collects ISCSI-related functionalities and
utils.

Due to the `iscsi_discovery` sub being the only subroutine that was
actually used publicly, keep its original definition and let it wrap
its "new" version from the common module. Also add a deprecation
warning for any potential users.

The code of all moved subroutines stays the same, though the subs
themselves are now also documented and use prototypes.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage.pm              |   4 +-
 src/PVE/Storage/Common.pm       |   4 +
 src/PVE/Storage/Common/ISCSI.pm | 475 ++++++++++++++++++++++++++++++++
 src/PVE/Storage/Common/Makefile |   1 +
 src/PVE/Storage/ISCSIPlugin.pm  | 255 +----------------
 5 files changed, 498 insertions(+), 241 deletions(-)
 create mode 100644 src/PVE/Storage/Common/ISCSI.pm

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 57b2038..3865b44 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::Common::ISCSI;
+
 use PVE::Storage::Plugin;
 use PVE::Storage::DirPlugin;
 use PVE::Storage::LVMPlugin;
@@ -1457,7 +1459,7 @@ sub scan_iscsi {
 	die "unable to parse/resolve portal address '${portal_in}'\n";
     }
 
-    return PVE::Storage::ISCSIPlugin::iscsi_discovery([ $portal ]);
+    return PVE::Storage::Common::ISCSI::iscsi_discovery([ $portal ]);
 }
 
 sub storage_default_format {
diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
index 3cc3c37..8a52498 100644
--- a/src/PVE/Storage/Common.pm
+++ b/src/PVE/Storage/Common.pm
@@ -40,6 +40,10 @@ be grouped in a submodule can also be found here.
 
 =over
 
+=item C<PVE::Storage::Common::ISCSI>
+
+Subroutines that provide ISCSI-related functionalities.
+
 =item C<PVE::Storage::Common::LVM>
 
 Utilities concerned with LVM, such as manipulating logical volumes.
diff --git a/src/PVE/Storage/Common/ISCSI.pm b/src/PVE/Storage/Common/ISCSI.pm
new file mode 100644
index 0000000..2eb7f65
--- /dev/null
+++ b/src/PVE/Storage/Common/ISCSI.pm
@@ -0,0 +1,475 @@
+package PVE::Storage::Common::ISCSI;
+
+use strict;
+use warnings;
+
+use File::stat;
+use IO::Dir;
+use IO::File;
+
+use PVE::Network;
+use PVE::Tools qw(
+    $IPV4RE
+    $IPV6RE
+    dir_glob_foreach
+    dir_glob_regex
+    file_read_firstline
+    run_command
+);
+
+use parent qw(Exporter);
+
+our @EXPORT_OK = qw(
+    assert_iscsi_support
+    iscsi_device_list
+    iscsi_discovery
+    iscsi_login
+    iscsi_logout
+    iscsi_portals
+    iscsi_session_list
+    iscsi_session_rescan
+    iscsi_test_portal
+);
+
+=pod
+
+=head1 NAME
+
+PVE::Storage::Common::ISCSI - Provides helper subroutines that wrap commonly used ISCSI commands
+
+=head1 FUNCTIONS
+
+=cut
+
+my $ISCSIADM = '/usr/bin/iscsiadm';
+my $found_iscsi_adm_exe;
+
+=pod
+
+=head3 assert_iscsi_support
+
+    $is_supported = assert_iscsi_support($noerr)
+
+Asserts whether ISCSI operations are supported by the host, raising an exception
+if they are not.
+
+Optionally, C<$noerr> may be set to C<1> in order to return C<undef> instead
+of raising an exception.
+
+ISCSI operations are considered to be supported if C</usr/bin/iscsiadm> exists
+and is executable by the current user.
+
+=cut
+
+sub assert_iscsi_support : prototype(;$) {
+    my ($noerr) = @_;
+    return $found_iscsi_adm_exe if $found_iscsi_adm_exe; # assume it won't be removed if ever found
+
+    $found_iscsi_adm_exe = -x $ISCSIADM;
+
+    if (!$found_iscsi_adm_exe) {
+	die "error: no iscsi support - please install open-iscsi\n" if !$noerr;
+	warn "warning: no iscsi support - please install open-iscsi\n";
+    }
+    return $found_iscsi_adm_exe;
+}
+
+# Example: 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f
+my $ISCSI_TARGET_RE = qr/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/;
+
+=pod
+
+=head3 iscsi_session_list
+
+    $active_sessions = iscsi_session_list()
+
+Scans for active ISCSI sessions and returns a hash that maps each ISCSI target
+to a list of hashes describing the session.
+
+Asserts whether ISCSI is supported on the host beforehand.
+
+The returned hash has the following structure:
+
+    {
+	'iqn.1998-01.example.hostname.iscsi:name1' => [
+	    {
+		session_id => ...,
+		portal => ...,
+	    },
+	    ...
+	],
+	'iqn.1998-01.example.hostname.iscsi:name1001' => [
+	    {
+		session_id => ...,
+		portal => ...,
+	    },
+	    {
+		session_id => ...,
+		portal => ...,
+	    },
+	    ...
+	],
+	...
+    }
+
+=cut
+
+sub iscsi_session_list : prototype() {
+    assert_iscsi_support();
+
+    my $cmd = [$ISCSIADM, '--mode', 'session'];
+
+    my $res = {};
+    eval {
+	run_command($cmd, errmsg => 'iscsi session scan failed', outfunc => sub {
+	    my $line = shift;
+	    # example: tcp: [1] 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f (non-flash)
+	    if ($line =~ m/^tcp:\s+\[(\S+)\]\s+((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s+\S+?\s*$/) {
+		my ($session_id, $portal, $target) = ($1, $2, $3);
+		# there can be several sessions per target (multipath)
+		push @{$res->{$target}}, { session_id => $session_id, portal => $portal };
+	    }
+	});
+    };
+    if (my $err = $@) {
+	die $err if $err !~ m/: No active sessions.$/i;
+    }
+
+    return $res;
+}
+
+=pod
+
+=head3 iscsi_test_portal
+
+    $is_available = iscsi_test_portal($portal)
+
+Tests whether the given ISCSI C<$portal> can be reached by pinging it.
+
+=cut
+
+sub iscsi_test_portal : prototype($) {
+    my ($portal) = @_;
+
+    my ($server, $port) = PVE::Tools::parse_host_and_port($portal);
+    return 0 if !$server;
+    return PVE::Network::tcp_ping($server, $port || 3260, 2);
+}
+
+=pod
+
+=head3 iscsi_portals
+
+    $portals = iscsi_portals($target, $portal_in)
+
+Lists all available ISCSI portals whose target equals C<$target>.
+
+Should the lookup fail, a warning with the error message is emitted and a list
+only containing C<$portal_in> is returned as a fallback.
+
+=cut
+
+sub iscsi_portals : prototype($$) {
+    my ($target, $portal_in) = @_;
+
+    assert_iscsi_support();
+
+    my $res = [];
+    my $cmd = [$ISCSIADM, '--mode', 'node'];
+    eval {
+	run_command($cmd, outfunc => sub {
+	    my $line = shift;
+
+	    if ($line =~ $ISCSI_TARGET_RE) {
+		my ($portal, $portal_target) = ($1, $2);
+		if ($portal_target eq $target) {
+		    push @{$res}, $portal;
+		}
+	    }
+	});
+    };
+
+    if ($@) {
+	warn $@;
+	return [ $portal_in ];
+    }
+
+    return $res;
+}
+
+=pod
+
+=head3 iscsi_discovery
+
+    $discovered_targets = iscsi_discovery($portals)
+
+Scans each portal in C<$portals> for available ISCSI targets and returns a hash
+that maps each target to a list of portals.
+
+Asserts whether ISCSI is supported on the host beforehand.
+
+The returned hash has the following structure:
+
+    {
+	'iqn.1998-01.example.hostname.iscsi:name1' => [
+	    '172.16.64.177:3260',
+	    ...
+	],
+	'iqn.1998-01.example.hostname.iscsi:name1001' => [
+	    '172.16.64.178:3260',
+	    '172.16.64.179:3260',
+	    ...
+	],
+	...
+    }
+
+=cut
+
+sub iscsi_discovery : prototype($) {
+    my ($portals) = @_;
+
+    assert_iscsi_support();
+
+    my $res = {};
+    for my $portal ($portals->@*) {
+	next if !iscsi_test_portal($portal); # fixme: raise exception here?
+
+	my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
+	eval {
+	    run_command($cmd, outfunc => sub {
+		my $line = shift;
+
+		if ($line =~ $ISCSI_TARGET_RE) {
+		    my ($portal, $target) = ($1, $2);
+		    # one target can have more than one portal (multipath)
+		    # and sendtargets should return all of them in single call
+		    push @{$res->{$target}}, $portal;
+		}
+	    });
+	};
+
+	# In case of multipath we can stop after receiving targets from any available portal
+	last if scalar(keys %$res) > 0;
+    }
+
+    return $res;
+}
+
+=pod
+
+=head3 iscsi_login
+
+    iscsi_login($target, $portals)
+
+Logs in to the given ISCSI C<$target>. Will try to L<discover|/iscsi_discovery>
+any targets for all C<$portals> beforehand.
+
+Asserts whether ISCSI is supported on the host beforehand.
+
+=cut
+
+sub iscsi_login : prototype($$) {
+    my ($target, $portals) = @_;
+
+    assert_iscsi_support();
+
+    eval { iscsi_discovery($portals); };
+    warn $@ if $@;
+
+    run_command([$ISCSIADM, '--mode', 'node', '--targetname',  $target, '--login']);
+}
+
+=pod
+
+=head3 iscsi_logout
+
+    iscsi_logout($target)
+
+Logs out of the given ISCSI C<$target>.
+
+Asserts whether ISCSI is supported on the host beforehand.
+
+=cut
+
+sub iscsi_logout : prototype($) {
+    my ($target) = @_;
+
+    assert_iscsi_support();
+
+    run_command([$ISCSIADM, '--mode', 'node', '--targetname', $target, '--logout']);
+}
+
+=pod
+
+=head3 iscsi_session_rescan
+
+    iscsi_session_rescan($session_list)
+
+Rescans each ISCSI session in C<$session_list>.
+
+Asserts whether ISCSI is supported on the host beforehand.
+
+Frequent rescans are prevented by using a lock file located at
+C</var/run/pve-iscsi-rescan.lock>. The access time of the file is used to
+determine when the last rescan was performed.
+
+=cut
+
+my $rescan_filename = "/var/run/pve-iscsi-rescan.lock";
+
+sub iscsi_session_rescan : prototype($) {
+    my $session_list = shift;
+
+    assert_iscsi_support();
+
+    my $rstat = stat($rescan_filename);
+
+    if (!$rstat) {
+	if (my $fh = IO::File->new($rescan_filename, "a")) {
+	    utime undef, undef, $fh;
+	    close($fh);
+	}
+    } else {
+	my $atime = $rstat->atime;
+	my $tdiff = time() - $atime;
+	# avoid frequent rescans
+	return if !($tdiff < 0 || $tdiff > 10);
+	utime undef, undef, $rescan_filename;
+    }
+
+    foreach my $session (@$session_list) {
+	my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $session->{session_id}, '--rescan'];
+	eval { run_command($cmd, outfunc => sub {}); };
+	warn $@ if $@;
+    }
+}
+
+my sub load_stable_scsi_paths {
+
+    my $stable_paths = {};
+
+    my $stabledir = "/dev/disk/by-id";
+
+    if (my $dh = IO::Dir->new($stabledir)) {
+	foreach my $tmp (sort $dh->read) {
+           # exclude filenames with part in name (same disk but partitions)
+           # use only filenames with scsi(with multipath i have the same device
+	   # with dm-uuid-mpath , dm-name and scsi in name)
+           if($tmp !~ m/-part\d+$/ && ($tmp =~ m/^scsi-/ || $tmp =~ m/^dm-uuid-mpath-/)) {
+                 my $path = "$stabledir/$tmp";
+                 my $bdevdest = readlink($path);
+		 if ($bdevdest && $bdevdest =~ m|^../../([^/]+)|) {
+		     $stable_paths->{$1}=$tmp;
+		 }
+	   }
+       }
+       $dh->close;
+    }
+    return $stable_paths;
+}
+
+=pod
+
+=head3 iscsi_device_list
+
+    $device_list = iscsi_device_list()
+
+Reads and parses the metadata of all ISCSI devices connected to the host,
+returning a nested hash that maps ISCSI targets to the parsed device data.
+
+Devices that cannot be parsed are silently ignored.
+
+The returned hash has the following structure:
+
+    {
+	'iqn.2024-07.tld.example.hostname:lun01' => {
+	    '0.0.1.scsi-360000000000000000e00000000010001' => {
+		'channel' => 0,
+		'format' => 'raw',
+		'id' => 0,
+		'lun' => 1,
+		'size' => '17179869184',
+		'vmid' => 0
+	    },
+	    ...
+	},
+	'iqn.2024-07.tld.example.hostname.lun02' => {
+	    '0.0.1.scsi-360000000000000000e00000000020001' => {
+		'channel' => 0,
+		'format' => 'raw',
+		'id' => 0,
+		'lun' => 1,
+		'size' => '17179869184',
+		'vmid' => 0
+	    },
+	    ...
+	},
+	...
+    }
+
+=cut
+
+sub iscsi_device_list : prototype() {
+
+    my $res = {};
+
+    my $dirname = '/sys/class/iscsi_session';
+
+    my $stable_paths = load_stable_scsi_paths();
+
+    dir_glob_foreach($dirname, 'session(\d+)', sub {
+	my ($ent, $session) = @_;
+
+	my $target = file_read_firstline("$dirname/$ent/targetname");
+	return if !$target;
+
+	my (undef, $host) = dir_glob_regex("$dirname/$ent/device", 'target(\d+):.*');
+	return if !defined($host);
+
+	dir_glob_foreach("/sys/bus/scsi/devices", "$host:" . '(\d+):(\d+):(\d+)', sub {
+	    my ($tmp, $channel, $id, $lun) = @_;
+
+	    my $type = file_read_firstline("/sys/bus/scsi/devices/$tmp/type");
+	    return if !defined($type) || $type ne '0'; # list disks only
+
+	    my $bdev;
+	    if (-d "/sys/bus/scsi/devices/$tmp/block") { # newer kernels
+		(undef, $bdev) = dir_glob_regex("/sys/bus/scsi/devices/$tmp/block/", '([A-Za-z]\S*)');
+	    } else {
+		(undef, $bdev) = dir_glob_regex("/sys/bus/scsi/devices/$tmp", 'block:(\S+)');
+	    }
+	    return if !$bdev;
+
+	    #check multipath
+	    if (-d "/sys/block/$bdev/holders") {
+		my $multipathdev = dir_glob_regex("/sys/block/$bdev/holders", '[A-Za-z]\S*');
+		$bdev = $multipathdev if $multipathdev;
+	    }
+
+	    my $blockdev = $stable_paths->{$bdev};
+	    return if !$blockdev;
+
+	    my $size = file_read_firstline("/sys/block/$bdev/size");
+	    return if !$size;
+
+	    my $volid = "$channel.$id.$lun.$blockdev";
+
+	    $res->{$target}->{$volid} = {
+		'format' => 'raw',
+		'size' => int($size * 512),
+		'vmid' => 0, # not assigned to any vm
+		'channel' => int($channel),
+		'id' => int($id),
+		'lun' => int($lun),
+	    };
+
+	    #print "TEST: $target $session $host,$bus,$tg,$lun $blockdev\n";
+	});
+
+    });
+
+    return $res;
+}
+
+
+1;
diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
index 863f7c7..e15a47c 100644
--- a/src/PVE/Storage/Common/Makefile
+++ b/src/PVE/Storage/Common/Makefile
@@ -1,4 +1,5 @@
 SOURCES = \
+	  ISCSI.pm \
 	  LVM.pm \
 	  Path.pm \
 
diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 2bdd9a2..8a56cfe 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -8,254 +8,29 @@ use IO::Dir;
 use IO::File;
 
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::Storage::Common qw(get_deprecation_warning);
+use PVE::Storage::Common::ISCSI qw(
+    assert_iscsi_support
+    iscsi_device_list
+    iscsi_login
+    iscsi_logout
+    iscsi_portals
+    iscsi_session_list
+    iscsi_session_rescan
+    iscsi_test_portal
+);
 use PVE::Storage::Plugin;
-use PVE::Tools qw(run_command file_read_firstline trim dir_glob_regex dir_glob_foreach $IPV4RE $IPV6RE);
 
 use base qw(PVE::Storage::Plugin);
 
-# iscsi helper function
-
-my $ISCSIADM = '/usr/bin/iscsiadm';
-
-my $found_iscsi_adm_exe;
-my sub assert_iscsi_support {
-    my ($noerr) = @_;
-    return $found_iscsi_adm_exe if $found_iscsi_adm_exe; # assume it won't be removed if ever found
-
-    $found_iscsi_adm_exe = -x $ISCSIADM;
-
-    if (!$found_iscsi_adm_exe) {
-	die "error: no iscsi support - please install open-iscsi\n" if !$noerr;
-	warn "warning: no iscsi support - please install open-iscsi\n";
-    }
-    return $found_iscsi_adm_exe;
-}
-
-# Example: 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f
-my $ISCSI_TARGET_RE = qr/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/;
-
-sub iscsi_session_list {
-    assert_iscsi_support();
-
-    my $cmd = [$ISCSIADM, '--mode', 'session'];
-
-    my $res = {};
-    eval {
-	run_command($cmd, errmsg => 'iscsi session scan failed', outfunc => sub {
-	    my $line = shift;
-	    # example: tcp: [1] 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f (non-flash)
-	    if ($line =~ m/^tcp:\s+\[(\S+)\]\s+((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s+\S+?\s*$/) {
-		my ($session_id, $portal, $target) = ($1, $2, $3);
-		# there can be several sessions per target (multipath)
-		push @{$res->{$target}}, { session_id => $session_id, portal => $portal };
-	    }
-	});
-    };
-    if (my $err = $@) {
-	die $err if $err !~ m/: No active sessions.$/i;
-    }
-
-    return $res;
-}
-
-sub iscsi_test_portal {
-    my ($portal) = @_;
-
-    my ($server, $port) = PVE::Tools::parse_host_and_port($portal);
-    return 0 if !$server;
-    return PVE::Network::tcp_ping($server, $port || 3260, 2);
-}
-
-sub iscsi_portals {
-    my ($target, $portal_in) = @_;
-
-    assert_iscsi_support();
-
-    my $res = [];
-    my $cmd = [$ISCSIADM, '--mode', 'node'];
-    eval {
-	run_command($cmd, outfunc => sub {
-	    my $line = shift;
-
-	    if ($line =~ $ISCSI_TARGET_RE) {
-		my ($portal, $portal_target) = ($1, $2);
-		if ($portal_target eq $target) {
-		    push @{$res}, $portal;
-		}
-	    }
-	});
-    };
-
-    if ($@) {
-	warn $@;
-	return [ $portal_in ];
-    }
-
-    return $res;
-}
-
 sub iscsi_discovery {
     my ($portals) = @_;
 
-    assert_iscsi_support();
-
-    my $res = {};
-    for my $portal ($portals->@*) {
-	next if !iscsi_test_portal($portal); # fixme: raise exception here?
-
-	my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
-	eval {
-	    run_command($cmd, outfunc => sub {
-		my $line = shift;
-
-		if ($line =~ $ISCSI_TARGET_RE) {
-		    my ($portal, $target) = ($1, $2);
-		    # one target can have more than one portal (multipath)
-		    # and sendtargets should return all of them in single call
-		    push @{$res->{$target}}, $portal;
-		}
-	    });
-	};
-
-	# In case of multipath we can stop after receiving targets from any available portal
-	last if scalar(keys %$res) > 0;
-    }
-
-    return $res;
-}
-
-sub iscsi_login {
-    my ($target, $portals) = @_;
-
-    assert_iscsi_support();
-
-    eval { iscsi_discovery($portals); };
-    warn $@ if $@;
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::ISCSI::iscsi_discovery"
+    );
 
-    run_command([$ISCSIADM, '--mode', 'node', '--targetname',  $target, '--login']);
-}
-
-sub iscsi_logout {
-    my ($target) = @_;
-
-    assert_iscsi_support();
-
-    run_command([$ISCSIADM, '--mode', 'node', '--targetname', $target, '--logout']);
-}
-
-my $rescan_filename = "/var/run/pve-iscsi-rescan.lock";
-
-sub iscsi_session_rescan {
-    my $session_list = shift;
-
-    assert_iscsi_support();
-
-    my $rstat = stat($rescan_filename);
-
-    if (!$rstat) {
-	if (my $fh = IO::File->new($rescan_filename, "a")) {
-	    utime undef, undef, $fh;
-	    close($fh);
-	}
-    } else {
-	my $atime = $rstat->atime;
-	my $tdiff = time() - $atime;
-	# avoid frequent rescans
-	return if !($tdiff < 0 || $tdiff > 10);
-	utime undef, undef, $rescan_filename;
-    }
-
-    foreach my $session (@$session_list) {
-	my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $session->{session_id}, '--rescan'];
-	eval { run_command($cmd, outfunc => sub {}); };
-	warn $@ if $@;
-    }
-}
-
-sub load_stable_scsi_paths {
-
-    my $stable_paths = {};
-
-    my $stabledir = "/dev/disk/by-id";
-
-    if (my $dh = IO::Dir->new($stabledir)) {
-	foreach my $tmp (sort $dh->read) {
-           # exclude filenames with part in name (same disk but partitions)
-           # use only filenames with scsi(with multipath i have the same device
-	   # with dm-uuid-mpath , dm-name and scsi in name)
-           if($tmp !~ m/-part\d+$/ && ($tmp =~ m/^scsi-/ || $tmp =~ m/^dm-uuid-mpath-/)) {
-                 my $path = "$stabledir/$tmp";
-                 my $bdevdest = readlink($path);
-		 if ($bdevdest && $bdevdest =~ m|^../../([^/]+)|) {
-		     $stable_paths->{$1}=$tmp;
-		 }
-	   }
-       }
-       $dh->close;
-    }
-    return $stable_paths;
-}
-
-sub iscsi_device_list {
-
-    my $res = {};
-
-    my $dirname = '/sys/class/iscsi_session';
-
-    my $stable_paths = load_stable_scsi_paths();
-
-    dir_glob_foreach($dirname, 'session(\d+)', sub {
-	my ($ent, $session) = @_;
-
-	my $target = file_read_firstline("$dirname/$ent/targetname");
-	return if !$target;
-
-	my (undef, $host) = dir_glob_regex("$dirname/$ent/device", 'target(\d+):.*');
-	return if !defined($host);
-
-	dir_glob_foreach("/sys/bus/scsi/devices", "$host:" . '(\d+):(\d+):(\d+)', sub {
-	    my ($tmp, $channel, $id, $lun) = @_;
-
-	    my $type = file_read_firstline("/sys/bus/scsi/devices/$tmp/type");
-	    return if !defined($type) || $type ne '0'; # list disks only
-
-	    my $bdev;
-	    if (-d "/sys/bus/scsi/devices/$tmp/block") { # newer kernels
-		(undef, $bdev) = dir_glob_regex("/sys/bus/scsi/devices/$tmp/block/", '([A-Za-z]\S*)');
-	    } else {
-		(undef, $bdev) = dir_glob_regex("/sys/bus/scsi/devices/$tmp", 'block:(\S+)');
-	    }
-	    return if !$bdev;
-
-	    #check multipath
-	    if (-d "/sys/block/$bdev/holders") {
-		my $multipathdev = dir_glob_regex("/sys/block/$bdev/holders", '[A-Za-z]\S*');
-		$bdev = $multipathdev if $multipathdev;
-	    }
-
-	    my $blockdev = $stable_paths->{$bdev};
-	    return if !$blockdev;
-
-	    my $size = file_read_firstline("/sys/block/$bdev/size");
-	    return if !$size;
-
-	    my $volid = "$channel.$id.$lun.$blockdev";
-
-	    $res->{$target}->{$volid} = {
-		'format' => 'raw',
-		'size' => int($size * 512),
-		'vmid' => 0, # not assigned to any vm
-		'channel' => int($channel),
-		'id' => int($id),
-		'lun' => int($lun),
-	    };
-
-	    #print "TEST: $target $session $host,$bus,$tg,$lun $blockdev\n";
-	});
-
-    });
-
-    return $res;
+    return PVE::Storage::Common::ISCSI::iscsi_discovery($portals);
 }
 
 # Configuration
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 29/36] plugin: iscsi: make helper subroutine `iscsi_session` private

[Max Carrara]

Because this subroutine is not really a valuable helper for the common
ISCSI module, keep it inside the plugin and make it private instead.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/ISCSIPlugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 8a56cfe..2e55f23 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -167,7 +167,7 @@ sub list_images {
     return $res;
 }
 
-sub iscsi_session {
+my sub iscsi_session {
     my ($cache, $target) = @_;
     $cache->{iscsi_sessions} = iscsi_session_list() if !$cache->{iscsi_sessions};
     return $cache->{iscsi_sessions}->{$target};
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 30/36] plugin: lvm: update definition of subroutine `check_tags`

[Max Carrara]

So it's not just an anonymous sub assigned to a reference, but a
"proper" private subroutine instead. Also update its only call site
correspondingly.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index a9bc178..df041aa 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -307,7 +307,7 @@ sub free_image {
     return undef;
 }
 
-my $check_tags = sub {
+my sub check_tags {
     my ($tags) = @_;
 
     return defined($tags) && $tags =~ /(^|,)pve-vm-\d+(,|$)/;
@@ -331,7 +331,7 @@ sub list_images {
 
 	    my $info = $dat->{$volname};
 
-	    next if $scfg->{tagged_only} && !&$check_tags($info->{tags});
+	    next if $scfg->{tagged_only} && !check_tags($info->{tags});
 
 	    # Allow mirrored and RAID LVs
 	    next if $info->{lv_type} !~ m/^[-mMrR]$/;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 31/36] plugin: lvmthin: update definition of subroutine `activate_lv`

[Max Carrara]

.. so that it is not just an anonymous sub assigned to a reference,
but a "proper" private subroutine instead. Also update its call sites
correspondingly.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LvmThinPlugin.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 1fcafdd..c89b382 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -204,7 +204,7 @@ sub status {
     );
 }
 
-my $activate_lv = sub {
+my sub activate_lv {
     my ($vg, $lv, $cache) = @_;
 
     my $lvs = $cache->{lvs} ||= lvm_list_volumes();
@@ -225,7 +225,7 @@ sub activate_storage {
 
     $class->SUPER::activate_storage($storeid, $scfg, $cache);
 
-    $activate_lv->($scfg->{vgname}, $scfg->{thinpool}, $cache);
+    activate_lv($scfg->{vgname}, $scfg->{thinpool}, $cache);
 }
 
 sub activate_volume {
@@ -234,7 +234,7 @@ sub activate_volume {
     my $vg = $scfg->{vgname};
     my $lv = $snapname ? "snap_${volname}_$snapname" : $volname;
 
-    $activate_lv->($vg, $lv, $cache);
+    activate_lv($vg, $lv, $cache);
 }
 
 sub deactivate_volume {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 32/36] plugin: nfs: make helper subroutines private

[Max Carrara]

The `nfs_is_mounted` and `nfs_mount` subroutines are not part of any
(official) public interface / API, nor are they used anywhere in our
code, so make them private to prevent haphazard use.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/NFSPlugin.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
index 0d02b49..6dbf8b4 100644
--- a/src/PVE/Storage/NFSPlugin.pm
+++ b/src/PVE/Storage/NFSPlugin.pm
@@ -23,7 +23,7 @@ use base qw(PVE::Storage::Plugin);
 
 # NFS helper functions
 
-sub nfs_is_mounted {
+my sub nfs_is_mounted {
     my ($server, $export, $mountpoint, $mountdata) = @_;
 
     $server = "[$server]" if Net::IP::ip_is_ipv6($server);
@@ -38,7 +38,7 @@ sub nfs_is_mounted {
     return undef;
 }
 
-sub nfs_mount {
+my sub nfs_mount {
     my ($server, $export, $mountpoint, $options) = @_;
 
     $server = "[$server]" if Net::IP::ip_is_ipv6($server);
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 33/36] plugin: rbd: update private sub signatures and make helpers private

[Max Carrara]

Instead of assigning anonymous subs to a reference, make them "proper"
private subs instead. Thus, signatures like

  my $foo = sub { ... };

are changed to

  my sub foo { ... }

and their call sites are updated correspondingly.

The remaining helpers are also made private, because they're not used
anywhere else in our code.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/RBDPlugin.pm | 84 ++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 42 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index f45ad3f..919b9f9 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -20,13 +20,13 @@ use PVE::Tools qw(run_command trim file_read_firstline);
 
 use base qw(PVE::Storage::Plugin);
 
-my $get_parent_image_name = sub {
+my sub get_parent_image_name {
     my ($parent) = @_;
     return undef if !$parent;
     return $parent->{image} . "@" . $parent->{snapshot};
 };
 
-my $librados_connect = sub {
+my sub librados_connect {
     my ($scfg, $storeid, $options) = @_;
 
     $options->{timeout} = 60
@@ -55,7 +55,7 @@ my sub get_rbd_dev_path {
 	# NOTE: the config doesn't support this currently (but it could!), hack for qemu-server tests
 	$cluster_id = $scfg->{fsid};
     } elsif ($scfg->{monhost}) {
-	my $rados = $librados_connect->($scfg, $storeid);
+	my $rados = librados_connect($scfg, $storeid);
 	$cluster_id = $rados->mon_command({ prefix => 'fsid', format => 'json' })->{fsid};
     } else {
 	$cluster_id = cfs_read_file('ceph.conf')->{global}->{fsid};
@@ -82,7 +82,7 @@ my sub get_rbd_dev_path {
     return $pve_path;
 }
 
-my $build_cmd = sub {
+my sub build_cmd {
     my ($binary, $scfg, $storeid, $op, @options) = @_;
 
     my $cmd_option = PVE::CephConfig::ceph_connect_option($scfg, $storeid);
@@ -110,20 +110,20 @@ my $build_cmd = sub {
     return $cmd;
 };
 
-my $rbd_cmd = sub {
+my sub rbd_cmd {
     my ($scfg, $storeid, $op, @options) = @_;
 
-    return $build_cmd->('/usr/bin/rbd', $scfg, $storeid, $op, @options);
+    return build_cmd('/usr/bin/rbd', $scfg, $storeid, $op, @options);
 };
 
-my $rados_cmd = sub {
+my sub rados_cmd {
     my ($scfg, $storeid, $op, @options) = @_;
 
-    return $build_cmd->('/usr/bin/rados', $scfg, $storeid, $op, @options);
+    return build_cmd('/usr/bin/rados', $scfg, $storeid, $op, @options);
 };
 
 # needed for volumes created using ceph jewel (or higher)
-my $krbd_feature_update = sub {
+my sub krbd_feature_update {
     my ($scfg, $storeid, $name) = @_;
 
     my (@disable, @enable);
@@ -150,7 +150,7 @@ my $krbd_feature_update = sub {
 
     if ($to_disable) {
 	print "disable RBD image features this kernel RBD drivers is not compatible with: $to_disable\n";
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'feature', 'disable', $name, $to_disable);
+	my $cmd = rbd_cmd($scfg, $storeid, 'feature', 'disable', $name, $to_disable);
 	run_rbd_command(
 	    $cmd,
 	    errmsg => "could not disable krbd-incompatible image features '$to_disable' for rbd image: $name",
@@ -159,7 +159,7 @@ my $krbd_feature_update = sub {
     if ($to_enable) {
 	print "enable RBD image features this kernel RBD drivers supports: $to_enable\n";
 	eval {
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'feature', 'enable', $name, $to_enable);
+	    my $cmd = rbd_cmd($scfg, $storeid, 'feature', 'enable', $name, $to_enable);
 	    run_rbd_command(
 		$cmd,
 		errmsg => "could not enable krbd-compatible image features '$to_enable' for rbd image: $name",
@@ -169,7 +169,7 @@ my $krbd_feature_update = sub {
     }
 };
 
-sub run_rbd_command {
+my sub run_rbd_command {
     my ($cmd, %args) = @_;
 
     my $lasterr;
@@ -198,7 +198,7 @@ sub run_rbd_command {
     return undef;
 }
 
-sub rbd_ls {
+my sub rbd_ls {
     my ($scfg, $storeid) = @_;
 
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
@@ -207,7 +207,7 @@ sub rbd_ls {
     my $raw = '';
     my $parser = sub { $raw .= shift };
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'ls', '-l', '--format', 'json');
+    my $cmd = rbd_cmd($scfg, $storeid, 'ls', '-l', '--format', 'json');
     eval {
 	run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);
     };
@@ -237,7 +237,7 @@ sub rbd_ls {
 	$list->{$pool}->{$image} = {
 	    name => $image,
 	    size => $el->{size},
-	    parent => $get_parent_image_name->($el->{parent}),
+	    parent => get_parent_image_name($el->{parent}),
 	    vmid => $owner
 	};
     }
@@ -245,10 +245,10 @@ sub rbd_ls {
     return $list;
 }
 
-sub rbd_ls_snap {
+my sub rbd_ls_snap {
     my ($scfg, $storeid, $name) = @_;
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'ls', $name, '--format', 'json');
+    my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'ls', $name, '--format', 'json');
 
     my $raw = '';
     run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => sub { $raw .= shift; });
@@ -277,7 +277,7 @@ sub rbd_ls_snap {
     return $res;
 }
 
-sub rbd_volume_info {
+my sub rbd_volume_info {
     my ($scfg, $storeid, $volname, $snap) = @_;
 
     my $cmd = undef;
@@ -287,7 +287,7 @@ sub rbd_volume_info {
 	push @options, '--snap', $snap;
     }
 
-    $cmd = $rbd_cmd->($scfg, $storeid, @options);
+    $cmd = rbd_cmd($scfg, $storeid, @options);
 
     my $raw = '';
     my $parser = sub { $raw .= shift };
@@ -303,17 +303,17 @@ sub rbd_volume_info {
 	die "got unexpected data from rbd info: '$raw'\n";
     }
 
-    $volume->{parent} = $get_parent_image_name->($volume->{parent});
+    $volume->{parent} = get_parent_image_name($volume->{parent});
     $volume->{protected} = defined($volume->{protected}) && $volume->{protected} eq "true" ? 1 : undef;
 
     return $volume->@{qw(size parent format protected features)};
 }
 
-sub rbd_volume_du {
+my sub rbd_volume_du {
     my ($scfg, $storeid, $volname) = @_;
 
     my @options = ('du', $volname, '--format', 'json');
-    my $cmd = $rbd_cmd->($scfg, $storeid, @options);
+    my $cmd = rbd_cmd($scfg, $storeid, @options);
 
     my $raw = '';
     my $parser = sub { $raw .= shift };
@@ -484,7 +484,7 @@ sub path {
 sub find_free_diskname {
     my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'ls');
+    my $cmd = rbd_cmd($scfg, $storeid, 'ls');
 
     my $disk_list = [];
 
@@ -528,7 +528,7 @@ sub create_base {
 
     my $newvolname = $basename ? "$basename/$newname" : "$newname";
 
-    my $cmd = $rbd_cmd->(
+    my $cmd = rbd_cmd(
 	$scfg,
 	$storeid,
 	'rename',
@@ -547,7 +547,7 @@ sub create_base {
     my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $newname, $snap);
 
     if (!$protected){
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $newname, '--snap', $snap);
+	my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'protect', $newname, '--snap', $snap);
 	run_rbd_command($cmd, errmsg => "rbd protect $newname snap '$snap' error");
     }
 
@@ -575,7 +575,7 @@ sub clone_image {
 	my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $volname, $snapname);
 
 	if (!$protected) {
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $volname, '--snap', $snapname);
+	    my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'protect', $volname, '--snap', $snapname);
 	    run_rbd_command($cmd, errmsg => "rbd protect $volname snap $snapname error");
 	}
     }
@@ -589,7 +589,7 @@ sub clone_image {
     );
     push @options, ('--data-pool', $scfg->{'data-pool'}) if $scfg->{'data-pool'};
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'clone', @options, get_rbd_path($scfg, $name));
+    my $cmd = rbd_cmd($scfg, $storeid, 'clone', @options, get_rbd_path($scfg, $name));
     run_rbd_command($cmd, errmsg => "rbd clone '$basename' error");
 
     return $newvol;
@@ -610,7 +610,7 @@ sub alloc_image {
     );
     push @options, ('--data-pool', $scfg->{'data-pool'}) if $scfg->{'data-pool'};
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'create', @options, $name);
+    my $cmd = rbd_cmd($scfg, $storeid, 'create', @options, $name);
     run_rbd_command($cmd, errmsg => "rbd create '$name' error");
 
     return $name;
@@ -626,17 +626,17 @@ sub free_image {
     my $snaps = rbd_ls_snap($scfg, $storeid, $name);
     foreach my $snap (keys %$snaps) {
 	if ($snaps->{$snap}->{protected}) {
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $name, '--snap', $snap);
+	    my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'unprotect', $name, '--snap', $snap);
 	    run_rbd_command($cmd, errmsg => "rbd unprotect $name snap '$snap' error");
 	}
     }
 
     $class->deactivate_volume($storeid, $scfg, $volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'purge',  $name);
+    my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'purge',  $name);
     run_rbd_command($cmd, errmsg => "rbd snap purge '$name' error");
 
-    $cmd = $rbd_cmd->($scfg, $storeid, 'rm', $name);
+    $cmd = rbd_cmd($scfg, $storeid, 'rm', $name);
     run_rbd_command($cmd, errmsg => "rbd rm '$name' error");
 
     return undef;
@@ -679,7 +679,7 @@ sub list_images {
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
-    my $rados = $librados_connect->($scfg, $storeid);
+    my $rados = librados_connect($scfg, $storeid);
     my $df = $rados->mon_command({ prefix => 'df', format => 'json' });
 
     my $pool = $scfg->{'data-pool'} // $scfg->{pool} // 'rbd';
@@ -724,9 +724,9 @@ sub map_volume {
     return $kerneldev if -b $kerneldev; # already mapped
 
     # features can only be enabled/disabled for image, not for snapshot!
-    $krbd_feature_update->($scfg, $storeid, $img_name);
+    krbd_feature_update($scfg, $storeid, $img_name);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name);
+    my $cmd = rbd_cmd($scfg, $storeid, 'map', $name);
     run_rbd_command($cmd, errmsg => "can't map rbd volume $name");
 
     return $kerneldev;
@@ -741,7 +741,7 @@ sub unmap_volume {
     my $kerneldev = get_rbd_dev_path($scfg, $storeid, $name);
 
     if (-b $kerneldev) {
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'unmap', $kerneldev);
+	my $cmd = rbd_cmd($scfg, $storeid, 'unmap', $kerneldev);
 	run_rbd_command($cmd, errmsg => "can't unmap rbd device $kerneldev");
     }
 
@@ -780,7 +780,7 @@ sub volume_resize {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', int(ceil($size/1024/1024)), $name);
+    my $cmd = rbd_cmd($scfg, $storeid, 'resize', '--size', int(ceil($size/1024/1024)), $name);
     run_rbd_command($cmd, errmsg => "rbd resize '$volname' error");
     return undef;
 }
@@ -790,7 +790,7 @@ sub volume_snapshot {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'create', '--snap', $snap, $name);
+    my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'create', '--snap', $snap, $name);
     run_rbd_command($cmd, errmsg => "rbd snapshot '$volname' error");
     return undef;
 }
@@ -800,7 +800,7 @@ sub volume_snapshot_rollback {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'rollback', '--snap', $snap, $name);
+    my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'rollback', '--snap', $snap, $name);
     run_rbd_command($cmd, errmsg => "rbd snapshot $volname to '$snap' error");
 }
 
@@ -813,11 +813,11 @@ sub volume_snapshot_delete {
 
     my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $name, $snap);
     if ($protected){
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $name, '--snap', $snap);
+	my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'unprotect', $name, '--snap', $snap);
 	run_rbd_command($cmd, errmsg => "rbd unprotect $name snap '$snap' error");
     }
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'rm', '--snap', $snap, $name);
+    my $cmd = rbd_cmd($scfg, $storeid, 'snap', 'rm', '--snap', $snap, $name);
 
     run_rbd_command($cmd, errmsg => "rbd snapshot '$volname' error");
 
@@ -869,12 +869,12 @@ sub rename_volume {
 	if !$target_volname;
 
     eval {
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $target_volname);
+	my $cmd = rbd_cmd($scfg, $storeid, 'info', $target_volname);
 	run_rbd_command($cmd, errmsg => "exist check",  quiet => 1);
     };
     die "target volume '${target_volname}' already exists\n" if !$@;
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'rename', $source_image, $target_volname);
+    my $cmd = rbd_cmd($scfg, $storeid, 'rename', $source_image, $target_volname);
 
     run_rbd_command(
 	$cmd,
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 34/36] common: zfs: introduce module for common ZFS helpers

[Max Carrara]

Like the LVM and ISCSI modules, this module is meant to provide
commonly used ZFS helpers, so that ZFS-related functionality may be
shared more easily among plugins. Moreover, this module also ought to
help to reduce the inter-dependency of our own ZFS plugins, while
simultaneously making ZFS utilities availble to third-party plugin
authors.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common.pm       |  4 ++++
 src/PVE/Storage/Common/Makefile |  1 +
 src/PVE/Storage/Common/ZFS.pm   | 21 +++++++++++++++++++++
 3 files changed, 26 insertions(+)
 create mode 100644 src/PVE/Storage/Common/ZFS.pm

diff --git a/src/PVE/Storage/Common.pm b/src/PVE/Storage/Common.pm
index 8a52498..f0d5ddf 100644
--- a/src/PVE/Storage/Common.pm
+++ b/src/PVE/Storage/Common.pm
@@ -52,6 +52,10 @@ Utilities concerned with LVM, such as manipulating logical volumes.
 
 Utilities concerned with working with paths.
 
+=item C<PVE::Storage::Common::ZFS>
+
+Shared ZFS utilities and command line wrappers.
+
 =back
 
 =head1 FUNCTIONS
diff --git a/src/PVE/Storage/Common/Makefile b/src/PVE/Storage/Common/Makefile
index e15a47c..d77d67e 100644
--- a/src/PVE/Storage/Common/Makefile
+++ b/src/PVE/Storage/Common/Makefile
@@ -2,6 +2,7 @@ SOURCES = \
 	  ISCSI.pm \
 	  LVM.pm \
 	  Path.pm \
+	  ZFS.pm \
 
 
 .PHONY: install
diff --git a/src/PVE/Storage/Common/ZFS.pm b/src/PVE/Storage/Common/ZFS.pm
new file mode 100644
index 0000000..8b0969c
--- /dev/null
+++ b/src/PVE/Storage/Common/ZFS.pm
@@ -0,0 +1,21 @@
+package PVE::Storage::Common::ZFS;
+
+use strict;
+use warnings;
+
+use parent qw(Exporter);
+
+our @EXPORT_OK = qw();
+
+=pod
+
+=head1 NAME
+
+PVE::Storage::Common::ZFS - Shared ZFS utilities and command line wrappers
+
+=head1 FUNCTIONS
+
+=cut
+
+
+1;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 35/36] plugin: zfspool: move helper `zfs_parse_zvol_list` to common module

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common/ZFS.pm    | 93 +++++++++++++++++++++++++++++++-
 src/PVE/Storage/ZFSPoolPlugin.pm | 45 +++-------------
 2 files changed, 99 insertions(+), 39 deletions(-)

diff --git a/src/PVE/Storage/Common/ZFS.pm b/src/PVE/Storage/Common/ZFS.pm
index 8b0969c..21b28d9 100644
--- a/src/PVE/Storage/Common/ZFS.pm
+++ b/src/PVE/Storage/Common/ZFS.pm
@@ -5,7 +5,9 @@ use warnings;
 
 use parent qw(Exporter);
 
-our @EXPORT_OK = qw();
+our @EXPORT_OK = qw(
+    zfs_parse_zvol_list
+);
 
 =pod
 
@@ -17,5 +19,94 @@ PVE::Storage::Common::ZFS - Shared ZFS utilities and command line wrappers
 
 =cut
 
+=pod
+
+=head3 zfs_parse_zvol_list
+
+    $zvol_list = zfs_parse_zvol_list($text, $pool)
+
+Parses ZVOLs from a C<zfs list> command output for a given C<$pool> and returns
+them as a list of hashes.
+
+C<$text> must contain the output of the following command, where C<E<lt>POOLE<gt>>
+is expected to be the same as the provided C<$pool>:
+
+    zfs list -o name,volsize,origin,type,refquota -t volume,filesystem -d1 -Hp <POOL>
+
+C<$pool> should refer to the actual pool / dataset that contains ZVOLs. Usually
+this is C<E<lt>POOLNAMEE<gt>/vm-store>.
+
+The returned list has the following structure:
+
+    (
+	{
+	    name => "pool/vm-store/vm-9000-disk-0",
+	    size => 68719476736,  # size in bytes
+	    origin => "...",      # optional
+	    format => "raw",
+	    vmid => 9000,
+	},
+	{
+	    name => "pool/vm-store/vm-9000-disk-1",
+	    size => 68719476736,  # size in bytes
+	    origin => "...",      # optional
+	    format => "raw",
+	    vmid => 9000,
+	},
+	{
+	    name => "pool/vm-store/vm-9000-disk-2",
+	    size => 137438953472,  # size in bytes
+	    origin => "...",       # optional
+	    format => "raw",
+	    vmid => 9000,
+	},
+	...
+    )
+
+=cut
+
+sub zfs_parse_zvol_list : prototype($$) {
+    my ($text, $pool) = @_;
+
+    my $list = ();
+
+    return $list if !$text;
+
+    my @lines = split /\n/, $text;
+    foreach my $line (@lines) {
+	my ($dataset, $size, $origin, $type, $refquota) = split(/\s+/, $line);
+	next if !($type eq 'volume' || $type eq 'filesystem');
+
+	my $zvol = {};
+	my @parts = split /\//, $dataset;
+	next if scalar(@parts) < 2; # we need pool/name
+	my $name = pop @parts;
+	my $parsed_pool = join('/', @parts);
+	next if $parsed_pool ne $pool;
+
+	next unless $name =~ m!^(vm|base|subvol|basevol)-(\d+)-(\S+)$!;
+	$zvol->{owner} = $2;
+
+	$zvol->{name} = $name;
+	if ($type eq 'filesystem') {
+	    if ($refquota eq 'none') {
+		$zvol->{size} = 0;
+	    } else {
+		$zvol->{size} = $refquota + 0;
+	    }
+	    $zvol->{format} = 'subvol';
+	} else {
+	    $zvol->{size} = $size + 0;
+	    $zvol->{format} = 'raw';
+	}
+	if ($origin !~ /^-$/) {
+	    $zvol->{origin} = $origin;
+	}
+	push @$list, $zvol;
+    }
+
+    return $list;
+}
+
 
 1;
diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index 3669fe1..fdfedca 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -9,6 +9,8 @@ use POSIX;
 
 use PVE::ProcFSTools;
 use PVE::RPCEnvironment;
+use PVE::Storage::Common qw(get_deprecation_warning);
+use PVE::Storage::Common::ZFS;
 use PVE::Storage::Plugin;
 use PVE::Tools qw(run_command);
 
@@ -60,44 +62,11 @@ sub options {
 sub zfs_parse_zvol_list {
     my ($text, $pool) = @_;
 
-    my $list = ();
-
-    return $list if !$text;
-
-    my @lines = split /\n/, $text;
-    foreach my $line (@lines) {
-	my ($dataset, $size, $origin, $type, $refquota) = split(/\s+/, $line);
-	next if !($type eq 'volume' || $type eq 'filesystem');
-
-	my $zvol = {};
-	my @parts = split /\//, $dataset;
-	next if scalar(@parts) < 2; # we need pool/name
-	my $name = pop @parts;
-	my $parsed_pool = join('/', @parts);
-	next if $parsed_pool ne $pool;
-
-	next unless $name =~ m!^(vm|base|subvol|basevol)-(\d+)-(\S+)$!;
-	$zvol->{owner} = $2;
-
-	$zvol->{name} = $name;
-	if ($type eq 'filesystem') {
-	    if ($refquota eq 'none') {
-		$zvol->{size} = 0;
-	    } else {
-		$zvol->{size} = $refquota + 0;
-	    }
-	    $zvol->{format} = 'subvol';
-	} else {
-	    $zvol->{size} = $size + 0;
-	    $zvol->{format} = 'raw';
-	}
-	if ($origin !~ /^-$/) {
-	    $zvol->{origin} = $origin;
-	}
-	push @$list, $zvol;
-    }
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::ZFS::zfs_parse_zvol_list"
+    );
 
-    return $list;
+    return PVE::Storage::Common::ZFS::zfs_parse_zvol_list($text, $pool);
 }
 
 sub parse_volname {
@@ -383,7 +352,7 @@ sub zfs_list_zvol {
     );
     # It's still required to have zfs_parse_zvol_list filter by pool, because -d1 lists
     # $scfg->{pool} too and while unlikely, it could be named to be mistaken for a volume.
-    my $zvols = zfs_parse_zvol_list($text, $scfg->{pool});
+    my $zvols = PVE::Storage::Common::ZFS::zfs_parse_zvol_list($text, $scfg->{pool});
     return {} if !$zvols;
 
     my $list = {};
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [RFC pve-storage 36/36] plugin: zfspool: refactor method `zfs_request` into helper subroutine

[Max Carrara]

In order to separate the invocation of ZFS CLI utlis from the plugin,
the `zfs_request` method is refactored into a helper subroutine and
placed in the common ZFS module.

The signature is changed, removing the `$class` parameter. The body
remains the same, so no behaviour is actually altered.

The new helper sub is also documented and given a prototype.

The original method is changed to wrap the new helper and also emit a
deprecation warning when used.

The sites where the original method is called are updated to use the
helper subroutine instead.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common/ZFS.pm    |  84 +++++++++++++++++++++++
 src/PVE/Storage/ZFSPoolPlugin.pm | 112 ++++++++++++++++++-------------
 2 files changed, 150 insertions(+), 46 deletions(-)

diff --git a/src/PVE/Storage/Common/ZFS.pm b/src/PVE/Storage/Common/ZFS.pm
index 21b28d9..327a592 100644
--- a/src/PVE/Storage/Common/ZFS.pm
+++ b/src/PVE/Storage/Common/ZFS.pm
@@ -3,10 +3,16 @@ package PVE::Storage::Common::ZFS;
 use strict;
 use warnings;
 
+use PVE::RPCEnvironment;
+use PVE::Tools qw(
+    run_command
+);
+
 use parent qw(Exporter);
 
 our @EXPORT_OK = qw(
     zfs_parse_zvol_list
+    zfs_request
 );
 
 =pod
@@ -21,6 +27,84 @@ PVE::Storage::Common::ZFS - Shared ZFS utilities and command line wrappers
 
 =pod
 
+=head3 zfs_request
+
+    $output = zfs_request($scfg, $timeout, $method, @params)
+
+Wrapper that runs a ZFS command and returns its output.
+
+This subroutine has some special handling depending on which arguments are
+passed. See the description of the individual parameters below.
+
+=over
+
+=item C<$scfg>
+
+A section config hash. Unused and kept for backward compatibility.
+
+=item C<$timeout>
+
+Optional. The number of seconds to elapse before the wrapped command times out.
+
+If not provided, the invocation will time out after a default of C<10> seconds.
+
+If C<"zpool_import"> is passed as C<$method>, the timeout instead has a minimum
+of C<15> seconds and will automatically be increased if it is below.
+
+B<NOTE>: Should this subroutine be invoked in the context of an I<RPC> or
+I<REST> worker, the above is disregarded and the timeout has a minimum of
+B<five minutes>, automatically increasing it if it is below. Should no timeout
+be provided in this case, the default is B<one hour> instead.
+
+=item C<$method>
+
+The subcommand of the C<zfs> CLI to run. This can be something like C<"get">
+(C<zfs get>), C<"list"> (C<zfs list>), etc.
+
+There are two exceptions, however: If C<$method> is C<"zpool_list"> or
+C<"zpool_import">, C<zpool list> or C<zpool import> will respectively be called
+instead.
+
+=item C<@params>
+
+Optional. Further parameters to pass along to the C<zfs> or C<zpool> CLI.
+
+=back
+
+=cut
+
+sub zfs_request : prototype($$$;@) {
+    my ($scfg, $timeout, $method, @params) = @_;
+
+    my $cmd = [];
+
+    if ($method eq 'zpool_list') {
+	push @$cmd, 'zpool', 'list';
+    } elsif ($method eq 'zpool_import') {
+	push @$cmd, 'zpool', 'import';
+	$timeout = 15 if !$timeout || $timeout < 15;
+    } else {
+	push @$cmd, 'zfs', $method;
+    }
+    push @$cmd, @params;
+
+    my $msg = '';
+    my $output = sub { $msg .= "$_[0]\n" };
+
+    if (PVE::RPCEnvironment->is_worker()) {
+	$timeout = 60*60 if !$timeout;
+	$timeout = 60*5 if $timeout < 60*5;
+    } else {
+	$timeout = 10 if !$timeout;
+    }
+
+    run_command($cmd, errmsg => "zfs error", outfunc => $output, timeout => $timeout);
+
+    return $msg;
+}
+
+=pod
+
 =head3 zfs_parse_zvol_list
 
     $zvol_list = zfs_parse_zvol_list($text, $pool)
diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index fdfedca..c666166 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -132,31 +132,13 @@ sub path {
 sub zfs_request {
     my ($class, $scfg, $timeout, $method, @params) = @_;
 
-    my $cmd = [];
-
-    if ($method eq 'zpool_list') {
-	push @$cmd, 'zpool', 'list';
-    } elsif ($method eq 'zpool_import') {
-	push @$cmd, 'zpool', 'import';
-	$timeout = 15 if !$timeout || $timeout < 15;
-    } else {
-	push @$cmd, 'zfs', $method;
-    }
-    push @$cmd, @params;
-
-    my $msg = '';
-    my $output = sub { $msg .= "$_[0]\n" };
-
-    if (PVE::RPCEnvironment->is_worker()) {
-	$timeout = 60*60 if !$timeout;
-	$timeout = 60*5 if $timeout < 60*5;
-    } else {
-	$timeout = 10 if !$timeout;
-    }
-
-    run_command($cmd, errmsg => "zfs error", outfunc => $output, timeout => $timeout);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::ZFS::zfs_request"
+    );
 
-    return $msg;
+    return PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, $timeout, $method, @params
+    );
 }
 
 sub zfs_wait_for_zvol_link {
@@ -254,8 +236,9 @@ sub list_images {
 sub zfs_get_properties {
     my ($class, $scfg, $properties, $dataset, $timeout) = @_;
 
-    my $result = $class->zfs_request($scfg, $timeout, 'get', '-o', 'value',
-				     '-Hp', $properties, $dataset);
+    my $result = PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, $timeout, 'get', '-o', 'value', '-Hp', $properties, $dataset
+    );
     my @values = split /\n/, $result;
     return wantarray ? @values : $values[0];
 }
@@ -295,7 +278,7 @@ sub zfs_create_zvol {
 
     push @$cmd, '-V', "${size}k", "$scfg->{pool}/$zvol";
 
-    $class->zfs_request($scfg, undef, @$cmd);
+    PVE::Storage::Common::ZFS::zfs_request($scfg, undef, @$cmd);
 }
 
 sub zfs_create_subvol {
@@ -307,7 +290,7 @@ sub zfs_create_subvol {
     my $cmd = ['create', '-o', 'acltype=posixacl', '-o', 'xattr=sa',
 	       '-o', "refquota=${quota}", $dataset];
 
-    $class->zfs_request($scfg, undef, @$cmd);
+    PVE::Storage::Common::ZFS::zfs_request($scfg, undef, @$cmd);
 }
 
 sub zfs_delete_zvol {
@@ -317,7 +300,9 @@ sub zfs_delete_zvol {
 
     for (my $i = 0; $i < 6; $i++) {
 
-	eval { $class->zfs_request($scfg, undef, 'destroy', '-r', "$scfg->{pool}/$zvol"); };
+	eval { PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'destroy', '-r', "$scfg->{pool}/$zvol");
+	};
 	if ($err = $@) {
 	    if ($err =~ m/^zfs error:(.*): dataset is busy.*/) {
 		sleep(1);
@@ -338,7 +323,7 @@ sub zfs_delete_zvol {
 sub zfs_list_zvol {
     my ($class, $scfg) = @_;
 
-    my $text = $class->zfs_request(
+    my $text = PVE::Storage::Common::ZFS::zfs_request(
 	$scfg,
 	10,
 	'list',
@@ -383,7 +368,7 @@ sub zfs_get_sorted_snapshot_list {
     my $vname = ($class->parse_volname($volname))[1];
     push @params, "$scfg->{pool}\/$vname";
 
-    my $text = $class->zfs_request($scfg, undef, 'list', @params);
+    my $text = PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'list', @params);
     my @snapshots = split(/\n/, $text);
 
     my $snap_names = [];
@@ -435,7 +420,9 @@ sub volume_snapshot {
 
     my $vname = ($class->parse_volname($volname))[1];
 
-    $class->zfs_request($scfg, undef, 'snapshot', "$scfg->{pool}/$vname\@$snap");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'snapshot', "$scfg->{pool}/$vname\@$snap"
+    );
 }
 
 sub volume_snapshot_delete {
@@ -444,7 +431,9 @@ sub volume_snapshot_delete {
     my $vname = ($class->parse_volname($volname))[1];
 
     $class->deactivate_volume($storeid, $scfg, $vname, $snap, {});
-    $class->zfs_request($scfg, undef, 'destroy', "$scfg->{pool}/$vname\@$snap");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'destroy', "$scfg->{pool}/$vname\@$snap"
+    );
 }
 
 sub volume_snapshot_rollback {
@@ -452,13 +441,19 @@ sub volume_snapshot_rollback {
 
     my (undef, $vname, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
 
-    my $msg = $class->zfs_request($scfg, undef, 'rollback', "$scfg->{pool}/$vname\@$snap");
+    my $msg = PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'rollback', "$scfg->{pool}/$vname\@$snap"
+    );
 
     # we have to unmount rollbacked subvols, to invalidate wrong kernel
     # caches, they get mounted in activate volume again
     # see zfs bug #10931 https://github.com/openzfs/zfs/issues/10931
     if ($format eq 'subvol') {
-	eval { $class->zfs_request($scfg, undef, 'unmount', "$scfg->{pool}/$vname"); };
+	eval {
+	    PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'unmount', "$scfg->{pool}/$vname"
+	    );
+	};
 	if (my $err = $@) {
 	    die $err if $err !~ m/not currently mounted$/;
 	}
@@ -503,7 +498,7 @@ sub volume_snapshot_info {
     my $vname = ($class->parse_volname($volname))[1];
     push @params, "$scfg->{pool}\/$vname";
 
-    my $text = $class->zfs_request($scfg, undef, 'list', @params);
+    my $text = PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'list', @params);
     my @lines = split(/\n/, $text);
 
     my $info = {};
@@ -545,7 +540,11 @@ sub activate_storage {
 
     my $pool_imported = sub {
 	my @param = ('-o', 'name', '-H', $pool);
-	my $res = eval { $class->zfs_request($scfg, undef, 'zpool_list', @param) };
+	my $res = eval {
+	    PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'zpool_list', @param
+	    )
+	};
 	warn "$@\n" if $@;
 
 	return defined($res) && $res =~ m/$pool/;
@@ -554,13 +553,13 @@ sub activate_storage {
     if (!$pool_imported->()) {
 	# import can only be done if not yet imported!
 	my @param = ('-d', '/dev/disk/by-id/', '-o', 'cachefile=none', $pool);
-	eval { $class->zfs_request($scfg, undef, 'zpool_import', @param) };
+	eval { PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'zpool_import', @param) };
 	if (my $err = $@) {
 	    # just could've raced with another import, so recheck if it is imported
 	    die "could not activate storage '$storeid', $err\n" if !$pool_imported->();
 	}
     }
-    eval { $class->zfs_request($scfg, undef, 'mount', '-a') };
+    eval { PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'mount', '-a') };
     die "could not activate storage '$storeid', $@\n" if $@;
     return 1;
 }
@@ -582,7 +581,9 @@ sub activate_volume {
     } elsif ($format eq 'subvol') {
 	my $mounted = $class->zfs_get_properties($scfg, 'mounted', "$scfg->{pool}/$dataset");
 	if ($mounted !~ m/^yes$/) {
-	    $class->zfs_request($scfg, undef, 'mount', "$scfg->{pool}/$dataset");
+	    PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'mount', "$scfg->{pool}/$dataset"
+	    );
 	}
     }
 
@@ -607,11 +608,24 @@ sub clone_image {
     my $name = $class->find_free_diskname($storeid, $scfg, $vmid, $format);
 
     if ($format eq 'subvol') {
-	my $size = $class->zfs_request($scfg, undef, 'list', '-Hp', '-o', 'refquota', "$scfg->{pool}/$basename");
+	my $size = PVE::Storage::Common::ZFS::zfs_request(
+	    $scfg, undef, 'list', '-Hp', '-o', 'refquota', "$scfg->{pool}/$basename"
+	);
+
 	chomp($size);
-	$class->zfs_request($scfg, undef, 'clone', "$scfg->{pool}/$basename\@$snap", "$scfg->{pool}/$name", '-o', "refquota=$size");
+
+	PVE::Storage::Common::ZFS::zfs_request(
+	    $scfg,
+	    undef,
+	    'clone',
+	    "$scfg->{pool}/$basename\@$snap",
+	    "$scfg->{pool}/$name",
+	    '-o', "refquota=$size"
+	);
     } else {
-	$class->zfs_request($scfg, undef, 'clone', "$scfg->{pool}/$basename\@$snap", "$scfg->{pool}/$name");
+	PVE::Storage::Common::ZFS::zfs_request(
+	    $scfg, undef, 'clone', "$scfg->{pool}/$basename\@$snap", "$scfg->{pool}/$name"
+	);
     }
 
     return "$basename/$name";
@@ -635,7 +649,9 @@ sub create_base {
     }
     my $newvolname = $basename ? "$basename/$newname" : "$newname";
 
-    $class->zfs_request($scfg, undef, 'rename', "$scfg->{pool}/$name", "$scfg->{pool}/$newname");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'rename', "$scfg->{pool}/$name", "$scfg->{pool}/$newname"
+    );
 
     my $running  = undef; #fixme : is create_base always offline ?
 
@@ -660,7 +676,9 @@ sub volume_resize {
 	$new_size = $new_size + $padding;
     }
 
-    $class->zfs_request($scfg, undef, 'set', "$attr=${new_size}k", "$scfg->{pool}/$vname");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'set', "$attr=${new_size}k", "$scfg->{pool}/$vname"
+    );
 
     return $new_size;
 }
@@ -811,7 +829,9 @@ sub rename_volume {
 				  noerr => 1, quiet => 1);
     die "target volume '${target_volname}' already exists\n" if $exists;
 
-    $class->zfs_request($scfg, 5, 'rename', ${source_zfspath}, ${target_zfspath});
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, 5, 'rename', ${source_zfspath}, ${target_zfspath}
+    );
 
     $base_name = $base_name ? "${base_name}/" : '';
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments

[Thomas Lamprecht]

Am 17/07/2024 um 11:39 schrieb Max Carrara:
> These have been around since 2012 - suffice to say they're not needed
> anymore.

That's really not a good argument though? Just because nobody checked
those closely for a long time it does not mean that they became
magically irrelevant.

Look, it can be (and probably _is_) fine to remove them, but stating
that this is fine just because they were not touched since a while is a
rather dangerous tactic. Someone had some thoughts when adding this,
they might be still relevant or not, but it's definitively *not*
"suffice to say" that they aren't just because they have been around
since 2012, (iSCSI) portals and local storage still exist and are not
working really different compared to 12 years ago.

The node restriction FIXME comment can e.g. be removed as we delete any
such restriction in "parse_config", mentioning that as a reason would
make doing so fine, saying "it's old and unchanged" doesn't.

The storage portal one could be argued with not being defined elsewhere
and all use cases being covered by pve-storage-portal-dns, so removing
it won't hurt, especially as we can always recover it from history.

I think your intention quite surely matched those and meant well, but
removing something just because it's old is on its own IMO a bit of a
red flag, so one should get too used to that argumentation style even
if it's for removing comments, or other stuff that won't change semantics.

Anyhow, do not let this demotivate you from your clean-up efforts, they
are still quite appreciated.
While removing dead code is good, the argumentation behind should be
sound, and I only write this long tirade (sorry) as we got bitten by
some innocent looking changes stemming from a similar argumentation in
the past.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments

[Max Carrara]

On Wed Jul 17, 2024 at 6:02 PM CEST, Thomas Lamprecht wrote:
> Am 17/07/2024 um 11:39 schrieb Max Carrara:
> > These have been around since 2012 - suffice to say they're not needed
> > anymore.
>
> That's really not a good argument though? Just because nobody checked
> those closely for a long time it does not mean that they became
> magically irrelevant.
>
> Look, it can be (and probably _is_) fine to remove them, but stating
> that this is fine just because they were not touched since a while is a
> rather dangerous tactic. Someone had some thoughts when adding this,
> they might be still relevant or not, but it's definitively *not*
> "suffice to say" that they aren't just because they have been around
> since 2012, (iSCSI) portals and local storage still exist and are not
> working really different compared to 12 years ago.
>
> The node restriction FIXME comment can e.g. be removed as we delete any
> such restriction in "parse_config", mentioning that as a reason would
> make doing so fine, saying "it's old and unchanged" doesn't.
>
> The storage portal one could be argued with not being defined elsewhere
> and all use cases being covered by pve-storage-portal-dns, so removing
> it won't hurt, especially as we can always recover it from history.
>
> I think your intention quite surely matched those and meant well, but
> removing something just because it's old is on its own IMO a bit of a
> red flag, so one should get too used to that argumentation style even
> if it's for removing comments, or other stuff that won't change semantics.

I completely agree with you, I probably should've stated a better reason
there. IIRC I removed those two things for a valid reason, but because
the commit was made a while ago, I'm not actually sure anymore what they
were exactly. I guess this proves your point. ;)

In a future RFC / Series, this will definitely be updated. Thanks for
pointing that out.

>
> Anyhow, do not let this demotivate you from your clean-up efforts, they
> are still quite appreciated.
> While removing dead code is good, the argumentation behind should be
> sound, and I only write this long tirade (sorry) as we got bitten by
> some innocent looking changes stemming from a similar argumentation in
> the past.

No worries, no offense taken here - I really appreciate comment.
Sometimes these things do need to be pointed out, because e.g. for me
personally it just wasn't on my radar that a commit like this could
become a tough to debug issue in case things go south. That's probably
because I've never had to deal with debugging such a thing myself.

So again, no worries, I appreciate it!



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel