From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 19B0B1FF0E3 for ; Tue, 21 Jul 2026 15:23:45 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A919D214B4; Tue, 21 Jul 2026 15:23:44 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 21 Jul 2026 15:23:10 +0200 Message-Id: Subject: Re: [PATCH manager] fix #6855: ui: storage: add mount options field for nfs amd smb/cifs To: "Elias Huhsovitz" , From: "Max R. Carrara" X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20260721121334.115027-1-e.huhsovitz@proxmox.com> In-Reply-To: <20260721121334.115027-1-e.huhsovitz@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784640164083 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.034 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: TRX7GWWRWDOE5XCPBWI2MMAVXM5TTW3J X-Message-ID-Hash: TRX7GWWRWDOE5XCPBWI2MMAVXM5TTW3J X-MailFrom: m.carrara@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 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=3D42069`, 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/CIFS= Edit.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 =3D this; > + const me =3D this; > > if (values.password?.length =3D=3D=3D 0) { > delete values.password; > @@ -133,11 +133,26 @@ Ext.define('PVE.storage.CIFSInputPanel', { > delete values.subdir; > } > > + if (values.options) { > + values.options =3D values.options > + .split(',') > + .map((opt) =3D> opt.trim()) > + .filter((opt) =3D> opt !=3D=3D '') > + .join(','); > + } > + > + if (!values.options) { > + delete values.options; > + if (!me.isCreate) { > + values.delete =3D values.delete ? `${values.delete},opti= ons` : 'options'; > + } > + } > + > return me.callParent([values]); > }, > > initComponent: function () { > - var me =3D this; > + const me =3D this; > > me.column1 =3D [ > { > @@ -149,7 +164,7 @@ Ext.define('PVE.storage.CIFSInputPanel', { > listeners: { > change: function (f, value) { > if (me.isCreate) { > - let exportField =3D me.down('field[name=3Dsh= are]'); > + const exportField =3D me.down('field[name=3D= share]'); > exportField.setServer(value); > } > }, > @@ -166,7 +181,7 @@ Ext.define('PVE.storage.CIFSInputPanel', { > if (!me.isCreate) { > return; > } > - var exportField =3D me.down('field[name=3Dshare]= '); > + const exportField =3D me.down('field[name=3Dshar= e]'); > exportField.setUsername(value); > }, > }, > @@ -181,7 +196,7 @@ Ext.define('PVE.storage.CIFSInputPanel', { > minLength: 1, > listeners: { > change: function (f, value) { > - let exportField =3D me.down('field[name=3Dshare]= '); > + const exportField =3D me.down('field[name=3Dshar= e]'); > exportField.setPassword(value); > }, > }, > @@ -213,7 +228,7 @@ Ext.define('PVE.storage.CIFSInputPanel', { > listeners: { > change: function (f, value) { > if (me.isCreate) { > - let exportField =3D me.down('field[name=3Dsh= are]'); > + const exportField =3D me.down('field[name=3D= share]'); > exportField.setDomain(value); > } > }, > @@ -229,6 +244,17 @@ Ext.define('PVE.storage.CIFSInputPanel', { > }, > ]; > > + me.advancedColumn2 =3D [ > + { > + 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/NFSEd= it.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 =3D this; > - > - var i; > - var res =3D []; > - for (i =3D 0; i < me.options.length; i++) { > - let item =3D me.options[i]; > - if (!item.match(/^vers=3D(.*)$/)) { > - res.push(item); > - } > + const me =3D this; > + const res =3D []; > + > + if (values.options && values.options.trim() !=3D=3D '') { > + const opts =3D values.options.split(','); > + opts.forEach((opt) =3D> { > + const cleanOpt =3D opt.trim(); > + if (cleanOpt !=3D=3D '' && !cleanOpt.match(/^vers=3D(.*)= $/)) { > + res.push(cleanOpt); > + } > + }); > } > + > if (values.nfsversion && values.nfsversion !=3D=3D '__default__'= ) { > - res.push('vers=3D' + values.nfsversion); > + res.push(`vers=3D${values.nfsversion}`); > } > + > delete values.nfsversion; > - values.options =3D res.join(','); > - if (values.options =3D=3D=3D '') { > + > + if (res.length > 0) { > + values.options =3D res.join(','); > + } else { > delete values.options; > if (!me.isCreate) { > - values.delete =3D 'options'; > + values.delete =3D values.delete ? `${values.delete},opti= ons` : 'options'; > } > } > > @@ -92,21 +97,33 @@ Ext.define('PVE.storage.NFSInputPanel', { > }, > > setValues: function (values) { > - var me =3D this; > + const me =3D this; > if (values.options) { > - me.options =3D values.options.split(','); > - me.options.forEach(function (item) { > - var match =3D item.match(/^vers=3D(.*)$/); > + const opts =3D values.options > + .split(',') > + .map((item) =3D> item.trim()); > + > + // Extract "vers=3D..." for the combobox > + opts.forEach((item) =3D> { > + const match =3D item.match(/^vers=3D(.*)$/); > if (match) { > values.nfsversion =3D match[1]; > } > }); > + > + // Filter out "vers=3D..." and empty strings so it doesn't a= ppear duplicated > + const otherOptions =3D opts.filter((item) =3D> item !=3D=3D = '' && !item.match(/^vers=3D(.*)$/)); > + values.options =3D otherOptions.join(','); > + > + if (values.options =3D=3D=3D '') { > + delete values.options; > + } > } > return me.callParent([values]); > }, > > initComponent: function () { > - var me =3D this; > + const me =3D this; > > me.column1 =3D [ > { > @@ -118,7 +135,7 @@ Ext.define('PVE.storage.NFSInputPanel', { > listeners: { > change: function (f, value) { > if (me.isCreate) { > - let exportField =3D me.down('field[name=3Dex= port]'); > + const exportField =3D me.down('field[name=3D= export]'); > exportField.setServer(value); > exportField.setValue(''); > } > @@ -143,6 +160,14 @@ Ext.define('PVE.storage.NFSInputPanel', { > ]; > > me.advancedColumn2 =3D [ > + { > + xtype: 'textfield', > + name: 'options', > + fieldLabel: gettext('Mount Options'), > + emptyText: 'e.g., nconnect=3D4, soft', Same here as above, should be `${gettext(Example)}: nconnect=3D4, soft` instead. > + allowBlank: true, > + deleteEmpty: !me.isCreate, > + }, > { > xtype: 'proxmoxKVComboBox', > fieldLabel: gettext('NFS Version'),