[pve-devel] [PATCH v7 pve-storage 03/10] Basic iscsiadm interaction code

[Fabian Grünbichler]

high-level question:

would it make sense to wrap the iscsiadm calls once more? seems to me
like that might save a lot of duplication, and stuff like the -m and -p
parameters could become parameters of the wrapper.


On Tue, Jun 20, 2017 at 10:39:55PM +0200, mir at datanom.net wrote:
> From: Michael Rasmussen <mir at datanom.net>
> 
> Signed-off-by: Michael Rasmussen <mir at datanom.net>
> ---
>  PVE/Storage/FreeNASPlugin.pm | 119 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/PVE/Storage/FreeNASPlugin.pm b/PVE/Storage/FreeNASPlugin.pm
> index a50a2f6..e930cea 100644
> --- a/PVE/Storage/FreeNASPlugin.pm
> +++ b/PVE/Storage/FreeNASPlugin.pm
> @@ -22,6 +22,9 @@ my $rows_per_request = 50; # limit for get requests
>                             # in FreeNAS API is 20) can cause race conditions
>                             # on the FreeNAS host (seems like an unstable
>                             # pagination algorithm implemented in FreeNAS)
> +my $version;
> +my $target_prefix = 'iqn.2005-10.org.freenas.ctl'; # automatically prepended
> +                                                   # in FreeNAS
>  
>  # Configuration
>  
> @@ -242,6 +245,121 @@ my $freenas_list_zvol = sub {
>      return $list;
>  };
>  
> +my $os_request = sub {
> +    my ($cmd, $noerr, $timeout) = @_;
> +
> +    $timeout = 5 unless $timeout;
> +    $noerr = 0 unless $noerr;

that should not be needed?

> +
> +    my $text = '';
> +
> +    my $output = sub {
> +        my $line = shift;
> +        $text .= "$line\n";
> +    };
> +
> +    my $exit_code = run_command($cmd, noerr => $noerr, errfunc => $output, outfunc => $output, timeout => $timeout);
> +
> +    return wantarray ? ($exit_code, $text) : $exit_code;
> +};
> +
> +my $freenas_get_target_name = sub {
> +    my ($scfg, $volname) = @_;
> +    my $name = undef;
> +    

whitespace error (blank lines should not contain spaces)

> +    if ($volname =~ /^(vm|base)-(\d+)-/) {
> +        $name = "vm-$2";
> +        return "$target_prefix\:$name";
> +    }
> +
> +    return undef;
> +};
> +
> +my $get_sid = sub {
> +    my ($scfg, $volname) = @_;
> +    my $sid = -1;
> +    my $text = '';
> +    my $exit = 0;
> +    

whitespace error

> +    my $target = $freenas_get_target_name->($scfg, $volname);
> +
> +    eval {
> +        ($exit, $text) = $os_request->(
> +            ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '-s'], 1, 60);
> +    };
> +    if ($@) {
> +        # An exist code of 21 or 22 means no active session otherwise an error
> +        if ($exit != 21 || $exit != 22) {
> +            die "$@\n";
> +        }
> +    }

run_command with noerr does not work like this. if you pass noerr,
run_command will not die on a non-zero exit code, so $@ won't be defined
and you never take this if (well, you would if you run into the timeout,
or if the iscsiadm binary is missing, or ... - but then $exit is undef
anyway, so this is moot).

you need to check both $@ (timeout or other basic error) and $exit (for
the exit code), but separately.

also, please use the full binary path to be $PATH independent (this
applies to all the iscsiadm calls!).

> +    if ($text =~ /.*\[sid\:\s*(\d+),\s*.*/) {
> +        $sid = $1;
> +    }
> +
> +    return $sid;
> +};
> +
> +my $create_session = sub {
> +    my ($scfg, $volname) = @_;
> +    my $sid = -1;
> +    my $exit = undef;
> +    

whitespace

> +    my $target = $freenas_get_target_name->($scfg, $volname);
> +
> +    eval {
> +        $exit = $os_request->(
> +            ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '--login'], 1, 60);
> +        if ($exit == 21) {
> +            eval {
> +                $os_request->(
> +                    ['iscsiadm', '-m', 'discovery', '-t', 'sendtargets', '-p', $scfg->{portal}], 0, 60);
> +                $os_request->(
> +                    ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '--login'], 0, 60);
> +            };
> +        }

this one is a bit better - but what if the inner eval fails? shouldn't
we die then with some meaningful error message? note that $@ is not
propagated from inner to outer eval, so the error handling below is not
triggered.

> +    };
> +    if ($@) {
> +        if ($exit == 21) {

this is wrong again, see above for get_sid.

> +            eval {
> +                $os_request->(
> +                    ['iscsiadm', '-m', 'discovery', '-t', 'sendtargets', '-p', $scfg->{portal}], 0, 60);
> +                $os_request->(
> +                    ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '--login'], 0, 60);
> +            };

see above?

> +        } else {
> +            die "$@\n";
> +        }
> +    }
> +    eval {
> +        $sid = $get_sid->($scfg, $volname);
> +    };
> +    die "$@\n" if $@;
> +    die "Could not create session\n" if $sid < 0;
> +    

whitespace

> +    return $sid;
> +};
> +
> +my $delete_session = sub {
> +    my ($scfg, $sid) = @_;
> +    

whitespace

> +    eval {
> +        $os_request->(['iscsiadm', '-m', 'session', '-r', $sid, '--logout'], 0, 60);
> +    };
> +    warn "Delete session failed: $@\n" if $@;
> +};
> +
> +my $remove_local_lun = sub {
> +    my ($id) = @_;
> +    

whitespace

> +    open (my $fh, '>', "/sys/bus/scsi/devices/$id/delete") or
> +        warn "Remove local LUN failed: $!\n";
> +    if ($fh) {
> +        print $fh '1';
> +        close $fh;
> +    }
> +};

I'd prefer something like:

my $file = "/sys/...";
eval {
  open(my $fh, '>', $file) or die "failed to open $file for writing: $!\n";
  print {$fh} "1\n" or die "failed to write to $file\n";
  close($fh);
};
die "Removal of local LUN failed: $@\n" if $@;

(or warn if it is really only a warning?)


> +
>  # Storage implementation
>  
>  sub volume_size_info {
> @@ -256,6 +374,7 @@ sub volume_size_info {
>      die "Could not get zfs volume size\n";
>  }
>  
> +
>  sub parse_volname {
>      my ($class, $volname) = @_;
>  

I guess this last one was an accident?

> -- 
> 2.11.0
> 
> 
> ----
> 
> This mail was virus scanned and spam checked before delivery.
> This mail is also DKIM signed. See header dkim-signature.
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel