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 [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id D4C381FF15C
for <inbox@lore.proxmox.com>; Wed, 13 Nov 2024 20:59:26 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 8DCCF1C80E;
Wed, 13 Nov 2024 20:59:26 +0100 (CET)
Date: Wed, 13 Nov 2024 20:59:20 +0100
From: Stoiko Ivanov <s.ivanov@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Message-ID: <ZzUFGD4FLUd51-Is@rosa.proxmox.com>
References: <20241113163249.76434-1-d.herzig@proxmox.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <20241113163249.76434-1-d.herzig@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.072 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
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
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 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.
>
> 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)
> 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