[pve-devel] [PATCH storage] dirplugin: fix #3986: check for trailing slashes

[pve-devel] [PATCH storage] dirplugin: fix #3986: check for trailing slashes

[Daniel Herzig]

This patch removes trailing slashes from manually entered
paths by adding an additional if clause in the sub
check_config.

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



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

Re: [pve-devel] [PATCH storage] dirplugin: fix #3986: check for trailing slashes

[Stoiko Ivanov]

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

Re: [pve-devel] [PATCH storage] dirplugin: fix #3986: check for trailing slashes

[Daniel Herzig]

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