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.k...@proxmox.com>
Tested-by: Daniel Kral <d.k...@proxmox.com>
On 3/4/25 12:58, Markus Frank wrote:
Signed-off-by: Markus Frank <m.fr...@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
