Re: [pve-devel] [PATCH manager v14 12/12] ui: add options to add virtio-fs to qemu config

[Daniel Kral <d.kral@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