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

["Max R. Carrara" <m.carrara@proxmox.com>]

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