[pve-devel] [PATCH] fix #957 iscsi: don't check tcp connection directly

[Mira Limbeck]

Thank you for the patch!

some comments inline

> +sub iscsi_test_session {
> +    my ($portal, $sid) = @_;
> +    my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $sid, '-P1'];
> +
> +    my $res = 0;
> +    eval {
> +        run_command($cmd, errmsg => 'iscsi session test failed', outfunc => sub {
> +            my $line = shift;
> +            if ($line =~ m/^\s+iSCSI Session State: LOGGED_IN\s*$/) {
> +                $res = 1;
> +            }
> +        });
> +    };
> +    if (my $err = $@) {
> +        die $err;
> +    };
> +    return $res;
> +}
You pass in `$portal` which is never used. This should be removed unless
you have a use case for which it might be needed in the future?


> -
> +    my $cache = {};
>      for my $portal (@$portals) {
The $portal variable is never used below. Is it necessary to even loop
over all of them when just checking the cached sessions?
The session loop below will be run for each of the portals, leading to a
portals * sessions amount of iscsi_test_session calls.

> -	my $result = iscsi_test_portal($portal);
> -	return $result if $result;
> +        my $sessions = iscsi_session($cache, $scfg->{target});
> +        for my $session (@$sessions) {
> +            my $result = iscsi_test_session($portal, $session->{session_id});
> +            return $result if $result;
> +        }
>      }

Making this change in `check_connection` leads to storages never being
activated, since there's an early exit in case the storage is not
reachable (src/PVE/Storage.pm:1196):
```
    if (! eval { $plugin->check_connection($storeid, $scfg) }) {
	die "connection check for storage '$storeid' failed - $@\n" if $@;
	die "storage '$storeid' is not online\n";
    }
```

Maybe this could be changed to first see if there's a session available,
and if not, call `iscsi_test_portal`.
And if there's a session available, one can check the session state instead.
With this, we would still need the portals list.