* [pve-devel] [PATCH storage 0/2] cifs plugin - add options parameter @ 2022-05-13 13:55 Stefan Hrdlicka 2022-05-13 13:55 ` [pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920) Stefan Hrdlicka 2022-05-13 13:55 ` [pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920) Stefan Hrdlicka 0 siblings, 2 replies; 5+ messages in thread From: Stefan Hrdlicka @ 2022-05-13 13:55 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920) 2022-05-13 13:55 [pve-devel] [PATCH storage 0/2] cifs plugin - add options parameter Stefan Hrdlicka @ 2022-05-13 13:55 ` Stefan Hrdlicka 2022-07-28 10:26 ` Fiona Ebner 2022-05-13 13:55 ` [pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920) Stefan Hrdlicka 1 sibling, 1 reply; 5+ messages in thread From: Stefan Hrdlicka @ 2022-05-13 13:55 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920) 2022-05-13 13:55 ` [pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920) Stefan Hrdlicka @ 2022-07-28 10:26 ` Fiona Ebner 0 siblings, 0 replies; 5+ messages in thread From: Fiona Ebner @ 2022-07-28 10:26 UTC (permalink / raw) To: pve-devel, Stefan Hrdlicka 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 > + }, > }, > }; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920) 2022-05-13 13:55 [pve-devel] [PATCH storage 0/2] cifs plugin - add options parameter Stefan Hrdlicka 2022-05-13 13:55 ` [pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920) Stefan Hrdlicka @ 2022-05-13 13:55 ` Stefan Hrdlicka 2022-07-28 10:26 ` Fiona Ebner 1 sibling, 1 reply; 5+ messages in thread From: Stefan Hrdlicka @ 2022-05-13 13:55 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920) 2022-05-13 13:55 ` [pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920) Stefan Hrdlicka @ 2022-07-28 10:26 ` Fiona Ebner 0 siblings, 0 replies; 5+ messages in thread From: Fiona Ebner @ 2022-07-28 10:26 UTC (permalink / raw) To: pve-devel, Stefan Hrdlicka 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-28 10:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-13 13:55 [pve-devel] [PATCH storage 0/2] cifs plugin - add options parameter Stefan Hrdlicka 2022-05-13 13:55 ` [pve-devel] [PATCH storage 1/2] add options parameter to cifs plugin (fixes #2920) Stefan Hrdlicka 2022-07-28 10:26 ` Fiona Ebner 2022-05-13 13:55 ` [pve-devel] [PATCH docs 2/2] cifs: add options parameter (fix #2920) Stefan Hrdlicka 2022-07-28 10:26 ` Fiona Ebner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.