From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 6BA6499235
 for <pve-devel@lists.proxmox.com>; Tue, 10 Oct 2023 14:55:06 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 4768032F97
 for <pve-devel@lists.proxmox.com>; Tue, 10 Oct 2023 14:54:36 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pve-devel@lists.proxmox.com>; Tue, 10 Oct 2023 14:54:34 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8EEAE446C3;
 Tue, 10 Oct 2023 14:54:34 +0200 (CEST)
Message-ID: <46997106-a66c-cc16-15b8-a72d2fe7dac5@proxmox.com>
Date: Tue, 10 Oct 2023 14:54:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Content-Language: en-US
To: ykonotopov@gnome.org, pve-devel@lists.proxmox.com
References: <20230816115627.59681-1-ykonotopov@gnome.org>
 <20230816115627.59681-2-ykonotopov@gnome.org>
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <20230816115627.59681-2-ykonotopov@gnome.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 1.585 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -3.339 Looks like a legit reply (A)
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [proxmox.com, iscsiplugin.pm, plugin.pm]
Subject: Re: [pve-devel] [PATCH storage 1/1] iscsi: allow to configure
 multiple portals
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Tue, 10 Oct 2023 12:55:06 -0000

Hi,
thank you for the contribution! Please prepend the commit title with
"fix #254: ".

Am 16.08.23 um 13:56 schrieb ykonotopov@gnome.org:
> From: Yuri Konotopov <ykonotopov@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