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

[Daniel Herzig <d.herzig@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