* Re: [PATCH manager] fix #6855: ui: storage: add mount options field for nfs amd smb/cifs
2026-07-21 12:13 [PATCH manager] fix #6855: ui: storage: add mount options field for nfs amd smb/cifs Elias Huhsovitz
@ 2026-07-21 13:23 ` Max R. Carrara
0 siblings, 0 replies; 2+ messages in thread
From: Max R. Carrara @ 2026-07-21 13:23 UTC (permalink / raw)
To: Elias Huhsovitz, pve-devel
On Tue Jul 21, 2026 at 2:13 PM CEST, Elias Huhsovitz wrote:
> Add a text field for custom mount options to the NFS and CIFS
> storage configuration dialogs. This allows users to configure
> advanced mount parameters (like 'nconnect', 'rdma', or 'seal')
> directly via the web interface, without needing to use the CLI
> or manually edit storage.cfg.
>
> Reduce the usage of legacy JS practices, such as 'var' & "+ string
> concatination" in affected functions.
>
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
Gave this a quick spin as well; the new field shows up for each panel as
expected. I also edited the mount options of an existing NFS storage and
the *storage config* was updated as expected.
However, there are a few potential pitfalls here:
- When a user updates the mount options, how should the plugin react?
Right now, the mount is just kept as-is, as in, no re-mounting is
performed -- which is probably the best, since it would affect running
guests.
Do we want to add a hint in the UI that a remount of the share or a
reboot of the node is necessary, if the field was changed?
(Or something similar?)
--> Perhaps we should also do this when changing the NFS version
field?
- Duplicate options aren't handled -- while probably not something that
somebody will run into that easily, a little extra check for that
would certainly help, I feel.
- When setting an incorrect option, e.g. `port=42069`, and rebooting the
node (for completeness' sake), the storage won't connect to the NFS
share, even if the option is corrected later on. That's probably
something that we should handle here -- as in, if we couldn't mount
the storage yet, try again with the new options until it succeeds.
(I don't actually know off the top of my head why it doesn't do this
already, tbf.)
Other than that, this patch looks pretty nice -- some more comments
inline. Nice work so far!
> ---
> www/manager6/storage/CIFSEdit.js | 38 ++++++++++++++++---
> www/manager6/storage/NFSEdit.js | 65 ++++++++++++++++++++++----------
> 2 files changed, 77 insertions(+), 26 deletions(-)
>
> diff --git a/www/manager6/storage/CIFSEdit.js b/www/manager6/storage/CIFSEdit.js
> index 5fe3fefe..d34cd029 100644
> --- a/www/manager6/storage/CIFSEdit.js
> +++ b/www/manager6/storage/CIFSEdit.js
> @@ -121,7 +121,7 @@ Ext.define('PVE.storage.CIFSInputPanel', {
> onlineHelp: 'storage_cifs',
>
> onGetValues: function (values) {
> - let me = this;
> + const me = this;
>
> if (values.password?.length === 0) {
> delete values.password;
> @@ -133,11 +133,26 @@ Ext.define('PVE.storage.CIFSInputPanel', {
> delete values.subdir;
> }
>
> + if (values.options) {
> + values.options = values.options
> + .split(',')
> + .map((opt) => opt.trim())
> + .filter((opt) => opt !== '')
> + .join(',');
> + }
> +
> + if (!values.options) {
> + delete values.options;
> + if (!me.isCreate) {
> + values.delete = values.delete ? `${values.delete},options` : 'options';
> + }
> + }
> +
> return me.callParent([values]);
> },
>
> initComponent: function () {
> - var me = this;
> + const me = this;
>
> me.column1 = [
> {
> @@ -149,7 +164,7 @@ Ext.define('PVE.storage.CIFSInputPanel', {
> listeners: {
> change: function (f, value) {
> if (me.isCreate) {
> - let exportField = me.down('field[name=share]');
> + const exportField = me.down('field[name=share]');
> exportField.setServer(value);
> }
> },
> @@ -166,7 +181,7 @@ Ext.define('PVE.storage.CIFSInputPanel', {
> if (!me.isCreate) {
> return;
> }
> - var exportField = me.down('field[name=share]');
> + const exportField = me.down('field[name=share]');
> exportField.setUsername(value);
> },
> },
> @@ -181,7 +196,7 @@ Ext.define('PVE.storage.CIFSInputPanel', {
> minLength: 1,
> listeners: {
> change: function (f, value) {
> - let exportField = me.down('field[name=share]');
> + const exportField = me.down('field[name=share]');
> exportField.setPassword(value);
> },
> },
> @@ -213,7 +228,7 @@ Ext.define('PVE.storage.CIFSInputPanel', {
> listeners: {
> change: function (f, value) {
> if (me.isCreate) {
> - let exportField = me.down('field[name=share]');
> + const exportField = me.down('field[name=share]');
> exportField.setDomain(value);
> }
> },
> @@ -229,6 +244,17 @@ Ext.define('PVE.storage.CIFSInputPanel', {
> },
> ];
>
> + me.advancedColumn2 = [
> + {
> + xtype: 'textfield',
> + name: 'options',
> + fieldLabel: gettext('Mount Options'),
> + emptyText: 'e.g., rdma, seal',
The value for `emptyText` should probably be something like
`${gettext('Example')}: rdma, seal`, since we don't use "e.g." anywhere,
AFAIK. We do however use "Example:" in the username field of the Proxmox
Backup Server storage.
> + allowBlank: true,
> + deleteEmpty: !me.isCreate,
> + },
> + ];
> +
> me.callParent();
> },
> });
> diff --git a/www/manager6/storage/NFSEdit.js b/www/manager6/storage/NFSEdit.js
> index 72db156f..c21759d8 100644
> --- a/www/manager6/storage/NFSEdit.js
> +++ b/www/manager6/storage/NFSEdit.js
> @@ -57,7 +57,6 @@ Ext.define('PVE.storage.NFSScan', {
> me.callParent();
> },
> });
> -
> Ext.define('PVE.storage.NFSInputPanel', {
> extend: 'PVE.panel.StorageBase',
>
> @@ -66,25 +65,31 @@ Ext.define('PVE.storage.NFSInputPanel', {
> options: [],
>
> onGetValues: function (values) {
> - var me = this;
> -
> - var i;
> - var res = [];
> - for (i = 0; i < me.options.length; i++) {
> - let item = me.options[i];
> - if (!item.match(/^vers=(.*)$/)) {
> - res.push(item);
> - }
> + const me = this;
> + const res = [];
> +
> + if (values.options && values.options.trim() !== '') {
> + const opts = values.options.split(',');
> + opts.forEach((opt) => {
> + const cleanOpt = opt.trim();
> + if (cleanOpt !== '' && !cleanOpt.match(/^vers=(.*)$/)) {
> + res.push(cleanOpt);
> + }
> + });
> }
> +
> if (values.nfsversion && values.nfsversion !== '__default__') {
> - res.push('vers=' + values.nfsversion);
> + res.push(`vers=${values.nfsversion}`);
> }
> +
> delete values.nfsversion;
> - values.options = res.join(',');
> - if (values.options === '') {
> +
> + if (res.length > 0) {
> + values.options = res.join(',');
> + } else {
> delete values.options;
> if (!me.isCreate) {
> - values.delete = 'options';
> + values.delete = values.delete ? `${values.delete},options` : 'options';
> }
> }
>
> @@ -92,21 +97,33 @@ Ext.define('PVE.storage.NFSInputPanel', {
> },
>
> setValues: function (values) {
> - var me = this;
> + const me = this;
> if (values.options) {
> - me.options = values.options.split(',');
> - me.options.forEach(function (item) {
> - var match = item.match(/^vers=(.*)$/);
> + const opts = values.options
> + .split(',')
> + .map((item) => item.trim());
> +
> + // Extract "vers=..." for the combobox
> + opts.forEach((item) => {
> + const match = item.match(/^vers=(.*)$/);
> if (match) {
> values.nfsversion = match[1];
> }
> });
> +
> + // Filter out "vers=..." and empty strings so it doesn't appear duplicated
> + const otherOptions = opts.filter((item) => item !== '' && !item.match(/^vers=(.*)$/));
> + values.options = otherOptions.join(',');
> +
> + if (values.options === '') {
> + delete values.options;
> + }
> }
> return me.callParent([values]);
> },
>
> initComponent: function () {
> - var me = this;
> + const me = this;
>
> me.column1 = [
> {
> @@ -118,7 +135,7 @@ Ext.define('PVE.storage.NFSInputPanel', {
> listeners: {
> change: function (f, value) {
> if (me.isCreate) {
> - let exportField = me.down('field[name=export]');
> + const exportField = me.down('field[name=export]');
> exportField.setServer(value);
> exportField.setValue('');
> }
> @@ -143,6 +160,14 @@ Ext.define('PVE.storage.NFSInputPanel', {
> ];
>
> me.advancedColumn2 = [
> + {
> + xtype: 'textfield',
> + name: 'options',
> + fieldLabel: gettext('Mount Options'),
> + emptyText: 'e.g., nconnect=4, soft',
Same here as above, should be `${gettext(Example)}: nconnect=4, soft`
instead.
> + allowBlank: true,
> + deleteEmpty: !me.isCreate,
> + },
> {
> xtype: 'proxmoxKVComboBox',
> fieldLabel: gettext('NFS Version'),
^ permalink raw reply [flat|nested] 2+ messages in thread