Hi, thank you for the contribution! Please prepend the commit title with "fix #254: ".
Am 16.08.23 um 13:56 schrieb ykonoto...@gnome.org: > From: Yuri Konotopov <ykonoto...@gnome.org> > > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=254 To accept contributions, we need your Signed-off-by trailer here and you need to agree to the Harmony CLA (assuming you haven't sent it to us already): https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright > --- > PVE/Storage/ISCSIPlugin.pm | 44 +++++++++++++++++++++++--------------- > PVE/Storage/Plugin.pm | 18 ++++++++++++++++ > 2 files changed, 45 insertions(+), 17 deletions(-) > > diff --git a/PVE/Storage/ISCSIPlugin.pm b/PVE/Storage/ISCSIPlugin.pm > index a79fcb0..e056393 100644 > --- a/PVE/Storage/ISCSIPlugin.pm > +++ b/PVE/Storage/ISCSIPlugin.pm > @@ -69,24 +69,30 @@ sub iscsi_test_portal { > } > > sub iscsi_discovery { > - my ($portal) = @_; > + my @portals = @{$_[0]}; Style nit: Usually we'd still write this as my ($portals) = @_; and then use $portals->@* whenever we need to access the array below. Like that it's nicer to extend if a new parameter is added. > > check_iscsi_support (); > > my $res = {}; > - return $res if !iscsi_test_portal($portal); # fixme: raise exception > here? > + foreach my $portal (@portals) > + { Style nit: We use 'for' instead of 'foreach' for new code and the opening bracket should be on the same line > + next if !iscsi_test_portal($portal); # fixme: raise exception here? > > - my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', > '--portal', $portal]; > - run_command($cmd, outfunc => sub { > - my $line = shift; > + my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', > '--portal', $portal]; > + run_command($cmd, outfunc => sub { > + my $line = shift; > > - if ($line =~ m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) { > - my $portal = $1; > - my $target = $2; > - # one target can have more than one portal (multipath). > - push @{$res->{$target}}, $portal; > - } > - }); > + if ($line =~ m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) { > + my $portal = $1; > + my $target = $2; > + # one target can have more than one portal (multipath). > + push @{$res->{$target}}, $portal; > + } > + }); > + > + # In case of multipath we want to exit on any portal available> + > last; > + } > > return $res; > } > @@ -96,7 +102,7 @@ sub iscsi_login { > > check_iscsi_support(); > > - eval { iscsi_discovery($portal_in); }; > + eval { iscsi_discovery(PVE::Storage::Plugin::get_portals($portal_in)); }; > warn $@ if $@; > > run_command([$ISCSIADM, '--mode', 'node', '--targetname', $target, > '--login']); > @@ -245,8 +251,8 @@ sub properties { > type => 'string', > }, > portal => { > - description => "iSCSI portal (IP or DNS name with optional port).", > - type => 'string', format => 'pve-storage-portal-dns', > + description => "iSCSI portal (IP or DNS name with optional port). > Multiple portals can be separated by comma (for multipath).", Since separation for lists is (mostly) consistent in our API, you could also just write: "For multipath, multiple portals can be specified." > + type => 'string', format => 'pve-storage-portals-dns', Please note that for lists, you can just use the existing format and append "-list" in our schemas, i.e. using 'pve-storage-portal-dns-list' should give you what you want already and you don't need to register a new format. > }, > }; > } > @@ -403,8 +409,12 @@ sub deactivate_storage { > sub check_connection { > my ($class, $storeid, $scfg) = @_; > > - my $portal = $scfg->{portal}; > - return iscsi_test_portal($portal); > + foreach my $portal > (@{PVE::Storage::Plugin::get_portals($scfg->{portal})}) { > + my $result = iscsi_test_portal($portal); > + return $result if $result; > + } > + > + return 0; > } > > sub volume_resize { > diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm > index c323085..ee69b14 100644 > --- a/PVE/Storage/Plugin.pm > +++ b/PVE/Storage/Plugin.pm > @@ -205,6 +205,13 @@ sub content_hash_to_string { > return join(',', @cta); > } > > +sub get_portals { > + my $portal_cfg = shift; > + my $portals = [split(',', $portal_cfg)]; > + map { s/^\s+|\s+$//g; } @{$portals}; You can use the PVE::Tools::split_list() helper instead of this one. > + return $portals; > +} > + > sub valid_content_types { > my ($type) = @_; > > @@ -301,6 +308,17 @@ sub verify_portal_dns { > return $portal; > } > > +PVE::JSONSchema::register_format('pve-storage-portals-dns', > \&verify_portals_dns); > +sub verify_portals_dns { > + my ($portal_in, $noerr) = @_; > + > + foreach my $portal (@{get_portals($portal_in)}) { > + verify_portal_dns($portal, $noerr); > + } > + > + return $portal_in; > +} > + > PVE::JSONSchema::register_format('pve-storage-content', \&verify_content); > sub verify_content { > my ($ct, $noerr) = @_; Best Regards, Fiona _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
