On 3/7/25 12:59, Mira Limbeck wrote:
> 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.
Sorry, read that wrong. Best case would be the same session for each of
the portals, so `portals * 1` since there's an early exit if the result
is truthy. And only in the worst case (no session that's logged in)
would be `portals * sessions`.
>
>> - 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.
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
