From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 76E05910B9
for <pve-devel@lists.proxmox.com>; Fri, 26 Jan 2024 16:23:06 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 4E975379F7
for <pve-devel@lists.proxmox.com>; Fri, 26 Jan 2024 16:23:06 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pve-devel@lists.proxmox.com>; Fri, 26 Jan 2024 16:23:05 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C3095492E9
for <pve-devel@lists.proxmox.com>; Fri, 26 Jan 2024 16:23:04 +0100 (CET)
Message-ID: <e450eedb-e938-40cf-99b5-2dd9d93de8a2@proxmox.com>
Date: Fri, 26 Jan 2024 16:23:03 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Filip Schauer <f.schauer@proxmox.com>
References: <20231121102100.118669-1-f.schauer@proxmox.com>
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <20231121102100.118669-1-f.schauer@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.074 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
T_SCC_BODY_TEXT_LINE -0.01 -
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [result.data, proxmox.com]
Subject: Re: [pve-devel] [PATCH v2 manager] ui: lxc: add edit window for
device passthrough
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>
X-List-Received-Date: Fri, 26 Jan 2024 15:23:06 -0000
Am 21.11.23 um 11:21 schrieb Filip Schauer:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
High-level comment:
- would be nice to have the default values as emptyText for
UID/GID/Acces Mode and maybe also have an example text /dev/XYZ for the path
- if I add /dev/doesnotexist I'll get an error but it'll still be added
to the configuration
- I think the device index should be selectible too, to make it more
consistent with adding a mount point
- There's quite a bit of empty space on the right side in the advanced
options, maybe move one of the fields there?
- The menu entry for "Add" should be hidden or disabled for users
without sufficient permissions.
- Maybe add a warning, because it can be dangerous. Should it even be
exposed in the UI?
- Style nit: 'var' shouldn't be used, see
https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 9f44e560..f028685b 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1605,6 +1605,17 @@ Ext.define('PVE.Utils', {
> }
> },
>
> + dev_count: 256,
> +
> + forEachDev: function(func) {
> + for (let i = 0; i < PVE.Utils.dev_count; i++) {
> + let cont = func(i);
> + if (!cont && cont !== undefined) {
> + return;
> + }
> + }
> + },
> +
> hardware_counts: {
> net: 32,
> usb: 14,
Maybe it's time to finally split PVE.Utils? dev_count and forEachDev is
really too generic of a name for such a module, should be at least
mention "lxc".
> diff --git a/www/manager6/lxc/DeviceEdit.js b/www/manager6/lxc/DeviceEdit.js
> new file mode 100644
> index 00000000..1ee4f309
> --- /dev/null
> +++ b/www/manager6/lxc/DeviceEdit.js
> @@ -0,0 +1,166 @@
> +Ext.define('PVE.lxc.DeviceInputPanel', {
> + extend: 'Proxmox.panel.InputPanel',
> + mixins: ['Proxmox.Mixin.CBind'],
> +
> + autoComplete: false,
> +
> + cbindData: function(initialConfig) {
> + let me = this;
> + if (!me.pveSelNode) {
> + throw "no pveSelNode given";
> + }
> +
> + return { nodename: me.pveSelNode.data.node };
> + },
> +
> + viewModel: {
> + data: {},
> + },
> +
> + setVMConfig: function(vmconfig) {
> + var me = this;
> + me.vmconfig = vmconfig;
> + },
> +
> + onGetValues: function(values) {
> + var me = this;
> + if (!me.confid) {
> + let max_devices = 256;
Could use the dev_count from PVE.Util
> + for (let i = 0; i < max_devices; i++) {
> + let id = 'dev' + i.toString();
> + if (!me.vmconfig[id]) {
> + me.confid = id;
> + break;
> + }
> + }
> + }
> + var val = values.path;
> + delete values.path;
> +
> + if (values.mode) {
> + val += ',mode=' + values.mode;
> + }
> + delete values.mode;
> +
> + if (values.uid) {
> + val += ',uid=' + values.uid;
> + }
> + delete values.uid;
> +
> + if (values.gid) {
> + val += ',gid=' + values.gid;
> + }
> + delete values.gid;
> +
I think you can use PVE.Parser.printPropertyString() to save some lines
here.
> + values[me.confid] = val;
> + return values;
> + },
> +
> + items: [
> + {
> + xtype: 'textfield',
> + type: 'device',
> + name: 'path',
> + cbind: { pveSelNode: '{pveSelNode}' },
I might be missing something, but isn't this a normal ExtJS text field?
Does this cbind have any actual consequences? Same for the others.
> + editable: true,
> + allowBlank: false,
> + fieldLabel: gettext('Device Path'),
> + labelAlign: 'right',
> + validator: function(value) {
> + if (value.startsWith('/dev/')) {
> + return true;
> + }
> +
> + return "Path has to start with /dev/";
> + },
> + },
> + ],
> +
>
---cut---
> + me.load({
> + success: function(response, options) {
> + ipanel.setVMConfig(response.result.data);
> + if (me.isCreate) {
> + return;
> + }
> +
> + let data = PVE.Parser.parsePropertyString(response.result.data[me.confid], 'path');
> + let path, mode, uid, gid;
> + path = data.path;
> + mode = data.mode;
> + uid = data.uid;
> + gid = data.gid;
> +
> + var values = {
> + path,
> + mode,
> + uid,
> + gid,
> + };
> +
> + ipanel.setValues(values);
Couldn't you pass data directly? Or at least make constructing values
quite a bit shorter ;)