Re: [PATCH pve-storage 2/2] fix #6050: cifs: normalize share and subdir paths for mount and status

["Jakob Klocker" <j.klocker@proxmox.com>]

Comments inline. I'd also suggest mentioning why you didn't reuse the
`canonpath` function you used in the previous patch here.

On Fri Aug 7, 2026 at 11:36 AM CEST, David Riley wrote:
> Manual edits to the storage configuration bypass strict API
> validation. If a user adds extra or trailing slashes to the share or
> subdirectory paths, it causes two distinct issues for CIFS storages:
>
> * The Linux VFS CIFS module expects strict UNC [0] formatting and
>   will reject the mount attempt with a 'Malformed UNC in devname'
>   error.
> * The internal path used by the status daemon fails to match the
>   normalized path reported in /proc/mounts, causing working storages
>   to continuously report as 'inactive'.
>
> Sanitize the share and subdirectory strings by stripping leading and
> trailing slashes before executing mounts, verifying mount status, and
> testing the SMB connection. This ensures working storages mount
> successfully and are correctly recognized as active.
>
> [0] https://docs.kernel.org/admin-guide/cifs/usage.html#cifs-vfs-mount-options
>
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6050
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---
>  src/PVE/Storage/CIFSPlugin.pm | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/src/PVE/Storage/CIFSPlugin.pm b/src/PVE/Storage/CIFSPlugin.pm
> index 54f0f4e..78ed88c 100644
> --- a/src/PVE/Storage/CIFSPlugin.pm
> +++ b/src/PVE/Storage/CIFSPlugin.pm
> @@ -2,15 +2,20 @@ package PVE::Storage::CIFSPlugin;
>  
>  use strict;
>  use warnings;
> +
> +use File::Path;
>  use Net::IP;
> +
>  use PVE::Tools qw(run_command);
>  use PVE::ProcFSTools;
> -use File::Path;
>  use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  
>  use base qw(PVE::Storage::Plugin);
>  
> +my $RM_LEADING_SLASHES = qr{^/+};
> +my $RM_TRAILING_SLASHES = qr{/+$};

Since you're removing the slashes at three different locations in this
file, I'd prefere having a function which does that. Would make the code
easier to read. You could also think about moving that function into the
common module, since I can imagine this is something we could use from
time to time. What do you think?

> +
>  # CIFS helper functions
>  
>  sub cifs_is_mounted : prototype($$) {
> @@ -19,8 +24,16 @@ sub cifs_is_mounted : prototype($$) {
>      my ($mountpoint, $server, $share) = $scfg->@{ 'path', 'server', 'share' };
>      my $subdir = $scfg->{subdir} // '';
>  
> +    for my $path ($share, $subdir) {

Is stripping share actually needed? The documented workflow 
(pvesm scan cifs <address>) never returns slashes, and since share 
is a single name rather than a path, slashes there look more like
invalid input than a formatting variant.

> +        $path =~ s/$RM_LEADING_SLASHES// if $path;
> +        $path =~ s/$RM_TRAILING_SLASHES// if $path;
> +    }
> +
>      $server = "[$server]" if Net::IP::ip_is_ipv6($server);
> -    my $source = "//${server}/$share$subdir";
> +    my $source = "//${server}";
> +    $source .= "/$share";
> +    $source .= "/$subdir" if $subdir;
> +
>      $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata;
>  
>      return $mountpoint if grep {
> @@ -83,8 +96,15 @@ sub cifs_mount : prototype($$$$$) {
>      my ($mountpoint, $server, $share, $options) = $scfg->@{ 'path', 'server', 'share', 'options' };
>      my $subdir = $scfg->{subdir} // '';
>  
> +    for my $path ($share, $subdir) {
> +        $path =~ s/$RM_LEADING_SLASHES// if $path;
> +        $path =~ s/$RM_TRAILING_SLASHES// if $path;
> +    }
> +
>      $server = "[$server]" if Net::IP::ip_is_ipv6($server);
> -    my $source = "//${server}/$share$subdir";
> +    my $source = "//${server}";
> +    $source .= "/$share";
> +    $source .= "/$subdir" if $subdir;
>  
>      my $cmd = ['/bin/mount', '-t', 'cifs', $source, $mountpoint, '-o', 'soft'];
>  
> @@ -285,7 +305,11 @@ sub deactivate_storage {
>  sub check_connection {
>      my ($class, $storeid, $scfg) = @_;
>  
> -    my $servicename = '//' . $scfg->{server} . '/' . $scfg->{share};
> +    my $share = $scfg->{share};
> +    $share =~ s/$RM_LEADING_SLASHES// if $share;
> +    $share =~ s/$RM_TRAILING_SLASHES// if $share;
> +
> +    my $servicename = '//' . $scfg->{server} . '/' . $share;
>  
>      my $cmd = ['/usr/bin/smbclient', $servicename, '-d', '0'];
>