From: "Max R. Carrara" <m.carrara@proxmox.com>
To: "Elias Huhsovitz" <e.huhsovitz@proxmox.com>,
<pve-devel@lists.proxmox.com>
Subject: Re: [PATCH manager] fix #6855: ui: storage: add mount options field for nfs amd smb/cifs
Date: Tue, 21 Jul 2026 15:23:10 +0200 [thread overview]
Message-ID: <DK4A5D7GDH1S.1WY2YTP8AF8MG@proxmox.com> (raw)
In-Reply-To: <20260721121334.115027-1-e.huhsovitz@proxmox.com>
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'),
prev parent reply other threads:[~2026-07-21 13:23 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DK4A5D7GDH1S.1WY2YTP8AF8MG@proxmox.com \
--to=m.carrara@proxmox.com \
--cc=e.huhsovitz@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox