Thanks for your feedback, I started working on a v2 including your suggestions.
Stoiko Ivanov <s.iva...@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.her...@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
