[PATCH storage 0/9] Fix ZFS-over-iSCSI plugin w/ LIO Provider Issues

[PATCH storage 0/9] Fix ZFS-over-iSCSI plugin w/ LIO Provider Issues

[Max R. Carrara]

Fix ZFS-over-iSCSI plugin w/ LIO Provider Issues
================================================

When giving the ZFS-over-iSCSI plugin with the LIO iSCSI provider a spin
on our test suite, th caching logic in the LIO module causes certain
LUNs / ZFS zvols to end up in an inconsistent state -- sometimes.

Things I have noticed going wrong are mainly dangling LUNs / zvols --
volumes that were not deleted correctly -- and the plugin sometimes
preventing me from creating a new disk for a VM where I just recently
deleted a disk, because it thinks the deleted one still exists.

The latter scenario fixes itself once the cache is invalidated after 15
seconds, but the former case requires manual fixing on the remote host
using `targetcli`.

Instead of trying to fix the cache invalidation logic, this series
removes the cache altogether, resulting in the plugin running stable
even when a lot of operations are being performed.

Another issue that may occur is not being able to delete a volume with a
"nonstandard" name, e.g.:

    pvesh create /nodes/localhost/storage/zfs-iscsi-lio/content \
        --filename vm-110-disk-2-foo-bar-baz.raw \
	--size 4G \
	--vmid 110

Note that the main culprit here is actually the '.raw' suffix.

For ZFS-over-iSCSI, all volumes and LUNs are usually created in the
format of 'vm-100-disk-0', 'vm-420-disk-2', etc. The respective parsing
logic is carefully relaxed in this case, allowing such volumes to be
deleted again.

All of this is addressed in the first three patches. The remaining
patches modernize / clean up the code of the LIO module to bring it in
line with our contemporary code.

This entire series was tested using our test suite while developing it,
but some additional testing wouldn't hurt.

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


pve-storage:

Max R. Carrara (9):
  luncmd: lio: fix various errors by removing caching mechanism
  luncmd: lio: fix LUN ops failing when passing nonstandard LUN path
  luncmd: lio: remove rest of old parsing logic
  luncmd: lio: modernize helpers
  luncmd: lio: modernize & clean up `list_view()` and `list_lun()` subs
  luncmd: lio: modernize sub `create_lun()`
  luncmd: lio: modernize sub `delete_lun()`
  luncmd: lio: modernize remaining LUN command subroutines
  luncmd: lio: use v5.36 and use subroutine signatures

 src/PVE/Storage/LunCmd/LIO.pm | 521 ++++++++++++++++------------------
 1 file changed, 243 insertions(+), 278 deletions(-)


Summary over all repositories:
  1 files changed, 243 insertions(+), 278 deletions(-)

--
Generated by murpp 0.11.0

[PATCH pve-storage 1/9] luncmd: lio: fix various errors by removing caching mechanism

[Max R. Carrara]

When put under stress (such as our testing suite), any ZFS-over-iSCSI
storage using the LIO iSCSI provider will sooner or later end up
having dangling disk images. This is due to the cached remote config
not being updated correctly on LUN creation / deletion.

This also sometimes causes a given storage to report that a virtual
disk already exists when trying to create one, even though it was
previously deleted. Only after ~15 seconds will the cache be
refreshed and creating the disk succeeds.

Note that it is unlikely that anyone will run into these things during
"casual" operation.

Instead of trying hard to get cache invalidation right, remove the
config caching mechanism altogether.

My reasoning behind this is that the average user will most likely not
be bombarding their iSCSI storage with tons of new LUNs / virtual
disks all too frequently -- and even if that is the case, they can
probably afford the bandwidth plus CPU cycles used to transfer and
parse a single JSON file a couple times.

This fixes the plugin from causing things to end up in an inconsistent
state when put under stress.

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

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index 5a53c14d..8e03652c 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -35,8 +35,6 @@ my @CONFIG_FILES = (
 my $BACKSTORE = '/backstores/block';
 
 my $SETTINGS = undef;
-my $SETTINGS_TIMESTAMP = 0;
-my $SETTINGS_MAXAGE = 15; # in seconds
 
 my @ssh_opts = ('-o', 'BatchMode=yes');
 my @ssh_cmd = ('/usr/bin/ssh', @ssh_opts);
@@ -196,36 +194,6 @@ my $get_backstore_prefix = sub {
     return $pool . '-';
 };
 
-# removes the given lu_name from the local list of luns
-my $free_lu_name = sub {
-    my ($scfg, $lu_name) = @_;
-
-    my $new = [];
-    my $target = $get_target_settings->($scfg);
-    foreach my $lun (@{ $target->{luns} }) {
-        if ($lun->{storage_object} ne "$BACKSTORE/$lu_name") {
-            push @$new, $lun;
-        }
-    }
-
-    $target->{luns} = $new;
-};
-
-# locally registers a new lun
-my $register_lun = sub {
-    my ($scfg, $idx, $volname) = @_;
-
-    my $conf = {
-        index => $idx,
-        storage_object => "$BACKSTORE/$volname",
-        is_new => 1,
-    };
-    my $target = $get_target_settings->($scfg);
-    push @{ $target->{luns} }, $conf;
-
-    return $conf;
-};
-
 # extracts the ZFS volume name from a device path
 my $extract_volname = sub {
     my ($scfg, $lunpath) = @_;
@@ -327,8 +295,6 @@ my $create_lun = sub {
         die "unable to determine new LUN index: $res->{msg}";
     }
 
-    $register_lun->($scfg, $lun_idx, $volname);
-
     # step 3: unfortunately, targetcli doesn't always save changes, no matter
     #         if auto_save_on_exit is true or not. So saving to be safe ...
     $execute_remote_command->($scfg, $timeout, $targetcli, 'saveconfig');
@@ -366,9 +332,6 @@ my $delete_lun = sub {
         # step 3: save to be safe ...
         $execute_remote_command->($scfg, $timeout, $targetcli, 'saveconfig');
 
-        # update internal cache
-        $free_lu_name->($scfg, $volname);
-
         last;
     }
 
@@ -407,15 +370,10 @@ my %lun_cmd_map = (
 sub run_lun_command {
     my ($scfg, $timeout, $method, @params) = @_;
 
-    # fetch configuration from target if we haven't yet or if it is stale
-    my $timediff = time - $SETTINGS_TIMESTAMP;
-    my $target = $get_target_settings->($scfg);
-    if (!$target || $timediff > $SETTINGS_MAXAGE) {
-        $SETTINGS_TIMESTAMP = time;
-        $parser->($scfg);
-    }
-
     die "unknown command '$method'" unless exists $lun_cmd_map{$method};
+
+    $parser->($scfg);
+
     my $msg = $lun_cmd_map{$method}->($scfg, $timeout, $method, @params);
 
     return $msg;
-- 
2.47.3

[PATCH pve-storage 2/9] luncmd: lio: fix LUN ops failing when passing nonstandard LUN path

[Max R. Carrara]

For ZFS-over-iSCSI, all volumes and LUNs are usually created in the
format of 'vm-100-disk-0', 'vm-420-disk-2', etc.

However, it is possible to create a volume with a nonstandard volume
name, for example through `pvesh`:

    pvesh create /nodes/localhost/storage/zfs-iscsi-lio/content \
        --filename vm-110-disk-2-foo-bar-baz.raw \
	--size 4G \
	--vmid 110

Note that the main culprit here is actually the '.raw' suffix, because
'\w' doesn't match on periods, apparently.

Rather than restricting the API to match our preferred format (and
potentially break things), carefully make the regex that matches on
the LUN path less restrictive by also allowing to match on periods.

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

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index 8e03652c..9dd25ad2 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -200,7 +200,7 @@ my $extract_volname = sub {
     my $volname = undef;
 
     my $base = get_base($scfg);
-    if ($lunpath =~ /^$base\/$scfg->{pool}\/([\w\-]+)$/) {
+    if ($lunpath =~ /^$base\/$scfg->{pool}\/([\w\-\.]+)$/) {
         $volname = $1;
         my $prefix = $get_backstore_prefix->($scfg);
         my $target = $get_target_settings->($scfg);
-- 
2.47.3

[PATCH pve-storage 3/9] luncmd: lio: remove rest of old parsing logic

[Max R. Carrara]

Instead of saving the entire config to a package-level variable, fetch
and parse the remote config in the LUN command dispatching sub
(serving as an entry point for in each iSCSI provider) and pass it
along wherever needed.

Do this by taking the opportunity to start modernizing the code in
this package, introducing helpers for fetching and parsing the remote
config, as well as for running remote commands in general.

The new `run_remote_command()` function in particular will be used in
future commits for better error reporting, too.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LunCmd/LIO.pm | 278 +++++++++++++++++++---------------
 1 file changed, 152 insertions(+), 126 deletions(-)

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index 9dd25ad2..ae5f3188 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -34,13 +34,138 @@ my @CONFIG_FILES = (
 );
 my $BACKSTORE = '/backstores/block';
 
-my $SETTINGS = undef;
-
 my @ssh_opts = ('-o', 'BatchMode=yes');
 my @ssh_cmd = ('/usr/bin/ssh', @ssh_opts);
 my $id_rsa_path = '/etc/pve/priv/zfs';
 my $targetcli = '/usr/bin/targetcli';
 
+my sub run_remote_command {
+    my ($scfg, $cmd, %param) = @_;
+
+    my $out_log = '';
+    my $err_log = '';
+    my $merged_log = '';
+
+    my $outfunc = sub {
+        my $out = shift;
+        $out_log .= $out . "\n";
+        $merged_log .= $out . "\n";
+    };
+
+    my $errfunc = sub {
+        my $err = shift;
+        $err_log .= $err . "\n";
+        $merged_log .= $err . "\n";
+    };
+
+    my $timeout = $param{timeout} || 10;
+
+    my $ssh_key_path = $id_rsa_path . '/' . $scfg->{portal} . '_id_rsa';
+    my $target = 'root@' . $scfg->{portal};
+
+    my $remote_cmd = [
+        @ssh_cmd, '-i', $ssh_key_path, $target, '--', $cmd->@*,
+    ];
+
+    my $result = 1;
+
+    eval {
+        run_command(
+            $remote_cmd,
+            outfunc => $outfunc,
+            errfunc => $errfunc,
+            timeout => $timeout,
+        );
+    };
+    if (my $err = $@) {
+        $result = 0;
+    }
+
+    return {
+        'err-log' => $err_log,
+        'merged-log' => $merged_log,
+        'out-log' => $out_log,
+        'remote-cmd' => $remote_cmd,
+        result => $result,
+    };
+}
+
+my sub fetch_remote_config {
+    my ($scfg, $timeout) = @_;
+
+    for my $file (@CONFIG_FILES) {
+        my $fetch_res = run_remote_command(
+            $scfg, ['cat', $file], timeout => $timeout,
+        );
+        if (!$fetch_res->{result} && $fetch_res->{'err-log'} !~ m/no such file or directory/i) {
+            my $err_msg =
+                "Failed to fetch config from iSCSI portal $scfg->{portal}:\n"
+                . $fetch_res->{'err-log'} . "\n";
+            die $err_msg;
+        }
+
+        if (my $config = $fetch_res->{'out-log'}) {
+            return JSON->new->utf8->decode($config);
+        }
+    }
+
+    die "No configuration found. Install targetcli on $scfg->{portal}\n";
+}
+
+my sub parse_remote_config {
+    my ($scfg, $remote_config_json) = @_;
+
+    my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
+
+    $tpg =~ m/^tpg(?<tpg_tag>[0-9]+)$/;
+    my $tpg_tag = $+{tpg_tag};
+
+    if (!$tpg_tag) {
+        die "Target Portal Group has invalid value, must contain string 'tpg'"
+            . " and a suffix number, for example 'tpg17'\n";
+    }
+
+    my $id = $scfg->{portal} . '.' . $scfg->{target};
+    my $parsed_config = {};
+    my $found_target = 0;
+
+    for my $target ($remote_config_json->{targets}->@*) {
+        # We are only interested in iSCSI targets
+        next if $target->{fabric} ne 'iscsi';
+        next if $target->{wwn} ne $scfg->{target};
+
+        for my $tpg ($target->{tpgs}->@*) {
+            next if $tpg->{tag} != $tpg_tag;
+
+            my $luns = [];
+            for my $lun ($tpg->{luns}->@*) {
+                $lun->{index} =~ m/^(?<index>[0-9]+)$/;
+                my $index = $+{index};
+
+                $lun->{storage_object} =~ m|^(?<storage_object>$BACKSTORE/.*)$|;
+                my $storage_object = $+{storage_object};
+
+                if (!(defined($index) && defined($storage_object))) {
+                    die "Invalid LUN definition in config!\n";
+                }
+
+                push($luns->@*, { index => $index, storage_object => $storage_object });
+            }
+
+            $parsed_config->{$id}->{luns} = $luns;
+
+            $found_target = 1;
+            last;
+        }
+    }
+
+    if (!$found_target) {
+        die "Target Portal Group '$tpg' not found!\n";
+    }
+
+    return $parsed_config;
+}
+
 my $execute_remote_command = sub {
     my ($scfg, $timeout, $remote_command, @params) = @_;
 
@@ -82,110 +207,6 @@ my $execute_remote_command = sub {
     return $res;
 };
 
-# fetch targetcli configuration from the portal
-my $read_config = sub {
-    my ($scfg, $timeout) = @_;
-
-    my $msg = '';
-    my $err = undef;
-    my $luncmd = 'cat';
-    my $target;
-    my $retry = 1;
-
-    $timeout = 10 if !$timeout;
-
-    my $output = sub { $msg .= "$_[0]\n" };
-    my $errfunc = sub { $err .= "$_[0]\n" };
-
-    $target = 'root@' . $scfg->{portal};
-
-    foreach my $oneFile (@CONFIG_FILES) {
-        my $cmd =
-            [@ssh_cmd, '-i', "$id_rsa_path/$scfg->{portal}_id_rsa", $target, $luncmd, $oneFile];
-        eval {
-            run_command($cmd, outfunc => $output, errfunc => $errfunc, timeout => $timeout);
-        };
-        if ($@) {
-            die $err if ($err !~ /No such file or directory/);
-        }
-        return $msg if $msg ne '';
-    }
-
-    die "No configuration found. Install targetcli on $scfg->{portal}\n" if $msg eq '';
-
-    return $msg;
-};
-
-my $get_config = sub {
-    my ($scfg) = @_;
-    my @conf = undef;
-
-    my $config = $read_config->($scfg, undef);
-    die "Missing config file" unless $config;
-
-    return $config;
-};
-
-# Return settings of a specific target
-my $get_target_settings = sub {
-    my ($scfg) = @_;
-
-    my $id = "$scfg->{portal}.$scfg->{target}";
-    return undef if !$SETTINGS;
-    return $SETTINGS->{$id};
-};
-
-# fetches and parses targetcli config from the portal
-my $parser = sub {
-    my ($scfg) = @_;
-    my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
-    my $tpg_tag;
-
-    if ($tpg =~ /^tpg(\d+)$/) {
-        $tpg_tag = $1;
-    } else {
-        die
-            "Target Portal Group has invalid value, must contain string 'tpg' and a suffix number, eg 'tpg17'\n";
-    }
-
-    my $config = $get_config->($scfg);
-    my $jsonconfig = JSON->new->utf8->decode($config);
-
-    my $haveTarget = 0;
-    foreach my $target (@{ $jsonconfig->{targets} }) {
-        # only interested in iSCSI targets
-        next if !($target->{fabric} eq 'iscsi' && $target->{wwn} eq $scfg->{target});
-        # find correct TPG
-        foreach my $tpg (@{ $target->{tpgs} }) {
-            if ($tpg->{tag} == $tpg_tag) {
-                my $res = [];
-                foreach my $lun (@{ $tpg->{luns} }) {
-                    my ($idx, $storage_object);
-                    if ($lun->{index} =~ /^(\d+)$/) {
-                        $idx = $1;
-                    }
-                    if ($lun->{storage_object} =~ m|^($BACKSTORE/.*)$|) {
-                        $storage_object = $1;
-                    }
-                    die "Invalid lun definition in config!\n"
-                        if !(defined($idx) && defined($storage_object));
-                    push @$res, { index => $idx, storage_object => $storage_object };
-                }
-
-                my $id = "$scfg->{portal}.$scfg->{target}";
-                $SETTINGS->{$id}->{luns} = $res;
-                $haveTarget = 1;
-                last;
-            }
-        }
-    }
-
-    # seriously unhappy if the target server lacks iSCSI target configuration ...
-    if (!$haveTarget) {
-        die "target portal group tpg$tpg_tag not found!\n";
-    }
-};
-
 # Get prefix for backstores
 my $get_backstore_prefix = sub {
     my ($scfg) = @_;
@@ -196,14 +217,15 @@ my $get_backstore_prefix = sub {
 
 # extracts the ZFS volume name from a device path
 my $extract_volname = sub {
-    my ($scfg, $lunpath) = @_;
+    my ($scfg, $config, $lunpath) = @_;
     my $volname = undef;
 
     my $base = get_base($scfg);
     if ($lunpath =~ /^$base\/$scfg->{pool}\/([\w\-\.]+)$/) {
         $volname = $1;
         my $prefix = $get_backstore_prefix->($scfg);
-        my $target = $get_target_settings->($scfg);
+        my $id = "$scfg->{portal}.$scfg->{target}";
+        my $target = $config->{$id};
         foreach my $lun (@{ $target->{luns} }) {
             # If we have a lun with the pool prefix matching this vol, then return this one
             # like pool-pve-vm-100-disk-0
@@ -219,12 +241,13 @@ my $extract_volname = sub {
 
 # retrieves the LUN index for a particular object
 my $list_view = sub {
-    my ($scfg, $timeout, $method, @params) = @_;
+    my ($scfg, $config, $timeout, $method, @params) = @_;
     my $lun = undef;
 
     my $object = $params[0];
-    my $volname = $extract_volname->($scfg, $object);
-    my $target = $get_target_settings->($scfg);
+    my $volname = $extract_volname->($scfg, $config, $object);
+    my $id = "$scfg->{portal}.$scfg->{target}";
+    my $target = $config->{$id};
 
     return undef if !defined($volname); # nothing to search for..
 
@@ -239,11 +262,12 @@ my $list_view = sub {
 
 # determines, if the given object exists on the portal
 my $list_lun = sub {
-    my ($scfg, $timeout, $method, @params) = @_;
+    my ($scfg, $config, $timeout, $method, @params) = @_;
 
     my $object = $params[0];
-    my $volname = $extract_volname->($scfg, $object);
-    my $target = $get_target_settings->($scfg);
+    my $volname = $extract_volname->($scfg, $config, $object);
+    my $id = "$scfg->{portal}.$scfg->{target}";
+    my $target = $config->{$id};
 
     foreach my $lun (@{ $target->{luns} }) {
         if ($lun->{storage_object} eq "$BACKSTORE/$volname") {
@@ -256,14 +280,14 @@ my $list_lun = sub {
 
 # adds a new LUN to the target
 my $create_lun = sub {
-    my ($scfg, $timeout, $method, @params) = @_;
+    my ($scfg, $config, $timeout, $method, @params) = @_;
 
-    if ($list_lun->($scfg, $timeout, $method, @params)) {
+    if ($list_lun->($scfg, $config, $timeout, $method, @params)) {
         die "$params[0]: LUN already exists!";
     }
 
     my $device = $params[0];
-    my $volname = $extract_volname->($scfg, $device);
+    my $volname = $extract_volname->($scfg, $config, $device);
     # Here we create a new device, so we didn't get the volname prefixed with the pool name
     # as extract_volname couldn't find a matching vol yet
     $volname = $get_backstore_prefix->($scfg) . $volname;
@@ -303,14 +327,15 @@ my $create_lun = sub {
 };
 
 my $delete_lun = sub {
-    my ($scfg, $timeout, $method, @params) = @_;
+    my ($scfg, $config, $timeout, $method, @params) = @_;
     my $res = { msg => undef };
 
     my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
 
     my $path = $params[0];
-    my $volname = $extract_volname->($scfg, $path);
-    my $target = $get_target_settings->($scfg);
+    my $volname = $extract_volname->($scfg, $config, $path);
+    my $id = "$scfg->{portal}.$scfg->{target}";
+    my $target = $config->{$id};
 
     foreach my $lun (@{ $target->{luns} }) {
         next if $lun->{storage_object} ne "$BACKSTORE/$volname";
@@ -339,20 +364,20 @@ my $delete_lun = sub {
 };
 
 my $import_lun = sub {
-    my ($scfg, $timeout, $method, @params) = @_;
+    my ($scfg, $config, $timeout, $method, @params) = @_;
 
-    return $create_lun->($scfg, $timeout, $method, @params);
+    return $create_lun->($scfg, $config, $timeout, $method, @params);
 };
 
 # needed for example when the underlying ZFS volume has been resized
 my $modify_lun = sub {
-    my ($scfg, $timeout, $method, @params) = @_;
+    my ($scfg, $config, $timeout, $method, @params) = @_;
     # Nothing to do on volume modification for LIO
     return undef;
 };
 
 my $add_view = sub {
-    my ($scfg, $timeout, $method, @params) = @_;
+    my ($scfg, $config, $timeout, $method, @params) = @_;
 
     return '';
 };
@@ -372,9 +397,10 @@ sub run_lun_command {
 
     die "unknown command '$method'" unless exists $lun_cmd_map{$method};
 
-    $parser->($scfg);
+    my $remote_config_json = fetch_remote_config($scfg, $timeout);
+    my $config = parse_remote_config($scfg, $remote_config_json);
 
-    my $msg = $lun_cmd_map{$method}->($scfg, $timeout, $method, @params);
+    my $msg = $lun_cmd_map{$method}->($scfg, $config, $timeout, $method, @params);
 
     return $msg;
 }
-- 
2.47.3

[PATCH pve-storage 4/9] luncmd: lio: modernize helpers

[Max R. Carrara]

Modernize the `get_backstore_prefix()` and `extract_volname()` helpers
by making them "proper" private subs instead of lexically-scoped sub
references.

Refactor the body of `extract_volname()` by reducing nesting, more
modern syntax, and a little guard clause.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LunCmd/LIO.pm | 50 +++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index ae5f3188..abe0454b 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -207,37 +207,41 @@ my $execute_remote_command = sub {
     return $res;
 };
 
-# Get prefix for backstores
-my $get_backstore_prefix = sub {
+my sub get_backstore_prefix {
     my ($scfg) = @_;
     my $pool = $scfg->{pool};
     $pool =~ s/\//-/g;
     return $pool . '-';
-};
+}
 
 # extracts the ZFS volume name from a device path
-my $extract_volname = sub {
+my sub extract_volname {
     my ($scfg, $config, $lunpath) = @_;
-    my $volname = undef;
 
     my $base = get_base($scfg);
-    if ($lunpath =~ /^$base\/$scfg->{pool}\/([\w\-\.]+)$/) {
-        $volname = $1;
-        my $prefix = $get_backstore_prefix->($scfg);
-        my $id = "$scfg->{portal}.$scfg->{target}";
-        my $target = $config->{$id};
-        foreach my $lun (@{ $target->{luns} }) {
-            # If we have a lun with the pool prefix matching this vol, then return this one
-            # like pool-pve-vm-100-disk-0
-            # Else, just fallback to the old name scheme which is vm-100-disk-0
-            if ($lun->{storage_object} =~ /^$BACKSTORE\/($prefix$volname)$/) {
-                return $1;
-            }
+
+    $lunpath =~ m/^$base\/$scfg->{pool}\/(?<volname>[\w\-\.]+)$/;
+    my $volname = $+{volname};
+
+    if (!defined($volname)) {
+        return;
+    }
+
+    my $prefix = get_backstore_prefix($scfg);
+    my $id = "$scfg->{portal}.$scfg->{target}";
+    my $target = $config->{$id};
+
+    for my $lun ($target->{luns}->@*) {
+        # If we have a lun with the pool prefix matching this vol, then return this one
+        # like pool-pve-vm-100-disk-0
+        # Else, just fallback to the old name scheme which is vm-100-disk-0
+        if ($lun->{storage_object} =~ /^$BACKSTORE\/(?<volname>$prefix$volname)$/) {
+            return $+{volname};
         }
     }
 
     return $volname;
-};
+}
 
 # retrieves the LUN index for a particular object
 my $list_view = sub {
@@ -245,7 +249,7 @@ my $list_view = sub {
     my $lun = undef;
 
     my $object = $params[0];
-    my $volname = $extract_volname->($scfg, $config, $object);
+    my $volname = extract_volname($scfg, $config, $object);
     my $id = "$scfg->{portal}.$scfg->{target}";
     my $target = $config->{$id};
 
@@ -265,7 +269,7 @@ my $list_lun = sub {
     my ($scfg, $config, $timeout, $method, @params) = @_;
 
     my $object = $params[0];
-    my $volname = $extract_volname->($scfg, $config, $object);
+    my $volname = extract_volname($scfg, $config, $object);
     my $id = "$scfg->{portal}.$scfg->{target}";
     my $target = $config->{$id};
 
@@ -287,10 +291,10 @@ my $create_lun = sub {
     }
 
     my $device = $params[0];
-    my $volname = $extract_volname->($scfg, $config, $device);
+    my $volname = extract_volname($scfg, $config, $device);
     # Here we create a new device, so we didn't get the volname prefixed with the pool name
     # as extract_volname couldn't find a matching vol yet
-    $volname = $get_backstore_prefix->($scfg) . $volname;
+    $volname = get_backstore_prefix($scfg) . $volname;
     my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
 
     # step 1: create backstore for device
@@ -333,7 +337,7 @@ my $delete_lun = sub {
     my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
 
     my $path = $params[0];
-    my $volname = $extract_volname->($scfg, $config, $path);
+    my $volname = extract_volname($scfg, $config, $path);
     my $id = "$scfg->{portal}.$scfg->{target}";
     my $target = $config->{$id};
 
-- 
2.47.3

[PATCH pve-storage 5/9] luncmd: lio: modernize & clean up `list_view()` and `list_lun()` subs

[Max R. Carrara]

Make the `list_view()` and `list_lun()` subs "proper" private
subroutines instead of lexically-scoped sub references.

Also improve the code style along the way.

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

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index abe0454b..483e2459 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -244,49 +244,49 @@ my sub extract_volname {
 }
 
 # retrieves the LUN index for a particular object
-my $list_view = sub {
+my sub list_view {
     my ($scfg, $config, $timeout, $method, @params) = @_;
-    my $lun = undef;
 
-    my $object = $params[0];
+    my $object = shift @params;
     my $volname = extract_volname($scfg, $config, $object);
+    return if !defined($volname); # nothing to search for..
+
     my $id = "$scfg->{portal}.$scfg->{target}";
     my $target = $config->{$id};
 
-    return undef if !defined($volname); # nothing to search for..
-
-    foreach my $lun (@{ $target->{luns} }) {
+    for my $lun ($target->{luns}->@*) {
         if ($lun->{storage_object} eq "$BACKSTORE/$volname") {
             return $lun->{index};
         }
     }
 
-    return $lun;
-};
+    return;
+}
 
-# determines, if the given object exists on the portal
-my $list_lun = sub {
+# determines if the given object exists on the portal
+my sub list_lun {
     my ($scfg, $config, $timeout, $method, @params) = @_;
 
-    my $object = $params[0];
+    my $object = shift @params;
     my $volname = extract_volname($scfg, $config, $object);
+
     my $id = "$scfg->{portal}.$scfg->{target}";
     my $target = $config->{$id};
 
-    foreach my $lun (@{ $target->{luns} }) {
+    for my $lun ($target->{luns}->@*) {
         if ($lun->{storage_object} eq "$BACKSTORE/$volname") {
             return $object;
         }
     }
 
-    return undef;
-};
+    return;
+}
 
 # adds a new LUN to the target
 my $create_lun = sub {
     my ($scfg, $config, $timeout, $method, @params) = @_;
 
-    if ($list_lun->($scfg, $config, $timeout, $method, @params)) {
+    if (list_lun($scfg, $config, $timeout, $method, @params)) {
         die "$params[0]: LUN already exists!";
     }
 
@@ -392,8 +392,8 @@ my %lun_cmd_map = (
     import_lu => $import_lun,
     modify_lu => $modify_lun,
     add_view => $add_view,
-    list_view => $list_view,
-    list_lu => $list_lun,
+    list_view => sub { list_view(@_) },
+    list_lu => sub { list_lun(@_) },
 );
 
 sub run_lun_command {
-- 
2.47.3

[PATCH pve-storage 6/9] luncmd: lio: modernize sub `create_lun()`

[Max R. Carrara]

As before, make `create_lun()` a "proper" private subroutine instead
of a lexically-scoped sub reference.

Use the new `run_remote_command()` helper for slightly easier reading
and better error messages.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LunCmd/LIO.pm | 67 ++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 20 deletions(-)

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index 483e2459..143b3fda 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -283,14 +283,15 @@ my sub list_lun {
 }
 
 # adds a new LUN to the target
-my $create_lun = sub {
+my sub create_lun {
     my ($scfg, $config, $timeout, $method, @params) = @_;
 
+    my $device = $params[0];
+
     if (list_lun($scfg, $config, $timeout, $method, @params)) {
-        die "$params[0]: LUN already exists!";
+        die "$device: LUN already exists!";
     }
 
-    my $device = $params[0];
     my $volname = extract_volname($scfg, $config, $device);
     # Here we create a new device, so we didn't get the volname prefixed with the pool name
     # as extract_volname couldn't find a matching vol yet
@@ -298,37 +299,63 @@ my $create_lun = sub {
     my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
 
     # step 1: create backstore for device
-    my @cliparams = ($BACKSTORE, 'create', "name=$volname", "dev=$device");
-    my $res = $execute_remote_command->($scfg, $timeout, $targetcli, @cliparams);
-    die $res->{msg} if !$res->{result};
+    my $backstore_res = run_remote_command(
+        $scfg,
+        [$targetcli, $BACKSTORE, 'create', "name=$volname", "dev=$device"],
+        timeout => $timeout,
+    );
+    if (!$backstore_res->{result}) {
+        my $err_msg =
+            "Error while creating backstore for '$device':\n" . $backstore_res->{'merged-log'};
+        die $err_msg;
+    }
 
     # step 2: enable unmap support on the backstore
-    @cliparams = ($BACKSTORE . '/' . $volname, 'set', 'attribute', 'emulate_tpu=1');
-    $res = $execute_remote_command->($scfg, $timeout, $targetcli, @cliparams);
-    die $res->{msg} if !$res->{result};
+    my $unmap_res = run_remote_command(
+        $scfg,
+        [$targetcli, "$BACKSTORE/$volname", 'set', 'attribute', 'emulate_tpu=1'],
+        timeout => $timeout,
+    );
+    if (!$unmap_res->{result}) {
+        my $err_msg =
+            "Error while enabling unmap support on backstore for '$device':\n"
+            . $unmap_res->{'merged-log'};
+        die $err_msg;
+    }
 
     # step 3: register lun with target
     # targetcli /iscsi/iqn.2018-04.at.bestsolution.somehost:target/tpg1/luns/ create /backstores/block/foobar
-    @cliparams = ("/iscsi/$scfg->{target}/$tpg/luns/", 'create', "$BACKSTORE/$volname");
-    $res = $execute_remote_command->($scfg, $timeout, $targetcli, @cliparams);
-    die $res->{msg} if !$res->{result};
+    my $register_res = run_remote_command(
+        $scfg,
+        [$targetcli, "/iscsi/$scfg->{target}/$tpg/luns", 'create', "$BACKSTORE/$volname"],
+        timeout => $timeout,
+    );
+    if (!$register_res->{result}) {
+        my $err_msg =
+            "Error while registering LUN for '$device':\n" . $register_res->{'merged-log'};
+        die $err_msg;
+    }
 
     # targetcli responds with "Created LUN 99"
     # not calculating the index ourselves, because the index at the portal might have
     # changed without our knowledge, so relying on the number that targetcli returns
     my $lun_idx;
-    if ($res->{msg} =~ /LUN (\d+)/) {
+    if ($register_res->{'out-log'} =~ /LUN (\d+)/) {
         $lun_idx = $1;
     } else {
-        die "unable to determine new LUN index: $res->{msg}";
+        die "Unable to determine new LUN index: " . $register_res->{'merged-log'};
     }
 
-    # step 3: unfortunately, targetcli doesn't always save changes, no matter
+    # step 4: unfortunately, targetcli doesn't always save changes, no matter
     #         if auto_save_on_exit is true or not. So saving to be safe ...
-    $execute_remote_command->($scfg, $timeout, $targetcli, 'saveconfig');
+    my $saveconfig_res = run_remote_command(
+        $scfg, [$targetcli, 'saveconfig'], timeout => $timeout,
+    );
+    warn("Could not save LUN config: " . $saveconfig_res->{'merged-log'} . "\n")
+        if !$saveconfig_res->{result};
 
-    return $res->{msg};
-};
+    return $register_res->{'merged-log'};
+}
 
 my $delete_lun = sub {
     my ($scfg, $config, $timeout, $method, @params) = @_;
@@ -370,7 +397,7 @@ my $delete_lun = sub {
 my $import_lun = sub {
     my ($scfg, $config, $timeout, $method, @params) = @_;
 
-    return $create_lun->($scfg, $config, $timeout, $method, @params);
+    return create_lun($scfg, $config, $timeout, $method, @params);
 };
 
 # needed for example when the underlying ZFS volume has been resized
@@ -387,7 +414,7 @@ my $add_view = sub {
 };
 
 my %lun_cmd_map = (
-    create_lu => $create_lun,
+    create_lu => sub { create_lun(@_) },
     delete_lu => $delete_lun,
     import_lu => $import_lun,
     modify_lu => $modify_lun,
-- 
2.47.3

[PATCH pve-storage 7/9] luncmd: lio: modernize sub `delete_lun()`

[Max R. Carrara]

As before, make `delete_lun()` a "proper" private subroutine instead
of a lexically-scoped sub reference.

Use the new `run_remote_command()` helper for easier reading and
better error messages. Clean up the code style along the way a little.

Remove the old `$execute_remote_command` helper, as it is not used
anymore now.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LunCmd/LIO.pm | 96 +++++++++++++----------------------
 1 file changed, 35 insertions(+), 61 deletions(-)

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index 143b3fda..f4ad2ac8 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -166,47 +166,6 @@ my sub parse_remote_config {
     return $parsed_config;
 }
 
-my $execute_remote_command = sub {
-    my ($scfg, $timeout, $remote_command, @params) = @_;
-
-    my $msg = '';
-    my $err = undef;
-    my $target;
-    my $cmd;
-    my $res = ();
-
-    $timeout = 10 if !$timeout;
-
-    my $output = sub { $msg .= "$_[0]\n" };
-    my $errfunc = sub { $err .= "$_[0]\n" };
-
-    $target = 'root@' . $scfg->{portal};
-    $cmd = [
-        @ssh_cmd,
-        '-i',
-        "$id_rsa_path/$scfg->{portal}_id_rsa",
-        $target,
-        '--',
-        $remote_command,
-        @params,
-    ];
-
-    eval { run_command($cmd, outfunc => $output, errfunc => $errfunc, timeout => $timeout); };
-    if ($@) {
-        $res = {
-            result => 0,
-            msg => $err,
-        };
-    } else {
-        $res = {
-            result => 1,
-            msg => $msg,
-        };
-    }
-
-    return $res;
-};
-
 my sub get_backstore_prefix {
     my ($scfg) = @_;
     my $pool = $scfg->{pool};
@@ -357,10 +316,8 @@ my sub create_lun {
     return $register_res->{'merged-log'};
 }
 
-my $delete_lun = sub {
+my sub delete_lun {
     my ($scfg, $config, $timeout, $method, @params) = @_;
-    my $res = { msg => undef };
-
     my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
 
     my $path = $params[0];
@@ -368,31 +325,48 @@ my $delete_lun = sub {
     my $id = "$scfg->{portal}.$scfg->{target}";
     my $target = $config->{$id};
 
-    foreach my $lun (@{ $target->{luns} }) {
+    for my $lun ($target->{luns}->@*) {
         next if $lun->{storage_object} ne "$BACKSTORE/$volname";
 
         # step 1: delete the lun
-        my @cliparams = ("/iscsi/$scfg->{target}/$tpg/luns/", 'delete', "lun$lun->{index}");
-        my $res = $execute_remote_command->($scfg, $timeout, $targetcli, @cliparams);
-        do {
-            die $res->{msg};
-        } unless $res->{result};
+        my $lun_path = "/iscsi/$scfg->{target}/$tpg/luns/";
+        my $lun_id = 'lun' . $lun->{index};
+        my $lun_delete_res = run_remote_command(
+            $scfg,
+            [$targetcli, $lun_path, 'delete', $lun_id],
+            timeout => $timeout,
+        );
+        if (!$lun_delete_res->{result}) {
+            my $err_msg = "Failed to delete LUN '$lun_id' in '$lun_path':\n"
+                . $lun_delete_res->{'merged-log'};
+            die $err_msg;
+        }
 
         # step 2: delete the backstore
-        @cliparams = ($BACKSTORE, 'delete', $volname);
-        $res = $execute_remote_command->($scfg, $timeout, $targetcli, @cliparams);
-        do {
-            die $res->{msg};
-        } unless $res->{result};
+        my $backstore_delete_res = run_remote_command(
+            $scfg,
+            [$targetcli, $BACKSTORE, 'delete', $volname],
+            timeout => $timeout,
+        );
+        if (!$backstore_delete_res->{result}) {
+            my $err_msg = "Failed to delete volume '$volname' from backstore '$BACKSTORE':\n"
+                . $backstore_delete_res->{'merged-log'};
+            die $err_msg;
+        }
 
-        # step 3: save to be safe ...
-        $execute_remote_command->($scfg, $timeout, $targetcli, 'saveconfig');
+        # step 3: unfortunately, targetcli doesn't always save changes, no matter
+        #         if auto_save_on_exit is true or not. So saving to be safe ...
+        my $saveconfig_res = run_remote_command(
+            $scfg, [$targetcli, 'saveconfig'], timeout => $timeout,
+        );
+        warn("Could not save LUN config: " . $saveconfig_res->{'merged-log'} . "\n")
+            if !$saveconfig_res->{result};
 
-        last;
+        return $backstore_delete_res->{'out-log'};
     }
 
-    return $res->{msg};
-};
+    die "Failed to delete LUN '$path'";
+}
 
 my $import_lun = sub {
     my ($scfg, $config, $timeout, $method, @params) = @_;
@@ -415,7 +389,7 @@ my $add_view = sub {
 
 my %lun_cmd_map = (
     create_lu => sub { create_lun(@_) },
-    delete_lu => $delete_lun,
+    delete_lu => sub { delete_lun(@_) },
     import_lu => $import_lun,
     modify_lu => $modify_lun,
     add_view => $add_view,
-- 
2.47.3

[PATCH pve-storage 8/9] luncmd: lio: modernize remaining LUN command subroutines

[Max R. Carrara]

As done with the subs before, make the `import_lun()`, `modify_lun()`,
and `add_view()` "proper" private subroutines.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/LunCmd/LIO.pm | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index f4ad2ac8..534a0508 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -368,31 +368,31 @@ my sub delete_lun {
     die "Failed to delete LUN '$path'";
 }
 
-my $import_lun = sub {
+my sub import_lun {
     my ($scfg, $config, $timeout, $method, @params) = @_;
 
     return create_lun($scfg, $config, $timeout, $method, @params);
-};
+}
 
 # needed for example when the underlying ZFS volume has been resized
-my $modify_lun = sub {
+my sub modify_lun {
     my ($scfg, $config, $timeout, $method, @params) = @_;
     # Nothing to do on volume modification for LIO
-    return undef;
-};
+    return;
+}
 
-my $add_view = sub {
+my sub add_view {
     my ($scfg, $config, $timeout, $method, @params) = @_;
 
     return '';
-};
+}
 
 my %lun_cmd_map = (
     create_lu => sub { create_lun(@_) },
     delete_lu => sub { delete_lun(@_) },
-    import_lu => $import_lun,
-    modify_lu => $modify_lun,
-    add_view => $add_view,
+    import_lu => sub { import_lun(@_) },
+    modify_lu => sub { modify_lun(@_) },
+    add_view => sub { add_view(@_) },
     list_view => sub { list_view(@_) },
     list_lu => sub { list_lun(@_) },
 );
-- 
2.47.3

[PATCH pve-storage 9/9] luncmd: lio: use v5.36 and use subroutine signatures

[Max R. Carrara]

Finish modernizing the LIO module through `use v5.36;` and giving
every non-anonymous subroutine its respective signature.

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

diff --git a/src/PVE/Storage/LunCmd/LIO.pm b/src/PVE/Storage/LunCmd/LIO.pm
index 534a0508..62125e9e 100644
--- a/src/PVE/Storage/LunCmd/LIO.pm
+++ b/src/PVE/Storage/LunCmd/LIO.pm
@@ -19,8 +19,8 @@ package PVE::Storage::LunCmd::LIO;
 # Author: udo.rader@bestsolution.at
 # -----------------------------------------------------------------
 
-use strict;
-use warnings;
+use v5.36;
+
 use PVE::Tools qw(run_command);
 use JSON;
 
@@ -39,9 +39,7 @@ my @ssh_cmd = ('/usr/bin/ssh', @ssh_opts);
 my $id_rsa_path = '/etc/pve/priv/zfs';
 my $targetcli = '/usr/bin/targetcli';
 
-my sub run_remote_command {
-    my ($scfg, $cmd, %param) = @_;
-
+my sub run_remote_command($scfg, $cmd, %param) {
     my $out_log = '';
     my $err_log = '';
     my $merged_log = '';
@@ -90,9 +88,7 @@ my sub run_remote_command {
     };
 }
 
-my sub fetch_remote_config {
-    my ($scfg, $timeout) = @_;
-
+my sub fetch_remote_config($scfg, $timeout) {
     for my $file (@CONFIG_FILES) {
         my $fetch_res = run_remote_command(
             $scfg, ['cat', $file], timeout => $timeout,
@@ -112,9 +108,7 @@ my sub fetch_remote_config {
     die "No configuration found. Install targetcli on $scfg->{portal}\n";
 }
 
-my sub parse_remote_config {
-    my ($scfg, $remote_config_json) = @_;
-
+my sub parse_remote_config($scfg, $remote_config_json) {
     my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
 
     $tpg =~ m/^tpg(?<tpg_tag>[0-9]+)$/;
@@ -166,17 +160,14 @@ my sub parse_remote_config {
     return $parsed_config;
 }
 
-my sub get_backstore_prefix {
-    my ($scfg) = @_;
+my sub get_backstore_prefix($scfg) {
     my $pool = $scfg->{pool};
     $pool =~ s/\//-/g;
     return $pool . '-';
 }
 
 # extracts the ZFS volume name from a device path
-my sub extract_volname {
-    my ($scfg, $config, $lunpath) = @_;
-
+my sub extract_volname($scfg, $config, $lunpath) {
     my $base = get_base($scfg);
 
     $lunpath =~ m/^$base\/$scfg->{pool}\/(?<volname>[\w\-\.]+)$/;
@@ -203,9 +194,7 @@ my sub extract_volname {
 }
 
 # retrieves the LUN index for a particular object
-my sub list_view {
-    my ($scfg, $config, $timeout, $method, @params) = @_;
-
+my sub list_view($scfg, $config, $timeout, $method, @params) {
     my $object = shift @params;
     my $volname = extract_volname($scfg, $config, $object);
     return if !defined($volname); # nothing to search for..
@@ -223,9 +212,7 @@ my sub list_view {
 }
 
 # determines if the given object exists on the portal
-my sub list_lun {
-    my ($scfg, $config, $timeout, $method, @params) = @_;
-
+my sub list_lun($scfg, $config, $timeout, $method, @params) {
     my $object = shift @params;
     my $volname = extract_volname($scfg, $config, $object);
 
@@ -242,9 +229,7 @@ my sub list_lun {
 }
 
 # adds a new LUN to the target
-my sub create_lun {
-    my ($scfg, $config, $timeout, $method, @params) = @_;
-
+my sub create_lun($scfg, $config, $timeout, $method, @params) {
     my $device = $params[0];
 
     if (list_lun($scfg, $config, $timeout, $method, @params)) {
@@ -316,8 +301,7 @@ my sub create_lun {
     return $register_res->{'merged-log'};
 }
 
-my sub delete_lun {
-    my ($scfg, $config, $timeout, $method, @params) = @_;
+my sub delete_lun($scfg, $config, $timeout, $method, @params) {
     my $tpg = $scfg->{lio_tpg} || die "Target Portal Group not set, aborting!\n";
 
     my $path = $params[0];
@@ -368,22 +352,17 @@ my sub delete_lun {
     die "Failed to delete LUN '$path'";
 }
 
-my sub import_lun {
-    my ($scfg, $config, $timeout, $method, @params) = @_;
-
+my sub import_lun($scfg, $config, $timeout, $method, @params) {
     return create_lun($scfg, $config, $timeout, $method, @params);
 }
 
 # needed for example when the underlying ZFS volume has been resized
-my sub modify_lun {
-    my ($scfg, $config, $timeout, $method, @params) = @_;
+my sub modify_lun($scfg, $config, $timeout, $method, @params) {
     # Nothing to do on volume modification for LIO
     return;
 }
 
-my sub add_view {
-    my ($scfg, $config, $timeout, $method, @params) = @_;
-
+my sub add_view($scfg, $config, $timeout, $method, @params) {
     return '';
 }
 
@@ -397,9 +376,7 @@ my %lun_cmd_map = (
     list_lu => sub { list_lun(@_) },
 );
 
-sub run_lun_command {
-    my ($scfg, $timeout, $method, @params) = @_;
-
+sub run_lun_command($scfg, $timeout, $method, @params) {
     die "unknown command '$method'" unless exists $lun_cmd_map{$method};
 
     my $remote_config_json = fetch_remote_config($scfg, $timeout);
@@ -410,8 +387,7 @@ sub run_lun_command {
     return $msg;
 }
 
-sub get_base {
-    my ($scfg) = @_;
+sub get_base($scfg) {
     return $scfg->{'zfs-base-path'} || '/dev';
 }
 
-- 
2.47.3