From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
	by lore.proxmox.com (Postfix) with ESMTPS id F24101FF16B
	for <inbox@lore.proxmox.com>; Thu, 14 Nov 2024 10:41:35 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id 8B0C32D395;
	Thu, 14 Nov 2024 10:41:37 +0100 (CET)
From: Daniel Herzig <d.herzig@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
In-Reply-To: <ZzUFGD4FLUd51-Is@rosa.proxmox.com> (Stoiko Ivanov's message of
 "Wed, 13 Nov 2024 20:59:20 +0100")
References: <20241113163249.76434-1-d.herzig@proxmox.com>
 <ZzUFGD4FLUd51-Is@rosa.proxmox.com>
Date: Thu, 14 Nov 2024 10:41:00 +0100
Message-ID: <87y11mxidv.fsf@proxmox.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.392 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_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible
 spam tricks
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 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. [dirplugin.pm, proxmox.com]
Subject: Re: [pve-devel] [PATCH storage] dirplugin: fix #3986: check for
 trailing slashes
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>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>

Thanks for your feedback, I started working on a v2 including your suggestions.

Stoiko Ivanov <s.ivanov@proxmox.com> writes:

> Thanks for tackling this long-open issue!
>
> On Wed, Nov 13, 2024 at 05:32:49PM +0100, Daniel Herzig wrote:
>> This patch removes trailing slashes from manually entered
>> paths by adding an additional if clause in the sub
>> check_config.
> For me the context of the change, and why something in the commit is done
> the way it is, is more helpful than explaining what the code does (this
> should be clear from the code itself).
>
> I wanted to drop a small suggestion for a shortening of the perl-code (see
> comment inline below) - but then wondered why an adaptation of the path
> happens in check_config of a storage plugin - as it's been a while since I
> dealt with the storage-config.
>
> The change might be ok - as PVE::SectionConfig calls check_config of the
> plugin when parsing the configuration file - at least to me that's not
> directly obvious.
>
> Also mentioning how you tested a change can also be helpful to reviewers.
>
I'm currently testing the change by either adding a new dir storage
setting a path with trailing slashes via the GUI and `pvesm add dir
$STORAGENAME --path $PATH_TO_DIR`.

What's interesting here, is that if there's a path with trailing slash
set before applying the patch, it only seems to get updated once a new
dir storage gets added after applying it. I haven't yet finally figured out,
when exactly the config_check is getting triggered.

I.e. further tackling needed.

>> 
>> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
>> ---
>>  src/PVE/Storage/DirPlugin.pm | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
>> index 2efa8d5..9dcdf4a 100644
>> --- a/src/PVE/Storage/DirPlugin.pm
>> +++ b/src/PVE/Storage/DirPlugin.pm
>> @@ -244,6 +244,9 @@ sub check_config {
>>      if ($opts->{path} !~ m|^/[-/a-zA-Z0-9_.@]+$|) {
>>  	die "illegal path for directory storage: $opts->{path}\n";
>>      }
>> +    if ($opts->{path} =~ /.*\/$/) {
>> +	$opts->{path} = substr($opts->{path}, 0 , -1);
>> +    }
> without explicit testing - this could be put as:
> $opts->{path} =~ s/(.*)\/$/$1/;
> (you could also add + to strip multiple trailing slashes)
>

This is cool and works, thanks!

>>      return $opts;
>>  }
>>  
>> -- 
>> 2.39.5
>> 
>> 
>> 
>> _______________________________________________
>> 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


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel