From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 4FDB41FF17C
for <inbox@lore.proxmox.com>; Wed, 2 Apr 2025 15:42:25 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id CB9BE1BC23;
Wed, 2 Apr 2025 15:42:14 +0200 (CEST)
Message-ID: <ee7ce747-64d4-4340-a61f-9d5d0991e565@proxmox.com>
Date: Wed, 2 Apr 2025 15:42:11 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Markus Frank <m.frank@proxmox.com>
References: <20250304115803.194820-1-m.frank@proxmox.com>
<20250304115803.194820-13-m.frank@proxmox.com>
Content-Language: en-US
From: Daniel Kral <d.kral@proxmox.com>
In-Reply-To: <20250304115803.194820-13-m.frank@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.011 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH manager v14 12/12] ui: add options to add
virtio-fs to qemu config
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Some smaller higher level comments inline.
Else the patch looks good to me and with those addressed, consider this as:
Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>
On 3/4/25 12:58, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> v14:
> * disable expose-xattr when expose-acl is set
> * added missing writeback cache option
>
> www/manager6/Makefile | 1 +
> www/manager6/Utils.js | 1 +
> www/manager6/qemu/HardwareView.js | 19 +++++
> www/manager6/qemu/VirtiofsEdit.js | 137 ++++++++++++++++++++++++++++++
> 4 files changed, 158 insertions(+)
> create mode 100644 www/manager6/qemu/VirtiofsEdit.js
>
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index fabbdd24..fdf0e816 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -271,6 +271,7 @@ JSSRC= \
> qemu/Smbios1Edit.js \
> qemu/SystemEdit.js \
> qemu/USBEdit.js \
> + qemu/VirtiofsEdit.js \
> sdn/Browser.js \
> sdn/ControllerView.js \
> sdn/Status.js \
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 90011a8f..0f242ae1 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1645,6 +1645,7 @@ Ext.define('PVE.Utils', {
> serial: 4,
> rng: 1,
> tpmstate: 1,
> + virtiofs: 10,
> },
>
> // we can have usb6 and up only for specific machine/ostypes
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index c6d193fc..34aeb51e 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -319,6 +319,16 @@ Ext.define('PVE.qemu.HardwareView', {
> never_delete: !caps.nodes['Sys.Console'],
> header: gettext("VirtIO RNG"),
> };
> + for (let i = 0; i < PVE.Utils.hardware_counts.virtiofs; i++) {
> + let confid = "virtiofs" + i.toString();
> + rows[confid] = {
> + group: 50,
> + order: i,
> + iconCls: 'folder',
> + editor: 'PVE.qemu.VirtiofsEdit',
> + header: gettext('Virtiofs') + ' (' + confid +')',
> + };
> + }
>
> var sorterFn = function(rec1, rec2) {
> var v1 = rec1.data.key;
> @@ -595,6 +605,7 @@ Ext.define('PVE.qemu.HardwareView', {
> const noVMConfigDiskPerm = !caps.vms['VM.Config.Disk'];
> const noVMConfigCDROMPerm = !caps.vms['VM.Config.CDROM'];
> const noVMConfigCloudinitPerm = !caps.vms['VM.Config.Cloudinit'];
> + const noVMConfigOptionsPerm = !caps.vms['VM.Config.Options'];
>
> me.down('#addUsb').setDisabled(noHWPerm || isAtUsbLimit());
> me.down('#addPci').setDisabled(noHWPerm || isAtLimit('hostpci'));
> @@ -604,6 +615,7 @@ Ext.define('PVE.qemu.HardwareView', {
> me.down('#addRng').setDisabled(noSysConsolePerm || isAtLimit('rng'));
> efidisk_menuitem.setDisabled(noVMConfigDiskPerm || isAtLimit('efidisk'));
> me.down('#addTpmState').setDisabled(noVMConfigDiskPerm || isAtLimit('tpmstate'));
> + me.down('#addVirtiofs').setDisabled(noVMConfigOptionsPerm || isAtLimit('virtiofs'));
> me.down('#addCloudinitDrive').setDisabled(noVMConfigCDROMPerm || noVMConfigCloudinitPerm || hasCloudInit);
>
> if (!rec) {
> @@ -748,6 +760,13 @@ Ext.define('PVE.qemu.HardwareView', {
> disabled: !caps.nodes['Sys.Console'],
> handler: editorFactory('RNGEdit'),
> },
> + {
> + text: gettext("Virtiofs"),
> + itemId: 'addVirtiofs',
> + iconCls: 'fa fa-folder',
> + disabled: !caps.nodes['Sys.Console'],
> + handler: editorFactory('VirtiofsEdit'),
> + },
> ],
> }),
> },
> diff --git a/www/manager6/qemu/VirtiofsEdit.js b/www/manager6/qemu/VirtiofsEdit.js
> new file mode 100644
> index 00000000..0bbb5213
> --- /dev/null
> +++ b/www/manager6/qemu/VirtiofsEdit.js
> @@ -0,0 +1,137 @@
> +Ext.define('PVE.qemu.VirtiofsInputPanel', {
> + extend: 'Proxmox.panel.InputPanel',
> + xtype: 'pveVirtiofsInputPanel',
> + onlineHelp: 'qm_virtiofs',
> +
> + insideWizard: false,
> +
> + onGetValues: function(values) {
> + var me = this;
> + var confid = me.confid;
> + var params = {};
> + delete values.delete;
> + params[confid] = PVE.Parser.printPropertyString(values, 'dirid');
> + return params;
> + },
> +
> + setSharedfiles: function(confid, data) {
> + var me = this;
> + me.confid = confid;
> + me.virtiofs = data;
> + me.setValues(me.virtiofs);
> + },
> + initComponent: function() {
> + let me = this;
> +
> + me.nodename = me.pveSelNode.data.node;
> + if (!me.nodename) {
> + throw "no node name specified";
> + }
> + me.items = [
> + {
> + xtype: 'pveDirMapSelector',
> + emptyText: 'dirid',
> + nodename: me.nodename,
> + fieldLabel: gettext('Directory ID'),
> + name: 'dirid',
> + allowBlank: false,
> + },
> + {
> + xtype: 'proxmoxKVComboBox',
> + fieldLabel: gettext('Cache'),
> + name: 'cache',
> + value: '__default__',
> + deleteDefaultValue: false,
> + comboItems: [
> + ['__default__', Proxmox.Utils.defaultText + ' (auto)'],
> + ['auto', 'auto'],
> + ['always', 'always'],
> + ['never', 'never'],
> + ],
> + },
> + {
> + xtype: 'proxmoxcheckbox',
> + fieldLabel: gettext('Writeback cache'),
> + name: 'writeback',
> + },
> + {
> + xtype: 'proxmoxcheckbox',
> + fieldLabel: gettext('expose-xattr'),
nit: "Enable xattr support" or "Expose extended attributes" / "Expose
xattrs" could be a little user friendlier
> + name: 'expose-xattr',
> + },
> + {
> + xtype: 'proxmoxcheckbox',
> + fieldLabel: gettext('expose-acl (implies expose-xattr)'),
nit: "Enable POSIX ACLs support" or "Expose POSIX ACLs" as above
I'm also thinking about if it wouldn't be a little clearer for users if
either:
- 'expose-acl' is grayed out until 'expose-xattr' is enabled (maybe
indent the 'expose-attr' option if possible to indicate that the one
needs the other), or
- 'expose-xattr' is grayed out and set to true below if 'expose-acl' is
enabled
AFAICS if virtiofsd is started with --xattr it only makes any xattr()
filesystem calls fail with ENOSYS, and is automatically enabled if
--posix-acl (or --security-label for that matter) is provided, so
providing both instead of only one of them on the cmdline should make no
difference...
> + name: 'expose-acl',
> + listeners: {
> + change: function(f, value) {
> + let xattr = me.down('field[name=expose-xattr]');
> + xattr.setDisabled(value);
> + if (value) {
> + xattr.setValue(0);
...so `xattr.setValue(1);` should be possible here.
> + }
> + },
> + },
> + },
nit: and on another note, I think at least both of the xattr options
(maybe also Direct IO?) could be put in "Advanced" section to make this
a little shorter for the most important options.
> + {
> + xtype: 'proxmoxcheckbox',
> + fieldLabel: gettext('Direct IO'),
nit: users could benefit if this is called "Allow Direct IO" as only
"Direct IO" could imply that all IO done by the guest is suddenly done
with {O,IOCB}_DIRECT, which is obviously not the case.
> + name: 'direct-io',
> + },
> + ];
> +
> + me.virtiofs = {};
> + me.confid = 'virtiofs0';
> + me.callParent();
> + },
> +});
> +
> +Ext.define('PVE.qemu.VirtiofsEdit', {
> + extend: 'Proxmox.window.Edit',
> +
> + subject: gettext('Filesystem Passthrough'),
> +
> + initComponent: function() {
> + var me = this;
> +
> + me.isCreate = !me.confid;
> +
> + var ipanel = Ext.create('PVE.qemu.VirtiofsInputPanel', {
> + confid: me.confid,
> + pveSelNode: me.pveSelNode,
> + isCreate: me.isCreate,
> + });
> +
> + Ext.applyIf(me, {
> + items: ipanel,
> + });
> +
> + me.callParent();
> +
> + me.load({
> + success: function(response) {
> + me.conf = response.result.data;
> + var i, confid;
> + if (!me.isCreate) {
> + var value = me.conf[me.confid];
> + var virtiofs = PVE.Parser.parsePropertyString(value, "dirid");
> + if (!virtiofs) {
> + Ext.Msg.alert(gettext('Error'), 'Unable to parse virtiofs options');
> + me.close();
> + return;
> + }
I'm not sure about this here, but shouldn't this ever be reachable since
the virtiofs entry will automatically disappear as soon as the virtiofs
options are invalid?
But doesn't mean this shouldn't be here.
> + ipanel.setSharedfiles(me.confid, virtiofs);
> + } else {
> + for (i = 0; i < PVE.Utils.hardware_counts.virtiofs; i++) {
> + confid = 'virtiofs' + i.toString();
> + if (!Ext.isDefined(me.conf[confid])) {
> + me.confid = confid;
> + break;
> + }
> + }
> + ipanel.setSharedfiles(me.confid, {});
> + }
> + },
> + });
> + },
> +});
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel