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

[Stoiko Ivanov <s.ivanov@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