[pve-devel] [PATCH storage 0/2] cifs plugin - add options parameter

[pve-devel] [PATCH storage 0/2] cifs plugin - add options parameter

[Stefan Hrdlicka]

This adds the options parameter to the CIFS plugin.

check propertyList is common for all plugins and it is therefore
not possible to have options twice with different descriptions. This patch moves
the options property from NFSPlugin.pm to the Plugin.pm level. The options
parameter has the same use for NFS & CIFS so it shouldn't be confusing
that they share it.

# pve-storage 
---
 PVE/Storage/CIFSPlugin.pm | 10 ++++++++--
 PVE/Storage/NFSPlugin.pm  |  4 ----
 PVE/Storage/Plugin.pm     |  6 ++++++
 3 files changed, 14 insertions(+), 6 deletions(-)

# pve-docs
---
 pve-storage-cifs.adoc | 5 +++++
 1 file changed, 5 insertions(+)


-- 
2.30.2

[pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920)

[Stefan Hrdlicka]

this makes it possible to add all mount options offered by mount.cifs
NFS & CIFS now share the options parameter since the use it for
the same prupose

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/Storage/CIFSPlugin.pm | 10 ++++++++--
 PVE/Storage/NFSPlugin.pm  |  4 ----
 PVE/Storage/Plugin.pm     |  6 ++++++
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index 982040a..e4ea320 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -64,7 +64,7 @@ sub get_cred_file {
 }
 
 sub cifs_mount {
-    my ($server, $share, $mountpoint, $storeid, $smbver, $user, $domain) = @_;
+    my ($server, $share, $mountpoint, $storeid, $smbver, $user, $domain, $options) = @_;
 
     $server = "[$server]" if Net::IP::ip_is_ipv6($server);
     my $source = "//${server}/$share";
@@ -80,6 +80,10 @@ sub cifs_mount {
 
     push @$cmd, '-o', defined($smbver) ? "vers=$smbver" : "vers=default";
 
+    if ($options) {
+        push @$cmd, '-o', $options;
+    }
+
     run_command($cmd, errmsg => "mount error");
 }
 
@@ -144,6 +148,7 @@ sub options {
 	mkdir => { optional => 1 },
 	bwlimit => { optional => 1 },
 	preallocation => { optional => 1 },
+	options => { optional => 1 },
     };
 }
 
@@ -223,6 +228,7 @@ sub activate_storage {
     my $path = $scfg->{path};
     my $server = $scfg->{server};
     my $share = $scfg->{share};
+    my $options = $scfg->{options};
 
     if (!cifs_is_mounted($server, $share, $path, $cache->{mountdata})) {
 
@@ -232,7 +238,7 @@ sub activate_storage {
 	    "directory '$path' does not exist\n" if ! -d $path;
 
 	cifs_mount($server, $share, $path, $storeid, $scfg->{smbversion},
-	    $scfg->{username}, $scfg->{domain});
+	    $scfg->{username}, $scfg->{domain}, $options);
     }
 
     $class->SUPER::activate_storage($storeid, $scfg, $cache);
diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
index 5bd7313..f777483 100644
--- a/PVE/Storage/NFSPlugin.pm
+++ b/PVE/Storage/NFSPlugin.pm
@@ -69,10 +69,6 @@ sub properties {
 	    description => "Server IP or DNS name.",
 	    type => 'string', format => 'pve-storage-server',
 	},
-	options => {
-	    description => "NFS mount options (see 'man nfs')",
-	    type => 'string',  format => 'pve-storage-options',
-	},
     };
 }
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 0a100b7..cf65b3c 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -180,6 +180,12 @@ my $defaultData = {
 	    default => 'metadata',
 	    optional => 1,
 	},
+	options => {
+	    description => "NFS/CIFS mount options (see 'man nfs' " .
+                "or 'man mount.cifs')",
+	    type => 'string',  format => 'pve-storage-options',
+            optional => 1,
+	},
     },
 };
 
-- 
2.30.2

Re: [pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920)

[Fiona Ebner]

Please use "fix #2920" as a prefix

Am 13.05.22 um 15:55 schrieb Stefan Hrdlicka:
> this makes it possible to add all mount options offered by mount.cifs
> NFS & CIFS now share the options parameter since the use it for
> the same prupose
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>

Just style nits :), so you can add
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
after addressing them.

> ---
>  PVE/Storage/CIFSPlugin.pm | 10 ++++++++--
>  PVE/Storage/NFSPlugin.pm  |  4 ----
>  PVE/Storage/Plugin.pm     |  6 ++++++
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index 982040a..e4ea320 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -64,7 +64,7 @@ sub get_cred_file {
>  }
>  
>  sub cifs_mount {
> -    my ($server, $share, $mountpoint, $storeid, $smbver, $user, $domain) = @_;
> +    my ($server, $share, $mountpoint, $storeid, $smbver, $user, $domain, $options) = @_;
>  
>      $server = "[$server]" if Net::IP::ip_is_ipv6($server);
>      my $source = "//${server}/$share";
> @@ -80,6 +80,10 @@ sub cifs_mount {
>  
>      push @$cmd, '-o', defined($smbver) ? "vers=$smbver" : "vers=default";
>  
> +    if ($options) {
> +        push @$cmd, '-o', $options;
> +    }

Style nit: could be a single line with post-if

> +
>      run_command($cmd, errmsg => "mount error");
>  }
>  
> @@ -144,6 +148,7 @@ sub options {
>  	mkdir => { optional => 1 },
>  	bwlimit => { optional => 1 },
>  	preallocation => { optional => 1 },
> +	options => { optional => 1 },
>      };
>  }
>  
> @@ -223,6 +228,7 @@ sub activate_storage {
>      my $path = $scfg->{path};
>      my $server = $scfg->{server};
>      my $share = $scfg->{share};
> +    my $options = $scfg->{options};

Style nit: single-use variable that can be avoided by using
$scfg->{options} directly below.

>  
>      if (!cifs_is_mounted($server, $share, $path, $cache->{mountdata})) {
>  
> @@ -232,7 +238,7 @@ sub activate_storage {
>  	    "directory '$path' does not exist\n" if ! -d $path;
>  
>  	cifs_mount($server, $share, $path, $storeid, $scfg->{smbversion},
> -	    $scfg->{username}, $scfg->{domain});
> +	    $scfg->{username}, $scfg->{domain}, $options);

Style nit: already pre-existing problem, but nowadays, we put each
argument on a single line once it gets too long.

>      }
>  
>      $class->SUPER::activate_storage($storeid, $scfg, $cache);
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 5bd7313..f777483 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -69,10 +69,6 @@ sub properties {
>  	    description => "Server IP or DNS name.",
>  	    type => 'string', format => 'pve-storage-server',
>  	},
> -	options => {
> -	    description => "NFS mount options (see 'man nfs')",
> -	    type => 'string',  format => 'pve-storage-options',
> -	},
>      };
>  }
>  
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 0a100b7..cf65b3c 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -180,6 +180,12 @@ my $defaultData = {
>  	    default => 'metadata',
>  	    optional => 1,
>  	},
> +	options => {
> +	    description => "NFS/CIFS mount options (see 'man nfs' " .
> +                "or 'man mount.cifs')",

Style nit: wrong indentation

> +	    type => 'string',  format => 'pve-storage-options',
> +            optional => 1,

here as well

> +	},
>      },
>  };
>  

[pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920)

[Stefan Hrdlicka]

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
this depends on 1/2 since this changes the documentation :)

 pve-storage-cifs.adoc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/pve-storage-cifs.adoc b/pve-storage-cifs.adoc
index bb4b902..60775a4 100644
--- a/pve-storage-cifs.adoc
+++ b/pve-storage-cifs.adoc
@@ -57,6 +57,10 @@ path::
 
 The local mount point. Optional, defaults to `/mnt/pve/<STORAGE_ID>/`.
 
+options::
+
+CIFS mount options (see `man mount.cifs`).
+
 .Configuration Example (`/etc/pve/storage.cfg`)
 ----
 cifs: backup
@@ -64,6 +68,7 @@ cifs: backup
 	server 10.0.0.11
 	share VMData
 	content backup
+	options noserverino,cache=loose
 	username anna
 	smbversion 3
 
-- 
2.30.2

Re: [pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920)

[Fiona Ebner]

Am 13.05.22 um 15:55 schrieb Stefan Hrdlicka:
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
> this depends on 1/2 since this changes the documentation :)
> 
>  pve-storage-cifs.adoc | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/pve-storage-cifs.adoc b/pve-storage-cifs.adoc
> index bb4b902..60775a4 100644
> --- a/pve-storage-cifs.adoc
> +++ b/pve-storage-cifs.adoc
> @@ -57,6 +57,10 @@ path::
>  
>  The local mount point. Optional, defaults to `/mnt/pve/<STORAGE_ID>/`.
>  
> +options::
> +
> +CIFS mount options (see `man mount.cifs`).

Maybe add "Additional" and briefly mention that we set some options like
username, vers, etc. based on the other settings?

> +
>  .Configuration Example (`/etc/pve/storage.cfg`)
>  ----
>  cifs: backup
> @@ -64,6 +68,7 @@ cifs: backup
>  	server 10.0.0.11
>  	share VMData
>  	content backup
> +	options noserverino,cache=loose

Reading the man page, cache=loose sounds rather dangerous! I'd rather
have the example use something that can't lead to data corruption in
some scenarios :S

>  	username anna
>  	smbversion 3
>